Bug 122534 – AX: fieldset should have GroupRole and legend should be description.

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

[Samuel White]

feildset elements should be groups and their description should come from their legend if one is available.

[Radar WebKit Bug Importer]

<rdar://problem/15181830>

[Samuel White]

The fieldset element is meant to group like form elements and the associated legend is meant to label those elements. The current implementation doesn't make this concept clear as the legend is exposed separately and simply related back to the fieldset group using the titleUIElement mechanism.

Instead, our implementation should mirror how tables and their associated caption elements are handled. That is, tables are a group and they derive their label information from their caption as this is a caption elements intended purpose. The caption is not exposed on its own as this would make the relation less clear to users of AT.

[Samuel White]

Created attachment 213831 [details]
Patch for initial feedback, not review.

The intent is to keep the legend element from being exposed on its own, and to only use it for labeling purposes when dealing with a fieldset element.

My goal was to accomplish this without subclassing to avoid tinkering with the existing children and text generation machinery.

Be aware that this patch is only intended to get some initial feedback so please ignore TODOs and noise. Thanks in advance for any feedback.

[chris fleizach]

Comment on attachment 213831 [details]
Patch for initial feedback, not review.

View in context: https://bugs.webkit.org/attachment.cgi?id=213831&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:396
> +    case LegendRole:

do we need to make this change? what if someone just threw a <legend> in the middle of a webpage. we probably still want to expose the text

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1257
> +        AccessibilityObject* legend = axObjectCache()->getOrCreate(const_cast<HTMLLegendElement*>(toHTMLFieldSetElement(node)->legend()));

unncesary cast
const_cast<HTMLLegendElement*>

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1259
> +            textOrder.append(AccessibilityText(legend->textUnderElement(), AlternativeText));

what if someone put an aria-label on the legend?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1106
> +    if (m_renderer->isLegend())

we might only want to do this if it's in  a<fieldset>

[Samuel White]

Created attachment 213934 [details]
Updated patch.

[Samuel White]

