Bug 84137

We could certainly do this. I'm debating between what role to use. http://www.w3.org/TR/html-markup/legend.html made me think ATK_ROLE_CAPTION But when I visually look at the test case, it looks an awful lot like a GtkFrame, in which case perhaps it would make sense for it to be exposed as ATK_ROLE_FRAME. Piñeiro, opinions? BTW, looking at the expected results for the Mac, it's exposed as role static text (i.e. I was hoping we could just follow their lead, but in the ATK world, I don't think ATK_ROLE_TEXT is what this thing should be).

So... Piñeiro and I chatted about this in IRC. We also looked at what we get from Gtk+ and Gecko. In summary: * One could make a good case for ATK_ROLE_CAPTION. * One could make a similarly good case for ATK_ROLE_LABEL. * Looking at a similar-looking frame from Gtk+ demo in Accerciser: - The text corresponding to the legend has ATK_ROLE_LABEL. - The container itself has ATK_ROLE_PANEL. - Both are children of the ATK_ROLE_FRAME. * Looking at the Layout test example in Firefox with Accerciser: - The legend has ATK_ROLE_LABEL. - The container itself has ATK_ROLE_PANEL. So in the spirit of consistency, the container should be ATK_ROLE_PANEL (which it already is). The legend should have ATK_ROLE_LABEL and a corresponding AtkRelation set between it and the panel.

Created attachment 161155 [details] proposed patch * Created a new WebCore Accessibility Role, LegendRole. * Used it to map to the expected platform role, ATK_ROLE_LABEL. * Established the needed AtkRelation pair, label-for/labelled-by between the legend and fieldset. * Modified legend.html to use buildAccessibilityTree() [1] * Unskipped legend.html for Gtk. * Added expected results for Gtk. * Updated expected results for Mac because the output from buildAccessibilityTree() changes the appearance. Please review. [1] It's in three tests already so why not four? ;) Note that I did open bug 95286 because this handy function should be somewhere other than individual tests. Chris gave me some advice as to how. (Thanks!) So I'll give that a shot. In the meantime, if we can overlook the duplication I'd appreciate it.

Please review. Thanks!

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 161155 [details] proposed patch I think we probably want to move this legend test to the mac platform only, because what it is testing is that the titleUIElement is properly set, not that the tree hierarch matches something. so let's move legend.html to mac only and rename to legend-title-element.html and use this new one in all platforms as something like legend-hierarchy.html

(In reply to comment #6) > (From update of attachment 161155 [details]) > I think we probably want to move this legend test to the mac platform only, because what it is testing is that the titleUIElement is properly set, not that the tree hierarch matches something. > so let's move legend.html to mac only and rename to legend-title-element.html and use this new one in all platforms as something like legend-hierarchy.html I'd be happy to do that. But what about instead putting the titleUIElement back in the test itself? I have that implemented in DRT for Gtk now.

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 161155 [details] [details]) > > I think we probably want to move this legend test to the mac platform only, because what it is testing is that the titleUIElement is properly set, not that the tree hierarch matches something. > > so let's move legend.html to mac only and rename to legend-title-element.html and use this new one in all platforms as something like legend-hierarchy.html > > I'd be happy to do that. But what about instead putting the titleUIElement back in the test itself? I have that implemented in DRT for Gtk now. that will work as well

Created attachment 161173 [details] proposed patch (In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 161155 [details] [details] [details]) > > > I think we probably want to move this legend test to the mac platform only, because what it is testing is that the titleUIElement is properly set, not that the tree hierarchy matches something. Now the test prints the hierarchy and identifies the titleUIElement within the hierarchy. So mac results look like: AXRole: AXWebArea AXValue: AXRole: AXGroup AXValue: AXRole: AXGroup AXValue: << fieldset's titleUIElement AXRole: AXStaticText AXValue: Choose a shipping method: AXRole: AXGroup AXValue: AXRole: AXRadioButton AXValue: 0 AXRole: AXStaticText AXValue: Overnight AXRole: AXGroup AXValue: AXRole: AXStaticText AXValue: End of test And gtk results look like: AXRole: document frame AXRole: panel AXRole: label AXValue: Choose a shipping method: << fieldset's titleUIElement AXRole: panel AXRole: radio button AXValue: AXRole: section AXValue: End of test Two completely different hierarchies, same titleUIElement, one single cross-platform layout test. :) Whatdya think? If you don't like it, I'll go with your original suggestion namely: > > > so let's move legend.html to mac only and rename to legend-title-element.html and use this new one in all platforms as something like legend-hierarchy.html Thanks again!

Created attachment 161212 [details] proposed patch The previous patch is no longer master compatible due to the new CanvasRole. This one should be.

Comment on attachment 161212 [details] proposed patch Funny story: * This patch obsoletes the previous patch. * The previous patch became obsolete because it would not apply cleanly given this morning's committed fix for bug 95248. (http://trac.webkit.org/changeset/126972) * I happily created a new post-bug 95248 version tossed at at EWS, everything is green BUT cr-linux because the fix for bug 95248 was rolled out in bug 95349 hours later. (http://trac.webkit.org/changeset/127006) Yes I know these things happen. No big deal. :) But I would appreciate a review of this or the previous patch even though it does not currently pass EWS for the reasons described above. Unless Dominic makes some extreme change to fix bug 95349, one of these patches is bound to apply cleanly. ;) Sorry and thanks!

