Created attachment 92213 [details] [review]
first version of label fix for ARIA widgets

Mike: this is a formal patch for this issue and should supersede the hacked patch I delivered to you via email.

Created attachment 92214 [details] [review]
first version of label fix for ARIA widgets

Mike: this is a formal patch for this issue and should supersede the hacked patch I delivered to you via email.

Created attachment 92215 [details] [review]
first version of label fix for ARIA widgets

Mike: this is a formal patch for this issue and should supersede the hacked patch I delivered to you via email.

Sorry about repeated comments.  Bugzilla timed out but still entered entry.  Strange.

This does seem to work better but the aria stuff will be pretty tricky to test until the keyboard conflicts between orca navigation and application functionality get resolved.

A fix for the key binding contention problem is in the works.  In the meantime you can use orca+F12 and orca+z to turn off caret and structural navigation.

See http://bugzilla.gnome.org/show_bug.cgi?id=459618 for a patch for the key binding contention problem.

I think the best solution here would be to get the Firefox folks to fix the label for/by relations.  If they do that, we wouldn't need to patch Orca.  :-)

Right now, using obj.name (if it exists) as the label for the object might break things where the label for/by property is really working.  So, the patch is probably good for testing until they fix their stuff, but is not something that should be checked in.  Thanks!

I don't think we're waiting for the Firefox folks to fix stuff in this instance.   If the 

    aaa:labelledby="label" 

is present, we seem to get the labelled_by/label_for pair.  Just like with

    <label></label>

If that is not present we don't.  We only expect labelled_by/label_for when the page creator explicitly put a label in there. :-)

ARIA widgets seem to derive their name from the text enclosed in the tags that define the widget.  Thus:

    <li role="wairole:checkbox">Hello World</li>
 
Will give you a checkbox where obj.name == "Hello World".

    <li role="wairole:radio">Hello World</li>

Will give you a radio button where obj.name == "Hello World".

    <span role="role:radio">Yes</span>

Will give you a radio button where obj.name == "Yes".

And so on, and so forth.  Mind you it seems that more stuff goes into the tags; this is just for illustrative purposes.

Therefore, it seems to me that we do want to use obj.name but instead do that work in guessTheLabel() -- just like we do with other form-control-like objects which lack officially defined labels.

Right now, guessTheLabel() looks at obj.name as a last resort.  And *for non-ARIA widgets* I believe that is still what we want to do.  However, for ARIA widgets, I think the first thing guessTheLabel() should guess is obj.name.  So what about leaving getDisplayedLabel() alone and adding a check at the top of guessTheLabel() to see if it's an ARIA widget and, if so, set guess to obj.name (stripping out the whitespace)?

Thoughts?

The example 2 XHTML checkbox example at http://test.cita.uiuc.edu/aria/checkbox/xhtml.php?title=Checkbox%20Example%202&ginc=includes/checkbox2a.inc&gcss=css/checkbox2.css&gjs=js/checkbox2a.js,../js/enable_app.js,../js/globals.js contradicts comment #9.  The following markup snippet results in 'Romaine Lettuce' in the acc.name field and no labeled by relation.

<li id="cb1" 
      role="wairole:checkbox" 
      aaa:checked="false" 
      aaa:labelledby="cb1label"
      aaa:describedby="test1"
      tabindex="0">
      <img src="../images/unchecked.gif" alt="unchecked checkbox"/>Lettuce
</li>

<p id="cb1label" class="hidden">Romaine Lettuce</p>


If there was ever a time when the labeled by relation should be present it is when the aaa:labelledby attribute is included.  I feel that the Firefox developers should fix this if it is truly broken.  Joanie, can you post a test page demonstrating what you have seen?

What happens if you use that very same example but nuke the class="hidden" from the paragraphs (Or change the class name. Or leave it alone but change the associated style.)? When I do that two things happen:

1. The formerly-hidden paragraphs suddenly appear in the hierarchy.  (They were absent before.)

2.  The labelled_by/label_for relationship appears between the paragraph and the checkbox as expected.

