Summary: | AX: fieldset should have GroupRole and legend should be description. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Samuel White <samuel_white> | ||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Samuel White <samuel_white> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cdumez, cfleizach, commit-queue, dmazzoni, gyuyoung.kim, jdiggs, mario, rakuco, rniwa, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Attachments: |
|
Description
Samuel White
2013-10-08 16:48:10 PDT
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. 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.
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> Created attachment 213934 [details]
Updated patch.
(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. Looks like a rebase is needed. Created attachment 213939 [details]
Rebased patch.
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 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
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? 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 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
Created attachment 213954 [details]
Updated patch.
(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. 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. Created attachment 213959 [details]
Updated patch.
(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. 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 Created attachment 214004 [details]
Updated patch.
(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 (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. (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. Created attachment 214180 [details]
Updated patch.
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. 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 Created attachment 214188 [details]
Updated patch.
(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. 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 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.
Comment on attachment 214197 [details]
Rebased patch.
Toggling commit flag per webkit-dev feedback.
Comment on attachment 214197 [details] Rebased patch. Rejecting attachment 214197 [details] from commit-queue. samuel_white@apple.com 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. Comment on attachment 214197 [details] Rebased patch. Clearing flags on attachment: 214197 Committed r157434: <http://trac.webkit.org/changeset/157434> All reviewed patches have been landed. Closing bug. Committed r157436: <http://trac.webkit.org/changeset/157436> |