Created attachment 95822 [details] [review]
Take 1

After some discussion with Mike, here is take 1 at a solution based on my understanding of what he would like to see. 

The idea is this:

1. You cannot accidentally wander into a focusable HTML list simply by arrowing.  Doing so could lead to users changing values without meaning to and/or wandering for days (in the case of our very own bugzilla).

2. If we stumble across an HTML focusable list, set the caret just before that list and announce it.  The user, knowing that a focusable list comes next, has two options:  Give it focus by pressing Tab, or move on by using whatever navigation the spirit moves him to use.

Notes:

1.  This does not address the issue of how does one escape out of a list with focus.  That's another bug/RFE/refactor.

2. I have not yet tested this with ARIA, Dojo, and friends.  I believe we won't stomp on any non-HTML widgets, but I might be totally wrong. :-)  Once we can agree on the desired user interaction and get the kinks worked out of it, I'll look at web 2.0 stuff more closely and enlist Scott's aid to be sure.

Mike please let me know if this is in the ballpark of what you were looking for, and also please test all things related to list navigation.  Funny what a little change to a couple of methods can do.... ;-)

So far I'm really liking this patch.  I'll keep on testing but I think it is where we want to go.  
thanks Joanie for doing it

Created attachment 95908 [details] [review]
slight tweaks based on Will's feedback

1. Capitalize the "l" in "lists with..." so as not to introduce a new string in _getSpeechForList()

2. Don't assume that just because the selected item's name is the same as the label for the list we don't want to display it in braille (i.e. we might have a list whose label is "General" and the selected item in that list might be "General".

Created attachment 95909 [details]
test case

Will pointed out that not all pages with a bunch of lists will be like bugzilla. ;-)

The attached has the following structure:
1. A line of text.
2. A line of labels. The first two are purely functional, i.e. just text
   whose position suggests association with the list. The third is an
   honest-to-goodness label.
3. A line of lists.

Mike, please test the patch with this test case.  Is this the desired user experience?

One thing I noticed is that in braille the properly labelled list's label is displayed.

The current behavior of this patch is correct.with the exception of braille labels which are defined correctly as labels.  The label should not appear twice.  It should only appear on the line above the lists.

Created attachment 96041 [details]
revised version of the test case with a couple more lists

Created attachment 96042 [details] [review]
more changes

> The current behavior of this patch is correct.with the exception of braille
> labels which are defined correctly as labels.  

Actually.... I found some more things that were not correct. The changes to find{Next,Previous}CaretInOrder() cause us to display the row of lists if you arrow to the first item in a list.  The way we've been handling focused lists up to this point is to display just the focused item.  Therefore, I changed the patch to be consistent with that.  Mike is that the desired behavior?

The change to _getSpeechForList() -- in which we only speak the label if the list has focus -- keeps us from speaking the label if it's a proper label and on the same line as the list.  Compare the last two lists in the new test case using the old patch. To deal with this, I now have isLabellingContents() return the object being labelled or None (as opposed to True or False) so that we can do role-specific checks in getUtterancesFromContents().  

And this patch also addresses the change Mike specified.

Disclaimers:

1. While this patch seems to work.... I'm tired.  I need to look at this patch more closely when I'm not.  Mike, I'd appreciate your testing in the meantime.

2.  Still haven't looked at what impact, if any, this will have on Aria and friends.

Created attachment 96044 [details] [review]
whitespace nits

I just looked over the patch and could spot where I had added debugging statements.  The code hasn't changed in this version, but I know I'll forget to clean things up if I don't do it now.

ARIA lists are truly a different beast.  For convenience sake I have attached a test case which has an ARIA listbox tacked onto the end of the test case given here.   

What I do know is this.  setCaretPosition() receives a list item object as opposed to a list as seen for HTML lists.  This results in the list getting focus instead of the document frame.  If I change the code and set focus on the doc frame, caret navigation continues down the list, stopping at each list item.   I will spend more time on this once my regression tests are finished.

Here are some errors that Will found for ARIA lists.
http://bugzilla.gnome.org/show_bug.cgi?id=468968#c8

Created attachment 96230 [details]
HTML and ARIA lists

Note that this is an XHTML example

I'm quite happy with this functionallity in HTML.  I know you guys have more that you'd like to do with aria but I'm taking my name off for now.

Feel free to close this bug when you feel HTML lists are supported properly.  I already have this bug http://bugzilla.gnome.org/show_bug.cgi?id=473611 open for the ARIA list issues.

Update/Note to self:

Scott is going to make some changes to the patch to deal with ARIA widgets.  After that, I need to add some comments to better document why the characterOffset of the list is the EOC representing the list rather than the offset of text inside the list -- i.e. we're going against convention for lists.