If you look at the associated style sheet you will see the following:

    .hidden{

        display:none;

    }

So I guess the question is, should objects with the display:none style still appear in the hierarchy?  If so, they don't and there's your bug.  If they should not appear in the hierarchy then I would think that the relationship can't be made.

Excellent work.  That is an odd quandary and I don't know the answer.  

Also, note that the acc.name field is still coming from the paragraph that is serving as the label and not the inline text associated with the widget.  Do you have an example that shows a different result?

> Excellent work.  That is an odd quandary and I don't know the answer.  

I have mixed feelings about it.  I wouldn't mind them showing up in the hierarchy IF their extents were (0, 0, 0, 0) and they lacked STATE_VISIBLE.  That said....

If the accessible hierarchy should match the DOM, then they should be exposed.  If the accessible hierarchy should match what the user can potentially interact with, then I would think display:none; should keep things out of the hierarchy.  Which is a long way of saying I don't know the answer either. :-)  So as has become the Orca team custom, I'm slapping Will's name in the summary. :-)  He'll have an answer. :-)

> Also, note that the acc.name field is still coming from the paragraph that is
> serving as the label and not the inline text associated with the widget.  Do
> you have an example that shows a different result?

Yeah, take the same example you posted and nuke aaa:labelledby="whatever". :-)  You should find that the name becomes a combination of the alternative text (unchecked checkbox) and the displayed text (sprouts).  Looks like labels rule if present.

> I have mixed feelings about it.  I wouldn't mind them showing up in the
> hierarchy IF their extents were (0, 0, 0, 0) and they lacked STATE_VISIBLE. 
> That said....
> 
> If the accessible hierarchy should match the DOM, then they should be exposed. 
> If the accessible hierarchy should match what the user can potentially interact
> with, then I would think display:none; should keep things out of the hierarchy.

In the GUI toolkit world (GTK+, Java, OOo, etc.), we run into this a lot.  If the object is in the widget hierarchy, then it generally shows up in the AT-SPI hierarchy.  STATE_VISIBLE and STATE_SHOWING are then used to indicate whether the thing is burning phosphorous or not.  For example, menu items for menus that aren't popped up will still appear in the hierarchy.  I think the same thing goes for objects scrolled out of view in a scrollpane.

What this situation brings, however, is a little different beast.  We have a case where the thing is never ever intended to be shown, but it potentially influences information obtainable via the AT-SPI.  Weird and it's bizarre people want to go out of their way to do something so odd.  It's like setting the accessible name of a "Delete all files" pushbutton to "OK".  In these cases, I'd rather we act like a screen reader (i.e., read what's on the screen).

So....

1. We don't want these things to appear in the hierarchy (right?).  And they're not appearing in the hierarchy.

2.  If we want to read what's on the screen, perhaps we should go with obj.text instead of obj.name?

A cursory glance at some of the examples (including those with the modifications I suggested in earlier comments) indicates that for ARIA widgets the displayed text can be found in obj.text.  This might include EMBEDDED OBJECT CHARACTERS (if the displayed text includes a graphic that is serving as the checkbox, presumably if the displayed text is a link, etc.)  I came across this when I did the label guessing and created expandEOCs() for this purpose.

(In reply to comment #15)
> 2.  If we want to read what's on the screen, perhaps we should go with obj.text
> instead of obj.name?

The labelledby property gets mapped into the obj.name field even when the label is hidden.  Using obj.text may causes problems, especially with textboxes.  In this case the inputted text will be returned using IText.getText() and mis-identified as the label.  I think we have stumbled upon an odd example and hopefully will not be the way developers label form controls.  

Right.... Right.... But if I'm interpreting Will's comment correctly

> the accessible name of a "Delete all files" pushbutton to "OK".  In these
> cases, I'd rather we act like a screen reader (i.e., read what's on the
> screen).

