468418 - Expose level for nested lists in HTML

Status: VERIFIED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Aaron Leventhal]

Attachment: Contains two nested lists, one <ul> and one <ol> (but of course you can mix as well)

For <ol> and <ul> we should expose the level as an object attribute and in the groupPosition() method.

Screen readers will have a much easier time providing users with relevant positional information about nested lists.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 352540 [details] [diff] [review]
patch

I'll put new patch with some code reorganization

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 4

[Aaron Leventhal]

Alex, since the code was moved it's hard to see the diff in the method. Can you provide a description of your patch to help the reviewers review efficiently?

Comment 5

[alexander :surkov (:asurkov)]

Aaron, actually the patch2 is like patch1, so reviewers are free to look at the patch1 to see what have been changed actually (to review the logic changes). For the patch2 I changed comments and structure of 'if' blocks in moved code therefore it's worth to take a look at patch2.

Comment 6

[Aaron Leventhal]

I never looked at patch1. I'm just interested in the difference between the new method and the original code. Can't you give me an overview of changes? It helps me a lot.

Comment 7

[alexander :surkov (:asurkov)]

Logic difference is I added new "} else if (aRole == nsIAccessibleRole::ROLE_LISTITEM) {" block. Even if you never looked at patch1 then you can do this I guess ;) - it will show you clearly what's new in logic.

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 352680 [details] [diff] [review]
patch2

r=me with a few nits:

>+    // Calculate group attributes basing or accessible hierarhy if they weren't
>+    // provided by ARIA or by accessible class implementation.

"basing or accessible hierarhy" should be "based on accessible hierarchy".

>+  // The role of an accessible can be pointed by ARIA attribute but ARIA

"can be specified by ARIA" is better wording here. I realize that this is mostly copied old code, but while you're here, please correct these.

>+  // posinset, level, setsize may be skipped. As well this method is used
>+  // for non ARIA accessibles to avoid GetAccessibleInternal() method
>+  // implementation in subclasses. For example, it's used to calculation group
>+  // attributes for HTML li elements.

"it's used to calculation" should be "it's being used to calculate".

>+  if (aRole != nsIAccessibleRole::ROLE_LISTITEM &
>+      aRole != nsIAccessibleRole::ROLE_MENUITEM &
>+      aRole != nsIAccessibleRole::ROLE_CHECK_MENU_ITEM &
>+      aRole != nsIAccessibleRole::ROLE_RADIO_MENU_ITEM &
>+      aRole != nsIAccessibleRole::ROLE_RADIOBUTTON &
>+      aRole != nsIAccessibleRole::ROLE_PAGETAB &
>+      aRole != nsIAccessibleRole::ROLE_OPTION &
>+      aRole != nsIAccessibleRole::ROLE_RADIOBUTTON &
>+      aRole != nsIAccessibleRole::ROLE_OUTLINEITEM)

checking for ROLE_RADIOBUTTON is duplicated: Once above ROLE_PAGETAB and once above ROLE_OUTLINEITEM. Checking once is enough. :-)

Comment 9

[Aaron Leventhal]

#1
+ // We don't handle the case when nested
+ // list having more complex structure, i.e. when there are accessibles
+ // between parent listitem/list and nested list.
Good. I don't think we want to. They would really be separate constructs in that case, anyway.

#2
Sometimes an author will leave out the <ul> or <li>, unfortunately. For these cases, I believe we will be off by 1.
Therefore, we are probably better off increasing groupLevel by counting parents with ROLE_LISTITEM than by counting parents with ROLE_LIST.

#3
+    if (groupLevel > 0)
+      groupLevel--;
What is this for?

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 11

[Aaron Leventhal]

Comment on attachment 353395 [details] [diff] [review]
patch

Thank you, good work.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 353395 [details] [diff] [review]
patch

> 		$(warning test_groupattrs.xul temporarily disabled) \
>+		test_groupattrs.html \

Please test if reenabling the XUL file still makes the tests pass on your machine. The bug that originally caused me to disable XUL tests has since been fixed, so we should be able to gradually reenable the XUL files and fix them up if the tests fail.

>+    addLoadEvent(doTest);

Should we use the new AddA11yLoadEvent here and in the test_groupattrs.xul file?

In addition, my review nits from comment #8 haven't been addressed yet in this version of the patch.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch4

with enabled XUL test + plus MarcoZ comments. I put this patch for try server build, let's see if XUL tests are pass.

I'm not sure we must use addA11yLoadEvent everywhere, let's use it when we can't without it.

Comment 14

[alexander :surkov (:asurkov)]

https://build.mozilla.org/tryserver-builds/2008-12-27_23:[e-mail]-1230447754/

Comment 15

[alexander :surkov (:asurkov)]

pushed http://hg.mozilla.org/mozilla-central/rev/965d47130dfd

Comment 16

[Marco Zehe (:MarcoZ)]

Verified fixed using the attached test case and build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090105 Minefield/3.2a1pre.