Created attachment 96328 [details]
test case with lists at various levels of hierarchy

Test case that has HTML lists at different levels of hierarchy including in form, in table (and form), and in document frame.  Also includes additional text before and after some of the lists.

Created attachment 96330 [details] [review]
first version of ARIA additions

The new test case demonstrates a problem with the previous patch.  To test do the following:

1) Position caret somewhere on the paragraph "This is a paragraph outside the form".
2) Arrow down.  The list information will be output, but the caret moves to the top paragraph.
3) Press tab.  This will take you to the wrong list.

I realize that lists will probably appear in forms but this is far from certain.  The problem lies in the following code within setCaretPosition()

if obj.role == rolenames.ROLE_LIST \
           and obj.state.count(atspi.Accessibility.STATE_FOCUSABLE):
            obj = obj.parent

I don't believe we can simply use the parent here.  I think we need to use the previous caret position instead.  I have changed this code to now read

if obj.role == rolenames.ROLE_LIST \
           and obj.state.count(atspi.Accessibility.STATE_FOCUSABLE):
            [obj, characterOffset] = self.findPreviousCaretInOrder(obj=obj,
                                     includeNonText=False)

This seems to fix the problem for this test case but I am not entirely sure about other cases.  In addition, there is now a problem of speaking the first list item of the ARIA list when you arrow past it and land on the text entry.

Created attachment 96332 [details] [review]
second version with ARIA lists

This version fixes the labeling problem for the text entry that caused 'Green' to be announced.  However there is still one issue.  Do the following to see the problem.

1) Position caret anywhere above the group of lists, In " Another paragraph in the form" will be fine.
2) Arrow down through the table of lists.  The following exception will occur when you reach the 'Lions, Tigers' list.

Traceback (most recent call last):

+ Trace 166222

This results because of the workaround to bug 433527 in setCaretPosition(), namely

if obj.role == rolenames.ROLE_LABEL:
    obj = obj.parent

Removing this code fixes the exception problem.

I'm not sure what the right answer is, but here's why that is there:

In our very own bugzilla, there is some significant bogusity going on with the use of <label></label>.  In particular, in many cases it surrounds the object it labels (thus the object it labels is a child of the label).  That is bug 433527 and was filed back in April.  No one has touched it.

So who cares?  Why are labels a problem anyway?  We like labels, right? ;-)  ;-) ;-)  See this Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=397219.  We think we're updating our position when we set it to the label, but then when you press Tab and expect to land on the list that's immediately in front of you, you suddenly find yourself elsewhere.

The long and the short of it is this: It's a hack. ;-)  If it's removed without a replacement hack, our new handling of focusable lists may have some unpredictable results in gnome's bugzilla (i.e. when Mike and our users are interacting with forms), but I don't think the broader "in the wild" impact will be significant -- or even noticeable. <shrugs>

Created attachment 96333 [details] [review]
third version with ARIA lists

This version removes the bugzilla hack.

Mike, can you please test the latest patch "third version with ARIA lists"

Also, Will has some questions about the use of 'select' with single selection lists.

(In reply to comment #19)
> Mike, can you please test the latest patch "third version with ARIA lists"
> 
> Also, Will has some questions about the use of 'select' with single selection
> lists.
> 

Just spoke with Mike, he's OK with dropping the new _("select") string.  So, as long as thorough testing shows this is good, we should commit it to trunk and gnome-2-20.

Created attachment 96456 [details] [review]
fourth version with ARIA lists

Removed _("select") as requested.

This looks good (thanks!).  Mike - we still need a comment from you regarding the "remove the hack" portion that's described in comment 17.

Committed to trunk.

Waiting on further testing before committing to gnome-2-20.

If you look at my test case from Comment 6:  Using the last version of the patch I did, you could reliably arrow right up to each of those lists (the caret even visibly moved there) and, having done so, press Tab to move focus into those lists.  Now when arrow around on the line that contains just the three lists, the caret seems to remain in front of the colon at the end of the previous line.  If I right arrow to the second or third list on the that line and press Tab, I always land in the first list. 

To see if this was the result of removing the mozilla hack, I reversed the committed patch, applied my last patch, and removed the mozilla hack.  I could still arrow right up to each list and use Tab to reliably give it focus.  So it's something else that changed between then and now....

ATM I want to get back to figuring out what I need to do to rework the magnification patch I should have already finished by now. :-(  But I will look at this some more tomorrow unless (as Rich might say) Scott doesn't beat me to it. ;-)

The problem is caused by the following code in setCaretPosition():

if obj.role == rolenames.ROLE_LIST \
           and obj.state.count(atspi.Accessibility.STATE_FOCUSABLE):
            [obj, characterOffset] = self.findPreviousCaretInOrder(obj=obj,
                                     includeNonText=False)

The whitespace nits patch originally had obj=obj.parent in place of the findPreviousCaretInOrder() call.  However, using obj=obj.parent causes the problems outlined in comment #15.  I am not sure what the solution is yet, but I think something along the lines of  findPreviousCaretInOrder() is correct.  

What is interesting is that passing includeNonText=True to findPreviousCaretInOrder() returns a list even though the object that is passed in is the same list.  I will investigate this more tomorrow.

Created attachment 96568 [details] [review]
fifth version with ARIA lists

This version uses findPreviousObject() instead of findPreviousCaretPosition() and fixes the problem Joanie brought up.  However, I am still not sure if this is the optimal solution.  The following steps will illustrate my concern.

1) Open up the latest test case.
2) Arrow down from the top and note the caret position as you go to the list directly after 'A paragraph in the form'.  The caret remains at the same place as the list is announced.
3) Arrow down (or right) one more time.  The caret will move to 'Another paragraph in the form' and skip the list.