We only want to present the name if it's the displayed text.  If there's a label, it won't be the displayed text.  So.... Does that mean what we want the Mozilla guys to do is NOT expose the label as the name?  You know, that might solve the problems we're discussing.  You could then safely grab the name knowing that it's what's on the screen.  We could get then get the label, should the spirit move us to do so, by looking at the object pointed at by labelled_by.  I think that would mean that the only thing we couldn't access are labels that are hidden.   And -- to paraphrase that forest bit -- if I label falls hidden in a document, is it really labelling anything? ;-)

Just some thoughts....

Yes, but in Will's example the web developer created what I would consider an oddity.  For safety sake, I understand the point.  I do like the idea of Firefox not exposing the labeledby relation text in the name field.  As you state, this would allow us to safely grab the name field if the labeledby relation can't be made due to a hidden obj.  Using the name field as a first guess is what we want.  I think obj.text and expandEOCs() should be avoided due to the added roundtrip(s) of IText.getText().

Created attachment 92490 [details] [review]
second version of label fix for ARIA widgets

This one uses the Iaccessible.name field as a first guess for the label.

> 1. We don't want these things to appear in the hierarchy (right?).  And they're
> not appearing in the hierarchy.

I guess.  They are very odd things.  I wonder what the motivation for them really is.
 
> 2.  If we want to read what's on the screen, perhaps we should go with obj.text
> instead of obj.name?

Well...we should go for obj.text of the thing with text.  In looking at the "guessTheLabel", we it seems as though it should be returning the string of the thing holding the text.  Are we running into a case here where there's more than one thing holding the string for the label (i.e., the text of the thing doing the labelling and the name of the object being labelled)?

