Summary: | [ATK] Changing the mapping of aria rowheader and columnheader into respective ATK roles. | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrzej Badowski <a.badowski> | ||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, rniwa, samuel_white, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Andrzej Badowski
2014-08-22 01:30:49 PDT
Attachment 236974 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:682: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:684: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 4 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 236975 [details]
proposed patch 2
Created attachment 236977 [details]
proposed patch 3
Comment on attachment 236977 [details] proposed patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=236977&action=review > Source/WebCore/ChangeLog:10 > + No new tests because existing tests, especially lauout accessibility tests associated with the roles aria cover it. lauout spelled incorrect aria => ARIA Created attachment 237066 [details]
proposed patch 4
according to the commentary of Chris Fleizach
Mario, any more comments? Comment on attachment 237066 [details] proposed patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=237066&action=review > Source/WebCore/ChangeLog:10 > + No new tests because existing tests, especially accessibility layout tests associated with the ARIA roles, cover it. Could you mention existing tests are those ones covering this change? Asking that because it's not clear to me that this change is being tested at all (even though if the mapping seems correct) Created attachment 237453 [details]
proposed patch 5
Mario, I introduced an amendment to the test to be able to verify the change. Moreover I completed the code. Please comment this proposal of mapping. Comment on attachment 237453 [details] proposed patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=237453&action=review > LayoutTests/ChangeLog:8 > + Minor changes in the test to check the mapping of rowheader and columnheader. Still, I miss tests using the <th> element. Perhaps it's better to leave those for another patch, but I would be insteresting to try to add them now as well, if it's not much of a trouble > LayoutTests/accessibility/roles-exposed.html:210 > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="rowheader" class="ex">X</div> > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="columnheader" class="ex">X</div> You should remove the prefixed comment and probably comment on bug 125493 together with this patch > LayoutTests/accessibility/roles-exposed.html:274 > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div role="rowheader" data-platform="atk,mac" class="ex">X</div> > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div role="columnheader" data-platform="atk,mac" class="ex">X</div> Same here > Source/WebCore/ChangeLog:7 > + You need to write a description of the patch here (between this 'Reviewed by' line and the following one mentioning the tests). > Source/WebCore/ChangeLog:12 > + (atkRoleCell): > + (atkRole): A brief sentence describing each change here wouldn't hurt either :) > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:597 > + const AtomicString& roleCell = coreObject->getAttribute(HTMLNames::roleAttr); > + if (equalIgnoringCase(roleCell.string(), "rowheader")) > + return ATK_ROLE_ROW_HEADER; This seems wrong. The value of coreObject->roleValue() or coreObject->ariaRoleAttribute() should be RowHeaderRole and that's all you should need to make this decision here. Mannually checking the value of the role attribute like this is not a good idea (and in any case it should be done in another way, such as in AXObject::nodeHasRole()) Maybe there's something wrong in determineAccessibilityRole() or determineAriaRoleAttribute()? > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:696 > - return coreObject->inheritsPresentationalRole() ? ATK_ROLE_SECTION : ATK_ROLE_TABLE_CELL; > + return atkRoleCell(coreObject); If you fix the root reason why you could not get RowHeaderRole or ColumnHeaderRole to work (see comment above), you will probably not need this new function here either. Created attachment 237694 [details]
proposed patch 6
This is a proposal for discussion.
I am currently working on several tests that fail following these changes in the code.
Comment on attachment 237694 [details] proposed patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=237694&action=review > LayoutTests/accessibility/roles-exposed.html:210 > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="rowheader" class="ex">X</div> > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="columnheader" class="ex">X</div> Please remove the prefixed comments (and post a comment in bug 125493 too, if possible) > LayoutTests/accessibility/roles-exposed.html:274 > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div role="rowheader" data-platform="atk,mac" class="ex">X</div> > + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div role="columnheader" data-platform="atk,mac" class="ex">X</div> Ditto > Source/WebCore/ChangeLog:10 > + No new tests because the existing test with minor amendments and other tests cover it. > + > + Proposals for changes in the code that allow using listed ARIA roles. I know it's a proposal for discussion, but just in case, we normally put first the line describing the patch and last the one mentioning the tests (new or not) covering the changes. Would be nice to consider that for future patches > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2564 > + for ( ; node; node = node->parentNode()) { I think a 'while (node)' statement would be better in this case > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:684 > + case RowHeaderRole: > + return ATK_ROLE_ROW_HEADER; > + case ColumnHeaderRole: > + return ATK_ROLE_COLUMN_HEADER; I wonder if this change will break other a11y tests or functionality if that some AccessibilityObjects that used to report a CellRole role will now report a ColumnHeaderRole or RowHeaderRole. I have the feeling that some more changes would be needed such as, for instance, to update the roleIsTextType() static function in the ATK wrapper. Also, it would be great if you could share here a plain text representation of the ATK hierarchy that would be exposed for some common cases of HTML tables with column/row headers, so we are sure that whatever we will be exposing is what ATs (e.g. Orca) do expect. Comment on attachment 237694 [details] proposed patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=237694&action=review Thank you for your review. >> LayoutTests/accessibility/roles-exposed.html:210 >> + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div data-platform="atk,mac" role="columnheader" class="ex">X</div> > > Please remove the prefixed comments (and post a comment in bug 125493 too, if possible) OK >> LayoutTests/accessibility/roles-exposed.html:274 >> + <!-- [ATK] Wrong role (webkit.org/b/125493) --><div role="columnheader" data-platform="atk,mac" class="ex">X</div> > > Ditto OK >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2564 >> + for ( ; node; node = node->parentNode()) { > > I think a 'while (node)' statement would be better in this case I agree. Created attachment 239110 [details]
ptoposed patch 7
Comment on attachment 239110 [details] ptoposed patch 7 Attachment 239110 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4576392100446208 New failing tests: accessibility/aria-tables.html Created attachment 239112 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 239110 [details] ptoposed patch 7 Attachment 239110 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4691469575127040 New failing tests: accessibility/aria-tables.html Created attachment 239113 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239114 [details]
proposed patch 8
Comment on attachment 239114 [details] proposed patch 8 View in context: https://bugs.webkit.org/attachment.cgi?id=239114&action=review > LayoutTests/accessibility/table-roles-hierarchy.html:13 > + { brace goes on previous line > LayoutTests/accessibility/table-roles-hierarchy.html:19 > + { brace goes on previous line > LayoutTests/accessibility/table-roles-hierarchy.html:25 > + { brace goes on previous line > LayoutTests/accessibility/table-roles-hierarchy.html:36 > + if ( childAXObject.role != "AXRole: AXStaticText") extra space after ( > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2573 > + while (node) { this can be written as a for loop for (node = node->parentNode(); node; node = node->parentNode()) > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2578 > + if (node->hasTagName(tableTag)) are you sure you want to make a <Table> into a CellRole? Comment on attachment 239114 [details] proposed patch 8 View in context: https://bugs.webkit.org/attachment.cgi?id=239114&action=review >> LayoutTests/accessibility/table-roles-hierarchy.html:13 >> + { > > brace goes on previous line OK >> LayoutTests/accessibility/table-roles-hierarchy.html:19 >> + { > > brace goes on previous line OK >> LayoutTests/accessibility/table-roles-hierarchy.html:25 >> + { > > brace goes on previous line OK >> LayoutTests/accessibility/table-roles-hierarchy.html:36 >> + if ( childAXObject.role != "AXRole: AXStaticText") > > extra space after ( OK >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2573 >> + while (node) { > > this can be written as a for loop > for (node = node->parentNode(); node; node = node->parentNode()) The for loop is very good... >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2578 >> + if (node->hasTagName(tableTag)) > > are you sure you want to make a <Table> into a CellRole? Please note that tableTag at this point means that th element has no parent or the parent's parent and so on, which would be theadTag, tbodyTag or tfootTag item. In this situation, the decision for the original thTag is CellRole. Use tableTag lets stop checking on tableTag and not for example on body tag or html tag. Created attachment 239337 [details]
proposed patch 9
According to the comments by Chris.
If the current patch for bug 136818 (proposed patch 4) would be positively reviewed, this bug will depend on bug 136818. Accurate determination of role in the function AccessibilityTableCell::determineAccessibilityRole() will be based on the new functions: isRowHeaderCell and isColumnHeaderCell. Comment on attachment 239337 [details] proposed patch 9 View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2568 > + for (node = node->parentNode(); node; node = node->parentNode()) { I think you should use a different variable for node, since you're using it it everywhere else to reference the current node, but then you're in this for loop, changing it to parent nodes. I see that you don't leave this block, so there's probably not a logic problem, but for readability it might be good to use for (Node* parentNode = node->parentNode(); parentNode; parentNode = parentNode->parentNode()) Comment on attachment 239337 [details] proposed patch 9 View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567 > + if (node && (node->hasTagName(thTag))) { Instead of hasTagName(thTag) we should consider: if (is<HTMLTableHeaderElement>(node)) >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2568 >> + for (node = node->parentNode(); node; node = node->parentNode()) { > > I think you should use a different variable for node, since you're using it it everywhere else to reference the current node, but > then you're in this for loop, changing it to parent nodes. > I see that you don't leave this block, so there's probably not a logic problem, but for readability it might be good to use > > for (Node* parentNode = node->parentNode(); parentNode; parentNode = parentNode->parentNode()) This is much better: for (auto& ancestor : ancestorsOfType<HTMLElement>(downcast<HTMLTableHeaderElement>(*node)) All the hasTagName checks below will be more efficient if we already know it’s an HTML element. Comment on attachment 239337 [details] proposed patch 9 View in context: https://bugs.webkit.org/attachment.cgi?id=239337&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567 >> + if (node && (node->hasTagName(thTag))) { > > Instead of hasTagName(thTag) we should consider: > > if (is<HTMLTableHeaderElement>(node)) I don't think we have such class (yet). Sadly, both tdTag and thTag map to the same HTMLTableCellElement class. So currently, if you're only interested in thTag but not tdTag, using hasTagName() is correct. This is something we should likely improve. Created attachment 239525 [details]
proposed patch 10
according to a recent commentary by Chris
Comment on attachment 239525 [details] proposed patch 10 Clearing flags on attachment: 239525 Committed r174567: <http://trac.webkit.org/changeset/174567> All reviewed patches have been landed. Closing bug. |