The issue that concerns me is the right arrowing just after you have down arrowed to announce the first list.  It is intuitive to a sighted user that the caret should move to the next character in 'A paragraph in the form'.  However the caret jumps past the list.  I think this is the correct behavior but I am not entirely sure.

This version improves things, but in the HTML test case, moving to just before the label in the fourth line and pressing tab moves focus to the wrong list.  This seems to be the result of the Mozilla label bug.  Not sure what we want to do here:  wait for the bug to be fixed by Mozilla or not set the caret position on the label in setCaretPostion()?

Reassigning this to Scott since he's the one working on the patch most recently.  :-)

Created attachment 96582 [details] [review]
sixth version with ARIA lists

This version uses the following in the questionable block within setCaretPosition():

if obj.role == rolenames.ROLE_LIST \
           and obj.state.count(atspi.Accessibility.STATE_FOCUSABLE):
            characterOffset = self.getCharacterOffsetInParent(obj)
            obj = obj.parent

The problem is the characterOffset must be reset when the object is being reset, in this case to the parent.  I am not sure if the intension was to pass in the correct characterOffset and reset only the obj within setCaretPosition().  I believe these two steps should stay together.  The new patch seems to work on the test case, but will need to be edited.

Created attachment 96636 [details] [review]
seventh version with ARIA lists

This version removes blocks of code from find[Next|Prev]CaretInOrder() that deals with special handling of lists.  I do not believe these code blocks were ever executed because of the elif includeNonText and (startOffset < 0) ... clause above the block.  I believe the logic the removed block was trying to accomplish, namely getting both the obj and characterOffset correct for lists, is now done in setCaretPosition().  

There does not seem to be any other blocks of code that can be removed.  Orca continues to perform quite well on the latest test case.

Created attachment 96654 [details] [review]
eighth version with ARIA lists

Will noticed a problem when right arrowing to the ARIA list from '...instead of focusing it' where the caret would be sent back to a label within the table above.  This was caused by the next item, the entry in this case, not having state SHOWING set.  Instead of state SHOWING, I used VISIBLE.  This seems to work fine but Will says SHOWING and VISIBLE are often confused.  This fix is specific to Gecko and they seem to have it right so I don't see a problem here.

Another issue that arose was that the list was not being announced when right arrowing to it.  I added a check for the embed character in speakCharacterAtOffset() which fixed this problem.

I may be incorrect in saying that only VISIBLE is necessary.  It may be correct for entries but who knows if that is valid for all object types.  A good defense would be to use SHOWING or VISIBLE.

goNextLine, goPrevLine, goNextWord, goPrevWord , goNextCharacter and goPrevCharacter all pass the test case.

However, goNextList and goPrevList fail.

Are these not the types of lists a user would expect to find using 'l' or 'shift-l'?  If they should be found, then these lines of code are wrong in getNextList().

if obj and not (obj.state.count(atspi.Accessibility.STATE_FOCUSABLE)):
                found = True

Should the 'not' be removed?  If this truly is an error, then we have the problem of skipping over a previously found list.  Repeated getNext|PrevList find the same list.  If this is not an error, maybe we should add some unordered lists to our test case and consider including the test case in tests/html.

> Are these not the types of lists a user would expect to find using 'l' or
> 'shift-l'?  

They are not.  See the translator note, for instance:

        self.inputEventHandlers["goNextListHandler"] = \
            input_event.InputEventHandler(
                Script.goNextList,
                # Translators: this is for navigating between bulleted/numbered
                # lists in HTML
                #
                _("Goes to next list."))

We have form field structural navigation for lists of this sort.

If I'm not mistaken everything is now done for this bug.  I think it is finished.  

This one has been previously committed to trunk and seems good.