I think I'm still horribly confused and don't understand the full problem yet.  :-(  I'll look for you two on #orca tomorrow (I need to run now).

(In reply to comment #19)
> Created an attachment (id=92490) [edit]
> second version of label fix for ARIA widgets
> 
> This one uses the Iaccessible.name field as a first guess for the label.
> 

It seems as though you may want to first check if this is an ARIA widget.  Otherwise, this patch seems like it will completely disrupt the logic for the rest of the method:

+        # The first guess will be the accessible.name field
+        guess = obj.name.strip()
+        if guess:
+            return guess

I don't believe it is a problem the way it is.  Checking if it is an ARIA widget will cost us a roundtrip so I would like to avoid it.  Is there and HTML widget issue that I am not aware of?

Created attachment 92498 [details]
test case

What should Orca "guess" for this unlabeled entry?  Tab into the entry without your patch; then do it with the patch.

According to http://developer.mozilla.org/en/docs/Accessible_DHTML#Supported_roles "title" is suppose to get mapped to the name field for ARIA widgets as well.  So what else gets mapped there?  Why is our original label scheme not working for some of the ARIA widgets?  

>  So what else gets mapped there?  

Couldn't tell you.

> Why is our original label scheme not working for some
> of the ARIA widgets?  

Well, I haven't taken the time to put in debugging code into the various guess methods while examining ARIA widgets, but I suspect one culprit can be found in guessLabelFromLine().  As I commented there:

        # [[[TODO: JD: Nearby text that's not actually in the form may need
        # to be ignored.  Let's try that for now and adjust based on feedback
        # and testing.]]]
        #
        leftIsInForm = self.getContainingRole(onLeft, rolenames.ROLE_FORM)
        rightIsInForm = self.getContainingRole(onRight, rolenames.ROLE_FORM)

This was added as a direct result of quite a few "false positives" where we were guessing text that was awful close to the form field but which wasn't part of the form.   Mike's feedback was that this solved quite a few problems.  However, looking at the examples you've pointed to, we don't see ROLE_FORM. 

All of our form-related stuff (including structural navigation) assumes traditional "web 1.0" forms.  Now that we are implementing support for web 2.0 content, such assumptions are no longer valid. You will notice that there is a method isFormField().  Aside from not wanting to duplicate code, that method is there so that we can easily adjust our definition of what constitutes a form field to address these very conditions.  For that same reason, I tried to minimize the number of places we actual look at ROLE_FORM outside of isFormField().

Hope this helps!

(In reply to comment #22)
> I don't believe it is a problem the way it is.  Checking if it is an ARIA
> widget will cost us a roundtrip so I would like to avoid it.  Is there and HTML
> widget issue that I am not aware of?

The patch is saying "no matter what, if the name is set to any non-empty non-whitespace string, it is the label."  I'm really not sure it is safe to make this assumption for every object we'll ever encounter.  I'm not 100% sure that the *name* will *always* reflect what is being displayed as the *label* for every single object we encounter.  We might be sure of this for ARIA widgets, but I don't think we can be so sure of this for everything else.

Created attachment 92554 [details] [review]
third version of label fix for ARIA widgets

this version does not try to guess the label for an ARIA widget.  It just calls default:guessTheLabel() which currently returns None.

> this version does not try to guess the label for an ARIA widget.  It just calls
> default:guessTheLabel() which currently returns None.

Please check to make sure default.py:guessTheLabel exists (I don't think it does).  If not, please create it as part of this patch.  Thanks!  :-)

Created attachment 92568 [details] [review]
third version of label fix for ARIA widgets

Must have grabbed the wrong patch.  

this version does not try to guess the label for an ARIA widget.  It just calls
default:guessTheLabel() which currently returns None.

That looks pretty much like what we discussed on IRC.  :-)  Yeah!  Does it work OK?

Created attachment 92577 [details] [review]
fourth version of label fix for ARIA widgets

I'm glad you asked if it worked OK.  Further testing showed that it was not completely fixed.  The test pages that did get fixed have extra focus events that triggered the correct announcement.  However, other test pages that previously had problems did not have these focus events.  They ended up not announcing the label.  The new patch is a slightly different approach.  It involves using IText.getText() but stripping out the embed characters.  The result is an announcement of what is presented on the screen without the verbosity/overhead of using expandEOCs().  

In the future, an ARIA textfield may need to be an exception to the rule in getAriaLabel().

> announcing the label.  The new patch is a slightly different approach.  It
> involves using IText.getText() but stripping out the embed characters.  The
> result is an announcement of what is presented on the screen without the
> verbosity/overhead of using expandEOCs().  

I'm confused.  :-(  There may be some terminology disconnect, so I just want to make sure:

* For objects that display text on their own (e.g., ARIA checkboxes, text areas, buttons, etc.), we have a method called getDisplayedText.  This method returns the text that the object itself is displaying.  

* Labels are really separate objects that label other objects.  To get those labels, we have getDisplayedLabel.

So, for things like checkboxes, text areas and buttons:

"red" checkbox:  [x] red
unlabelled text:   [howdy_______]
OK button:          [OK]

There is no label for any of the above and getDisplayedText should report the text ("red" for the checkbox, "howdy" for the text area, and "OK" for the button).  getDisplayedLabel should return None for these.

For things like labelled text areas:

Say something: [howdy______]

getDisplayedText should still return "howdy" and getDisplayedLabel should return "Say something:".  

So...I'm very confused about why guessAriaLabel is taking the text from the object itself.  It seems as though this is really implementing getDisplayedText and not getDisplayedLabel.

> announcing the label.  The new patch is a slightly different approach.  It
> involves using IText.getText() but stripping out the embed characters. 

Just out of curiosity, how will that handle cases where the "label" that's not a label is composed of both text and links?  Like:

[x] Lettuce from _Whole_Foods_
[x] Tomatoes from _Shaws_

Where _Whole_Foods_ and _Shaws_ are each links to the respective stores' sites?  In those instances the functional label is the whole thing, i.e. "Lettuce from Whole Foods" and "Tomatoes from Shaws."   If you strip out the embedded object characters, will that turn the guessed labels into "Lettuce from" and "Tomatoes from"? 

    We are dealing with the rare case when ARIA widgets do not have a relation
    defining the label.  In these cases, you indicated that you wanted to announce
    what was on the screen.  I thought we decided that the displayed text served as
    a label in these cases.

    During navigation of my test widgets, getDisplayedText() is not called.  So
    navigation of these widgets yield no label announcement if guessTheLabel()
    returned None as in the third patch.  Without a patch the completely wrong
    label is announced.  I agree, getDisplayedText()text could be used in
    getAriaLabel().  I was unaware of it's existance.

    In lieu of fixing the guessing strategy, we have resorted to using the name or
    IText().  I'm open to any other strategy, but I'm about tapped out of ideas on
    this one.

    Here is a test case so you can see the error:
    http://test.cita.uiuc.edu/aria/radio/xhtml.php?title=Radio%20Example%201&ginc=includes/radio1a.inc&gcss=css/radio1a.css&gjs=../js/globals.js,../js/enable_app.js,js/radio1a.js 

    Use the arrow keys to move between the radio buttons, tab to move between
    groups.

Joanie,

You make a great point, but where do you stop?  How many children could be hanging from the obj of interest.  This could have a huge impact on verbosity/performance for just a label.  Would the user be able to use read current line to get the complete announcement?

> You make a great point, but where do you stop?  How many children could be
> hanging from the obj of interest.  

One would hope not all that many. :-) And if the EOCs aren't there, you're not descending anything.  So I would think it wouldn't be too painful.  But I could be wrong. <shrugs>

> verbosity/performance for just a label.  Would the user be able to use read
> current line to get the complete announcement?

Presumably yes.  Assuming the "label" is on the current line and not above it or below it; also assuming there are not other objects on that same line that are irrelevant to the form control with focus.  When there's other, irrelevant stuff on the same line, that places the user in the position of having to filter out what doesn't apply to the control.  Sighted users do this quite easily by examining the visual appearance and position of the text.  It's harder non-visually.   

The reason we have the current label guessing code in place, inefficient as it is, is because users -- who have always had the option of doing as you suggest and reading the current line (or the previous line, or in rare cases the next line) -- complained that we were not reading form field controls with focus.   Users, it would seem, don't want to look around.  I don't blame them. :-)

As I told you when we were chatting about this on the phone, life would be way, way better if content creators could be bothered to properly label their blessed form controls. :-)