(In reply to comment #4)
> (From update of attachment 213831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213831&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:396
> > +    case LegendRole:
> 
> do we need to make this change? what if someone just threw a <legend> in the middle of a webpage. we probably still want to expose the text

legend isn't valid outside fieldset so I hadn't considered that case (html 5 spec.), good catch, I added support and testing for this case.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1257
> > +        AccessibilityObject* legend = axObjectCache()->getOrCreate(const_cast<HTMLLegendElement*>(toHTMLFieldSetElement(node)->legend()));
> 
> unncesary cast
> const_cast<HTMLLegendElement*>

The legend method returns a const pointer so I do need this cast.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1259
> > +            textOrder.append(AccessibilityText(legend->textUnderElement(), AlternativeText));
> 
> what if someone put an aria-label on the legend?

Added support for this.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1106
> > +    if (m_renderer->isLegend())
> 
> we might only want to do this if it's in  a<fieldset>

Agreed.

[Samuel White]

Looks like a rebase is needed.

[Samuel White]

Created attachment 213939 [details]
Rebased patch.

[Build Bot]

Comment on attachment 213939 [details]
Rebased patch.

Attachment 213939 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3903032

New failing tests:
platform/mac/accessibility/role-subrole-roledescription.html

[Build Bot]

Created attachment 213944 [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

[chris fleizach]

Comment on attachment 213939 [details]
Rebased patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=213939&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1235
> +    if (!node->isHTMLElement())

can you leave this method where it was in the patch, so the diffset is smaller and easier to evaluate?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1300
> +        if (legend)

you can combine 1299 and 1300 into one line

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1107
> +        Node* node = this->node();

let's add a element() method to AccessibilityObject so that we don't have to do this isElementNode()/toElement() combo over and over again

> LayoutTests/accessibility/fieldset-element.html:29
> +</fieldset>

what happens if you have a field set with an aria-label and a <legend>

> LayoutTests/accessibility/fieldset-element.html:32
> +<!-- legend without fieldset. -->

do you have a test for a hidden legend and an off-screen legend?

[Build Bot]

Comment on attachment 213939 [details]
Rebased patch.

Attachment 213939 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3900035

New failing tests:
platform/mac/accessibility/role-subrole-roledescription.html

[Build Bot]

Created attachment 213945 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Samuel White]

Created attachment 213954 [details]
Updated patch.

[Samuel White]

(In reply to comment #11)
> (From update of attachment 213939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213939&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1235
> > +    if (!node->isHTMLElement())
> 
> can you leave this method where it was in the patch, so the diffset is smaller and easier to evaluate?

Sure. Needed to add a prototype to keep it in place.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1300
> > +        if (legend)
> 
> you can combine 1299 and 1300 into one line

Not sure what you mean here. WebKit style guidelines forbid putting things on same line as if. Also, I'd rather not get legend twice if that's what you mean as it isn't clear that the cost is trivial.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1107
> > +        Node* node = this->node();
> 
> let's add a element() method to AccessibilityObject so that we don't have to do this isElementNode()/toElement() combo over and over again

Good idea. Done.

> 
> > LayoutTests/accessibility/fieldset-element.html:29
> > +</fieldset>
> 
> what happens if you have a field set with an aria-label and a <legend>

Added case to make sure this works.

> 
> > LayoutTests/accessibility/fieldset-element.html:32
> > +<!-- legend without fieldset. -->
> 
> do you have a test for a hidden legend and an off-screen legend?

Added.

Thanks for the feedback.

[Ryosuke Niwa]

Comment on attachment 213954 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=213954&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1269
> +        HTMLLegendElement* legend = const_cast<HTMLLegendElement*>(toHTMLFieldSetElement(node)->legend());

We should just add non-const version of legend() on HTMLFieldSetElement.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1270
> +        if (legend)

You can do:
if (HTMLLegendElement* legend = ~)
to limit the scope of the variable.

[Samuel White]

Created attachment 213959 [details]
Updated patch.

[Samuel White]

(In reply to comment #16)
> (From update of attachment 213954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213954&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1269
> > +        HTMLLegendElement* legend = const_cast<HTMLLegendElement*>(toHTMLFieldSetElement(node)->legend());
> 
> We should just add non-const version of legend() on HTMLFieldSetElement.

I went ahead and made THAT version non-const so it was more inline with other classes. For example, the caption getter on HTMLTableElement:

HTMLTableCaptionElement* caption() const;

I found no existing call sites that would indicate that this change was not safe. Thoughts?

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1270
> > +        if (legend)
> 
> You can do:
> if (HTMLLegendElement* legend = ~)
> to limit the scope of the variable.

Sure. I misunderstood the original feedback from Chris. Thanks for the clarification.

Thanks for the feedback.

[chris fleizach]

Comment on attachment 213959 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=213959&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:272
> +Element* AccessibilityNodeObject::element() const

this whole method can be in AXObject.cpp and then it won't have to be virtual

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:408
> +        if (element())

you can do 
   if (Element* element = this->element())
           return ....(element)

so that you don't have to call element twice

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1108
> +            return IgnoreObject;

We should find out if GTK can support this paradigm where the label in on a group, instead of exposing the actual label.

Mario, do you know the answer?

> LayoutTests/accessibility/fieldset-element-expected.txt:19
> +PASS fieldsetDescription is 'AXDescription: High Score:'

i think this is the case where aria-hidden=true on the <legend>. i would expect the description to be empty in this case

> LayoutTests/accessibility/fieldset-element.html:43
> +<legend aria-hidden="true" style="left:-9999px; position:absolute;">High Score:</legend>

if you look at the test you're deleting hidden-legend.html, that tested two scenarios

display:none
left:-9999px

I don't see the display:none scenario here (which should make it so that the <fieldset> does not have a label --- unless aria-hidden=false

[Samuel White]

Created attachment 214004 [details]
Updated patch.

[Samuel White]

(In reply to comment #19)
> (From update of attachment 213959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213959&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:272
> > +Element* AccessibilityNodeObject::element() const
> 
> this whole method can be in AXObject.cpp and then it won't have to be virtual

Moved.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:408
> > +        if (element())
> 
> you can do 
>    if (Element* element = this->element())
>            return ....(element)
> 
> so that you don't have to call element twice

Good catch.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1108
> > +            return IgnoreObject;
> 
> We should find out if GTK can support this paradigm where the label in on a group, instead of exposing the actual label.
> 
> Mario, do you know the answer?
> 
> > LayoutTests/accessibility/fieldset-element-expected.txt:19
> > +PASS fieldsetDescription is 'AXDescription: High Score:'
> 
> i think this is the case where aria-hidden=true on the <legend>. i would expect the description to be empty in this case

You're right, thanks (step 1):
http://www.w3.org/TR/wai-aria/roles#namecalculation

> 
> > LayoutTests/accessibility/fieldset-element.html:43
> > +<legend aria-hidden="true" style="left:-9999px; position:absolute;">High Score:</legend>
> 
> if you look at the test you're deleting hidden-legend.html, that tested two scenarios
> 
> display:none
> left:-9999px
> 
> I don't see the display:none scenario here (which should make it so that the <fieldset> does not have a label --- unless aria-hidden=false

Step 1 of the above text alt computation states that only "hidden" things are skipped and hidden from accessibility is stated as only being aria-hidden==true:
http://www.w3.org/TR/wai-aria/terms#def_hidden

[Mario Sanchez Prada]

(In reply to comment #19)
> [...]
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1108
> > +            return IgnoreObject;
> 
> We should find out if GTK can support this paradigm where the label in on a 
> group, instead of exposing the actual label.
> 
> Mario, do you know the answer?
> 
I think that ATK/ATSPI based ATs (e.g. Orca) will still expect the legend to be exposed if it's there and it's not hidden, as they will still both implement the AtkText interface and allow navigating through them with the caret in caret browsing mode.

In that regard, would it be possible to add that check in Mac's specific implementation of AccessibilityObject::accessibilityPlatformIncludesObject()? That way, we would still have the option to keep exposing the legend for GTK. We would also need to have platform specific expectations in that case, but I guess that should not be a big deal.

In any case, I think Joanie's opinion would be very valuable here, as she's the Orca maintainer and knows better what to expect and what not. I will ping her on IRC to see if we can get her expert opinion here too.

[Samuel White]

(In reply to comment #22)
> (In reply to comment #19)
> > [...]
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1108
> > > +            return IgnoreObject;
> > 
> > We should find out if GTK can support this paradigm where the label in on a 
> > group, instead of exposing the actual label.
> > 
> > Mario, do you know the answer?
> > 
> I think that ATK/ATSPI based ATs (e.g. Orca) will still expect the legend to be exposed if it's there and it's not hidden, as they will still both implement the AtkText interface and allow navigating through them with the caret in caret browsing mode.
> 
> In that regard, would it be possible to add that check in Mac's specific implementation of AccessibilityObject::accessibilityPlatformIncludesObject()? That way, we would still have the option to keep exposing the legend for GTK. We would also need to have platform specific expectations in that case, but I guess that should not be a big deal.

Thanks for the input Mario. I will make this change platform specific as requested.

> 
> In any case, I think Joanie's opinion would be very valuable here, as she's the Orca maintainer and knows better what to expect and what not. I will ping her on IRC to see if we can get her expert opinion here too.

[Samuel White]

Created attachment 214180 [details]
Updated patch.

[Samuel White]

Changes in this patch:

1. Made legend hiding platform specific.
2. Made testing platform specific (debug output rather than platform 'failure' messages).
3. More robust "is visible" logic per comments by Chris.

Mario, can you think of any other functionality changes or tests we should have to support ATK/Orca? Thanks for your help with this.

[chris fleizach]

Comment on attachment 214180 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=214180&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1262
> +        if (object && !object->isARIAHidden() && !object->isDOMHidden())

should we have a single method that is

isHidden() {
   return obj->isARIAHidden() || object->isDOMHIdden();
}

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1104
> +    

extraneous white space addition

> LayoutTests/accessibility/self-referencing-aria-labelledby.html:40
> +        var testCount = document.getElementsByClassName("test").length;

seems like this should be in a different patch

[Samuel White]

Created attachment 214188 [details]
Updated patch.

[Samuel White]

(In reply to comment #26)
> (From update of attachment 214180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214180&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1262
> > +        if (object && !object->isARIAHidden() && !object->isDOMHidden())
> 
> should we have a single method that is
> 
> isHidden() {
>    return obj->isARIAHidden() || object->isDOMHIdden();
> }

Added.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1104
> > +    
> 
> extraneous white space addition

Removed.

> 
> > LayoutTests/accessibility/self-referencing-aria-labelledby.html:40
> > +        var testCount = document.getElementsByClassName("test").length;
> 
> seems like this should be in a different patch

Removed.

Thanks Chris.

[WebKit Commit Bot]

Comment on attachment 214188 [details]
Updated patch.

Rejecting attachment 214188 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 214188, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/git.webkit.org/WebKit
   8b2f263..68083b5  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 157423 = 8b2f263c03d7a0ea8b8ac23380bc631fab14f5ec
r157424 = 68083b5599c1cfca0f6963f8256eeaadc01fc70e
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/3867995

[Samuel White]

Created attachment 214197 [details]
Rebased patch.

Rebasing to make sure the issue isn't on my end (doesn't look like it). Will ping list about commit bot issues that happened over the weekend, might be related.

[Samuel White]

Comment on attachment 214197 [details]
Rebased patch.

Toggling commit flag per webkit-dev feedback.

[WebKit Commit Bot]

Comment on attachment 214197 [details]
Rebased patch.

Rejecting attachment 214197 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 214197 [details]
Rebased patch.

Clearing flags on attachment: 214197

Committed r157434: <http://trac.webkit.org/changeset/157434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Committed r157436: <http://trac.webkit.org/changeset/157436>