Bug 84137 – [GTK] Properly expose <legend> elements to ATs

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mario Sanchez Prada]

We should properly expose at least the role and text for elements like this:

       <legend>Choose a shipping method:</legend>

This should allow unskipping accessibility/legend.html

[Joanmarie Diggs]

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).

[Joanmarie Diggs]

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.

[Joanmarie Diggs]

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.

[Joanmarie Diggs]

Please review. Thanks!

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[chris fleizach]

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

[Joanmarie Diggs]

(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.

[chris fleizach]

(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

[Joanmarie Diggs]

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!

[Joanmarie Diggs]

Created attachment 161212 [details]
proposed patch

The previous patch is no longer master compatible due to the new CanvasRole. This one should be.

[Joanmarie Diggs]

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!

[chris fleizach]

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

[Joanmarie Diggs]

(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.

[chris fleizach]

(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.

[Joanmarie Diggs]

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.

[chris fleizach]

Comment on attachment 161626 [details]
proposed fix (addressed feedback from review)

thanks for the updates. looks good

[WebKit Review Bot]

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

[Joanmarie Diggs]

Created attachment 163254 [details]
Patch

[Joanmarie Diggs]

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

[WebKit Review Bot]

Comment on attachment 163254 [details]
Patch

Clearing flags on attachment: 163254

Committed r128140: <http://trac.webkit.org/changeset/128140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.