Anyhoo, just my 2 cents worth....

(In reply to comment #36)
> > You make a great point, but where do you stop?  How many children could be
> > hanging from the obj of interest.
> 
> One would hope not all that many. :-) And if the EOCs aren't there, you're not
> descending anything.  So I would think it wouldn't be too painful.  But I could
> be wrong. <shrugs>
> 

Fine by me if you are confident in the accessible hierarchy.  So what are you proposing, returning expandEOCs() if it is an ARIA widget label guess?  Sounds good!  Let's put this one to rest and move on :)

> Fine by me if you are confident in the accessible hierarchy.  

ROTFL.

Given that not even a fortnight has passed since the Great A11y Hierarchy Obliteration of 2007, that's an awful low blow Scott.
;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-)
;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-)
;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-) ;-)

(Nothin' personal Aaron ;-) )

> So what are you proposing, returning expandEOCs() if it is an 
> ARIA widget label guess?  

I am proposing that we do not dismiss the validity of EOCs in the "label" we "guess" -- at least not without giving it careful consideration.  Assuming we conclude these EOC's should indeed be part of the "label", how we go about ensuring  their inclusion (using the name and doing some checking, using  the text and doing some EOC expanding, going some third route as yet to be determined) is something I will defer to you and Will on because you've been looking at it in depth; I have not.  

>  Let's put this one to rest and move on :)

Agreed!  Will's added to my assigned bug list and I'd best get to those.  :-)

When I test this patch, I'm seeing very bad verbosity problems and braille isn't showing the information that it should be showing. Here's an example when I arrow down to the Rainbow Gardens radion button on http://test.cita.uiuc.edu/aria/radio/xhtml.php?title=Radio%20Example%201&ginc=includes/radio1a.inc&gcss=css/radio1a.css&gjs=../js/globals.js,../js/enable_app.js,js/radio1a.js:

SPEECH OUTPUT: 'Rainbow Gardens selected radio button'
VISIBLE:  'Rainbow Gardens'
SPEECH OUTPUT: 'Rainbow Gardens selected radio button radio button'

