This was found by Victor Tsaran of Yahoo! in the Yahoo! Mail Classic interface. Their table of messages has a landmark role of "main", and our nsHTMLTableAccessible::isProbablyForLayout treats *any* table that has a "role" attribute as layout table unconditionally, see: http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLTableAccessible.cpp#1324

Original issue was reported against NVDA in this ticket: http://www.nvda-project.org/ticket/315. It should not be necessary for NVDA or Orca to try and implement their own layout table heuristics.

Thank you, Marco, for investigation. This behaviour has been introduced in bug 275010. This bug is related with bug 469688 where we should not expose table structure if some ARIA roles are used on table. I think we should expose table structure if ARIA navigational landmarks are used. But here we should decide what ARIA navigational landmarks means table is for layout. Here's the list http://www.w3.org/TR/wai-aria/#roleattribute_inherits.

I don't think any of the ARIA navigational landmark roles should mean a table is for layout. The landmark roles are clearly made to help the user navigate a page. So my proposal is: If any of these landmark roles are encountered, fall through and use other means of heuristically determining whether a table is for layout or not. In other words, if we determine that the role attribute is present, test if it points to any of the landmark roles from this list, and if that's not the case, consider the table for layout, but if any of these roles are present, continue with the function execution.

(In reply to comment #3) > In other words, if we determine that the role attribute is present, test if it > points to any of the landmark roles from this list, and if that's not the case, > consider the table for layout, but if any of these roles are present, continue > with the function execution. What about another landmarks, any ARIA roles that do not override native role like marque or timer? Should these roles override native role? Should these roles stop us from nsHTMLTableAccessible usage?

Yes, basically what David does for native role in bug 481114 should be applied here I think.

Taking.

1. If the role rule is to use the native role, don't treat this as a layout table. That way, roles such as "log", "main" etc. will currently never be treated as layout tables. 2. Add tests for our isProbablyForLayout implementation. 2.a Make the testAttrs function also handle unexpected attributes.

Comment on attachment 382941 [details] [diff] [review] Patch r=me, looks correct. I only found one nit: >+ if ((prop.key in aExpectedAttrs)) { Why double (( )) ? I didn't look at the layout guesser method overall, just your addition, for correctness. I do wonder if we can rewrite the layout guesser at some point (not now).

Comment on attachment 382941 [details] [diff] [review] Patch >- if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role)) { >- RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, and role is table"); >+ if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role) && >+ mRoleMapEntry->roleRule != kUseNativeRole) { if I'm not wrong then it's always false excepting the grid. That is what expected? >+ RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, no landmarks, and role is table"); >\ No newline at end of file nit: new line please