Comment on attachment 161212 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161212&action=review > LayoutTests/accessibility/legend.html:52 > + buildAccessibilityTree(body, 0, fieldset.titleUIElement(), "<< fieldset's titleUIElement"); While this method works, I find it a little hard to understand. I think in addition to what you have here, doing an equality for the titleUIElement would be good. something like shouldBeTrue(titleUIElement.isEqual(fieldset.titleUIElement())) after you build the accessibility tree code. > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:205 > + extra line insertion can be removed

(In reply to comment #12) > (From update of attachment 161212 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161212&action=review > > > LayoutTests/accessibility/legend.html:52 > > + buildAccessibilityTree(body, 0, fieldset.titleUIElement(), "<< fieldset's titleUIElement"); > > While this method works, I find it a little hard to understand. Method as in "code method/function"? Or method as in "method of indicating the desired object"? (Yeah, I know. Both. Because it's how I feel.) Having said that, in this particular case I'm actually more concerned about "method of indicating the desired object". Because my plan is to tackle bug 95286 before too long, so the unclear method of code in this particular test will go away in favor of replacement code in the to-be-created accessibility-utlity.js). But I'd like to sort out what that new code should do. So about indicating the desired object... You have ideas and/or preferences? * Marking it on its left with an asterisk (or your character(s) of choice) * Enclosing that whole line in square brackets (or the characters of choice) * Something else > I think in addition to what you have here, doing an equality for the titleUIElement would be good. > > something like > > shouldBeTrue(titleUIElement.isEqual(fieldset.titleUIElement())) Nice! > after you build the accessibility tree code. > > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:205 > > + > > extra line insertion can be removed Will do. Thanks for the review and suggestions.

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 161212 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161212&action=review > > > > > LayoutTests/accessibility/legend.html:52 > > > + buildAccessibilityTree(body, 0, fieldset.titleUIElement(), "<< fieldset's titleUIElement"); > > > > While this method works, I find it a little hard to understand. > > Method as in "code method/function"? Or method as in "method of indicating the desired object"? (Yeah, I know. Both. Because it's how I feel.) > > Having said that, in this particular case I'm actually more concerned about "method of indicating the desired object". Because my plan is to tackle bug 95286 before too long, so the unclear method of code in this particular test will go away in favor of replacement code in the to-be-created accessibility-utlity.js). But I'd like to sort out what that new code should do. > > So about indicating the desired object... You have ideas and/or preferences? > * Marking it on its left with an asterisk (or your character(s) of choice) > * Enclosing that whole line in square brackets (or the characters of choice) > * Something else > I think the mechanism is fine, I just wanted to say that for a test that checks what the titleUIElement was, this method of encoding it within a buildAXTree method is not the clearest way to indicate that. > > I think in addition to what you have here, doing an equality for the titleUIElement would be good. > > > > something like > > > > shouldBeTrue(titleUIElement.isEqual(fieldset.titleUIElement())) > > Nice! > > > after you build the accessibility tree code. > > > > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:205 > > > + > > > > extra line insertion can be removed > > Will do. Thanks for the review and suggestions.

Created attachment 161626 [details] proposed fix (addressed feedback from review) (In reply to comment #12) > (From update of attachment 161212 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161212&action=review > > > LayoutTests/accessibility/legend.html:52 > > + buildAccessibilityTree(body, 0, fieldset.titleUIElement(), "<< fieldset's titleUIElement"); > > While this method works, I find it a little hard to understand. I think in addition to what you have here, doing an equality for the titleUIElement would be good. > > something like > > shouldBeTrue(titleUIElement.isEqual(fieldset.titleUIElement())) > > after you build the accessibility tree code. Wound up doing three things in response: 1. Null check: shouldBeTrue("titleUIElement != null"); 2. Platform independent (in theory) search for the text: var titleUIElementText = titleUIElement; while (titleUIElementText && titleUIElementText.childrenCount) titleUIElementText = titleUIElementText.childAtIndex(0); shouldBe("titleUIElementText.stringValue", "'AXValue: Choose a shipping method:'"); ("In theory" == looking at the html and how we do things, the ancestry will vary from platform to platform, but I can't think of a good reason why a platform would give the StaticText object a child. Moreover, I confirmed it works for the Gtk port as well as the Mac port.) 3. Added comments as to why the test was doing the things it is doing. In addition, I updated the Gtk test results to reflect what will be the case after the fix for bug 95180 is committed since you gave that one an R+ and I've since Cq?ed it. > > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:205 > > + > > extra line insertion can be removed Argh. I hate when I do that. Thanks for catching it. Removed.

Comment on attachment 161626 [details] proposed fix (addressed feedback from review) thanks for the updates. looks good

Comment on attachment 161626 [details] proposed fix (addressed feedback from review) Rejecting attachment 161626 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rc/AssertMatchingEnums.cpp patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/DumpRenderTree/chromium/TestRunner/AccessibilityUIElementChromium.cpp Hunk #1 FAILED at 208. 1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/chromium/TestRunner/AccessibilityUIElementChromium.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chris Flei..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13788758

Created attachment 163254 [details] Patch

Sorry to be spammy, but a just-in-case note for gardeners: This patch includes updated tests and may need rebaselining.

Comment on attachment 163254 [details] Patch Clearing flags on attachment: 163254 Committed r128140: <http://trac.webkit.org/changeset/128140>

All reviewed patches have been landed. Closing bug.