This should be more along the lines of:

SPEECH OUTPUT: 'Rainbow Gardens selected radio button'
VISIBLE:  '&=y Rainbow Gardens RadioButton'

Unless I'm not applying/testing the patch right, I think more work is needed on this.  :-(

In playing around with this some more, I'm also wondering if we might need a bit more extensive patch.  Something Joanie was questioning in IRC rings a bell now: I'm wondering if we do need to delegate to the super classes of the SpeechGenerator and BrailleGenerator in Gecko's overridden generator methods -- Gecko's overridden generators are to handle HTML-ish widgets embedded in HTML.  We don't want those for ARIA widgets -- we want to treat ARIA widgets like GTK+ widgets.

In addition, I'm wondering if we need to add some extra special isAriaWidget decisions in getUtterancesFromContents and updateBraille.  For example, something like the following in the loop that goes through each object in the line contents: "if isAriaWidget(obj): get the stuff from the generator immediately and avoid any other special logic used to handle HTML oddities".

We'll probably also need some tests that embed multiple ARIA widgets on the same line and intermingle them with plain old HTML text on the same line.  

BTW, I tried adding just the following to the top of inDocumentContent.  It got us closer, but I don't think it is the right thing to do:

         if not obj:
             obj = orca_state.locusOfFocus
+
+        # Treat ARIA widgets like regular default.py widgets
+        #
+        if self.isAriaWidget(obj):
+            return False

Created attachment 92646 [details] [review]
Patch to treat ARIA widgets like regular GTK+ widgets (default.py widgets)

Here's a patch for consideration.  The main work that needed to be done was to circumvent the code in Gecko.py that was dealing with the assumption that we were working with XUL/HTML widgets.  The circumvention merely just defers to the superclass of the speech and braille generators (i.e., the default generators) instead of using the custom speech and braille generators.  

There's also an onFocus modification that ignores focus: events on ARIA widgets if we managed to get there by up/down arrowing to them.  The reason for this is that the up/down arrowing to an ARIA widget causes two events t be delivered: focus and caret-moved.  Without the modification, the focus event handler would speak the object that just got focus, and the caret-moved event handler would speak/braille the entire line.  This would result in the ARIA widget being spoken twice.  By ignoring the focus: event when up/down arrow is used, the modification limits the presentation to just once.

This patch seems to be in the direction I think we want to go, but it definitely needs a lot more testing.  We need to work with a page that has mixtures of HTML/XUL widgets combined with ARIA widgets, and a page that also combines multiple combinations of these things single lines.   Scott, can you work on that?

I am glad you saw the problem associated with the multiple events.  I was going to bring this to your attention.  Do you foresee any major issues here since implementations of ARIA widgets can be so different?

Thank you for posting a patch demonstrating the fix you described.  It will help me understand not only this problem, but the architecture as well.

I will no problem constructing the test web page(s) you described.  Talk to you tomorrow.

Created attachment 92712 [details]
simple mixed widget lines test case

gzipped file contains a simple test case with mixed widgets (HTML, ARIA) on the same line.

The following link is the display:hidden label example that we have previously addressed.  (Firefox does not expose hidden obj and does not create labelledby relation):  

http://test.cita.uiuc.edu/aria/checkbox/xhtml.php?title=Checkbox%20Example%202&ginc=includes/checkbox2a.inc&gcss=css/checkbox2.css&gjs=js/checkbox2a.js,../js/enable_app.js,../js/globals.js

The latest patch fails to output what is on the screen due to the same issue we saw before, namely, the describedby field mapping to the acc.name field.  The solution, as we spoke about in irc with Aaron yesterday, is for Firefox to expose the hidden obj and set state to NOTVISIBLE and complete the labelledby relation.  I will post a Mozilla bug if this is the direction we want to go.

Beside the above issue, the patch seems good.  Off to test dojo widgets with it.

Patch committed to HEAD.  This is not slated for GNOME 2.20.

I believe this patch has stood the test of time and should be marked fixed.