(In reply to comment #9) > (From update of attachment 382941 [details] [diff] [review]) > > >- if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role)) { > >- RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, and role is table"); > >+ if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role) && > >+ mRoleMapEntry->roleRule != kUseNativeRole) { > if I'm not wrong then it's always false excepting the grid. That is what > expected? Yes.

Grid is not layout table.

(In reply to comment #11) > Grid is not layout table. Correct, as you can see in the mochitest. And this fact is also not touched by my patch: A grid was not considered a layout table even before my patch.

Comment on attachment 382941 [details] [diff] [review] Patch concerning mochitests only >+ test_layout_tables.html \ test_attrs_layoutguess? >-function testAttrs(aAccOrElmOrID, aAttrs, aSkipUnexpectedAttrs) >+function testAttrs(aAccOrElmOrID, aAttrs, aSkipUnexpectedAttrs, >+ aAbsentAttrs) would be nice to have two shortcuts for this testAbsentAttrs() and testPresentAttrs() >+function compareAbsentAttrs(aErrorMsg, aAttrs, aExpectedAttrs) it's much similar with compareAttrs, it might be nice to extend semantic of aSkipUnexpectedAttrs argument to handle absent attributes case. >+ if ((prop.key in aExpectedAttrs)) { >+ var msg = "Attribute '" + prop.key + " 'has wrong value" + aErrorMsg; >+ var expectedValue = aExpectedAttrs[prop.key]; >+ >+ if (typeof expectedValue == "function") >+ ok(expectedValue(prop.value), msg); >+ else >+ is(prop.value, expectedValue, msg); >+ } this is not needed because you don't care about value of absent attributes. >\ No newline at end of file new line please

(In reply to comment #12) > (In reply to comment #11) > > Grid is not layout table. > > Correct, as you can see in the mochitest. And this fact is also not touched by > my patch: A grid was not considered a layout table even before my patch. Marco, I think your tests work because of bug 498277.

Comment on attachment 382941 [details] [diff] [review] Patch cancelling review

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > Grid is not layout table. > > > > Correct, as you can see in the mochitest. And this fact is also not touched by > > my patch: A grid was not considered a layout table even before my patch. > > Marco, I think your tests work because of bug 498277. You're probably right. I'll adjust the logic in my patch once yours lands. I think grids should never be layout tables, so will adjust accordingly with the new patch that will also address your other comments. (In reply to comment #13) > >+ test_layout_tables.html \ > test_attrs_layoutguess? I chose the name for layout tables explicitly because this is the only case where we actually use that particular attribute. It's all about tables that are being used for layout in non-modern HTML concepts. It's used nowhere else, and layout table is a commonly used term, so I'd prefer to keep it at that if you don't mind.

(In reply to comment #16) > (In reply to comment #13) > > >+ test_layout_tables.html \ > > test_attrs_layoutguess? > > I chose the name for layout tables explicitly because this is the only case > where we actually use that particular attribute. It's all about tables that are > being used for layout in non-modern HTML concepts. It's used nowhere else, and > layout table is a commonly used term, so I'd prefer to keep it at that if you > don't mind. Ok, I think I have not strong opinion whether the name should be attribute oriented or table oriented. Probably we need test_table_layoutguess or test_table_attr_layoutguess name (taking into account another table specific attributes like "table-cell-index") for the second way I think.

Addresses comments and adds logic to deal with new situation after landing of bug 498277.

Comment on attachment 383703 [details] [diff] [review] Patch2 >+ nsAutoString ariaRole; >+ content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, ariaRole); >+ if (mRoleMapEntry->roleRule != kUseNativeRole && >+ ariaRole != NS_LITERAL_STRING("grid")) I bet this is always false. >+ RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, no landmarks, and role is table"); > } Marco, I really would like to fix bug 469688 because it affects on logic of this method so that it should become clear where we are.

Halting work on this until we've done a reach out to AT developers on this and bug 469688.

Updated after checking in bug 502154, and hopefully better way to distinguish the three possible outcomes.

Comment on attachment 386702 [details] [diff] [review] Patch3 > PRBool hasNonTableRole = > (nsAccUtils::Role(this) != nsIAccessibleRole::ROLE_TABLE); > if (hasNonTableRole) { > RETURN_LAYOUT_ANSWER(PR_FALSE, "Has role attribute"); > } It sounds this code is for treegird only. If so it's worth to comment. > > if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role)) { >- RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, and role is table"); >+ // Make sure landmarks plus other ARIA roles using accessibles from native >+ // markup, as well as ARIA grids are never treated as layout tables. >+ nsAutoString ariaRole; >+ content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, ariaRole); >+ if (ariaRole == NS_LITERAL_STRING("grid")) { >+ RETURN_LAYOUT_ANSWER(PR_FALSE, "Is an ARIA grid"); >+ } else if (mRoleMapEntry->roleRule == kUseNativeRole) { >+ RETURN_LAYOUT_ANSWER(PR_FALSE, "Is WAI-ARIA landmark"); >+ } else { >+ RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, and role is table"); >+ } if (content->HasAttr(...)) is true if it's ARIA grid or kUseNativeRole. Third else won't be ever achieved. It sounds it's enough to change PR_FALSE on PR_TRUE in existing code with proper comments. >- // table with role of grid XXX bug 495388 >- testAttrs("table1", attr, true); >+ // table with role of grid >+ testAbsentAttrs("table1", attr); > >- // table with landmark role XXX bug 495388 >- testAttrs("table2", attr, true); >+ // table with landmark role >+ testAbsentAttrs("table2", attr); please test treegrid also.

Is this what you had in mind? And do you by chance see why both table20 and table21 give me a test failure with "DOM element not found"?

(In reply to comment #23) > Created an attachment (id=386711) [details] > Patch4 > > Is this what you had in mind? yes. Just put good comments please. > > And do you by chance see why both table20 and table21 give me a test failure > with "DOM element not found"? because of unclosed iframe tag in table19.

OK, think I got it now. :)

Comment on attachment 386717 [details] [diff] [review] Patch5 > // Check role and role attribute replace this comment on something more sensible. > PRBool hasNonTableRole = > (nsAccUtils::Role(this) != nsIAccessibleRole::ROLE_TABLE); > if (hasNonTableRole) { > RETURN_LAYOUT_ANSWER(PR_FALSE, "Has role attribute"); > } currently I think it works for ARIA treegird only, otherwise we shouldn't expose table. > if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role)) { >- RETURN_LAYOUT_ANSWER(PR_TRUE, "Has role attribute, and role is table"); >+ // Role attribute is present, but strong roles have already been dealt with. >+ // Only landmarks and other weak roles are left. >+ RETURN_LAYOUT_ANSWER(PR_FALSE, "Has role attribute, weak role, and role is table"); > } i would replace "strong" term on something more widely used or would described is somewhere like ARIA role that overrides role from native markup, for example, role="treegrid". >+ >+ // tree grid, no layout table >+ testAbsentAttrs("table20", attr); >+ >+ // table with role of menu, no layout table, strong accessible >+ testAbsentAttrs("table21", attr); sure there is no layout-guess attribute because it's not a table, IsProbablyLayout is not called for this strong accessible :) If you like to keep this test then you should document that is not a table I think. I would like to see one more patch ;)

Addresses last comments.

Comment on attachment 386718 [details] [diff] [review] Patch7 nice, r=me

Pushed: http://hg.mozilla.org/mozilla-central/rev/dd5706bd627d