Bug 96325 – VO issues with hidden <legend> and last explicitly labelled element within a group <fieldset>

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

<legend> hidden off screen by CSS is not announced by VoiceOver when moving focus to group options within the <fieldset>.

Would expect display:none to hide from screen readers, but not when positioned off screen.

[chris fleizach]

Created attachment 163234 [details]
patch

[WebKit Review Bot]

Comment on attachment 163234 [details]
patch

Attachment 163234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13810478

New failing tests:
accessibility/hidden-legend.html

[chris fleizach]

(In reply to comment #2)
> (From update of attachment 163234 [details])
> Attachment 163234 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13810478
> 
> New failing tests:
> accessibility/hidden-legend.html

Is there a way to get what the failure was?

[Dominic Mazzoni]

I'm not sure if there's a way to download the test results from the ews bots, but here's the error:

Pick your favourite colour:
 red  yellow
Pick your favourite colour: red  yellow
 red  yellow
This tests that the legend is still used as the title UI element even when off-screen (but not when display:none is used)

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


FAIL fieldset1.titleUIElement().isEqual(fieldset1.childAtIndex(0)) should be true. Threw exception TypeError: Cannot call method 'isEqual' of null
FAIL fieldset2.titleUIElement().isEqual(fieldset2.childAtIndex(0)) should be true. Threw exception TypeError: Cannot call method 'isEqual' of null
FAIL fieldset3.titleUIElement().isValid should be false. Threw exception TypeError: Cannot read property 'isValid' of null
PASS successfullyParsed is true

TEST COMPLETE


Also, I did a quick dump of the accessibility tree - role, description, title, value - in case that helps.

Is it a difference of what's ignored? We really need a method to identify elements in the tree even if the hierarchy is slightly different.

CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:             AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: Pick your favourite colour:
CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:             AXRole: AXRadioButton AXDescription:  AXTitle: red AXValue: 
CONSOLE MESSAGE: line 53:             AXRole: AXRadioButton AXDescription:  AXTitle: yellow AXValue: 
CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:             AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: Pick your favourite colour:
CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:     AXRole: AXParagraph AXDescription:  AXTitle:  AXValue: 
CONSOLE MESSAGE: line 53:         AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: This test makes sure that a generic focusable div can get accessibility focus.

[chris fleizach]

(In reply to comment #4)
> I'm not sure if there's a way to download the test results from the ews bots, but here's the error:
> 
> Pick your favourite colour:
>  red  yellow
> Pick your favourite colour: red  yellow
>  red  yellow
> This tests that the legend is still used as the title UI element even when off-screen (but not when display:none is used)
> 
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
> FAIL fieldset1.titleUIElement().isEqual(fieldset1.childAtIndex(0)) should be true. Threw exception TypeError: Cannot call method 'isEqual' of null
> FAIL fieldset2.titleUIElement().isEqual(fieldset2.childAtIndex(0)) should be true. Threw exception TypeError: Cannot call method 'isEqual' of null
> FAIL fieldset3.titleUIElement().isValid should be false. Threw exception TypeError: Cannot read property 'isValid' of null
> PASS successfullyParsed is true
> 
> TEST COMPLETE
> 
> 
> Also, I did a quick dump of the accessibility tree - role, description, title, value - in case that helps.
> 
> Is it a difference of what's ignored? We really need a method to identify elements in the tree even if the hierarchy is slightly different.
> 
> CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:             AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: Pick your favourite colour:
> CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:             AXRole: AXRadioButton AXDescription:  AXTitle: red AXValue: 
> CONSOLE MESSAGE: line 53:             AXRole: AXRadioButton AXDescription:  AXTitle: yellow AXValue: 
> CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:             AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: Pick your favourite colour:
> CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:     AXRole: AXGroup AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXRadioButton AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:     AXRole: AXParagraph AXDescription:  AXTitle:  AXValue: 
> CONSOLE MESSAGE: line 53:         AXRole: AXStaticText AXDescription:  AXTitle:  AXValue: This test makes sure that a generic focusable div can get accessibility focus.

I think the problem is that chromium doesn't implement titleUIElement()  (maybe?)

[Dominic Mazzoni]

(In reply to comment #5)
> I think the problem is that chromium doesn't implement titleUIElement()  (maybe?)

You're right. Actually Chromium implements it, just DumpRenderTree's chromium port doesn't support it from JavaScript yet.

Just skip this test on Chromium for now. I've been meaning to go through and implement a bunch more methods in DumpRenderTree and see if I can enable a lot more tests.

[chris fleizach]

Created attachment 163644 [details]
patch

[Darin Adler]

Comment on attachment 163644 [details]
patch 

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

> Source/WebCore/ChangeLog:15
> +        (WebCore):

Please remove bogus lines like this one.

> Source/WebCore/ChangeLog:19
> +        (AccessibilityRenderObject):

Please remove bogus lines like this one.

> Source/WebCore/accessibility/AccessibilityRenderObject.h:249
> +    virtual RenderObject* findLegend(RenderObject*) const;

Why virtual?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1054
> +    // This is very similar to RenderFieldset::findLegend, except that it allows using legends
> +    // that are positioned off-screen for accessibility.
> +    for (RenderObject* legend = fieldset->firstChild(); legend; legend = legend->nextSibling()) {
> +        if (legend->style()->visibility() == VISIBLE && legend->node() && legend->node()->hasTagName(legendTag))
> +            return legend;
> +    }
> +    return 0;

We may want this new function to be a RenderFieldset member even though its use is accessibility-specific. I’m concerned that this code will get out of sync of RenderFieldset::findLegend if the logic has reason to change there.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1065
>      if (isFieldset())
> -        return axObjectCache()->getOrCreate(toRenderFieldset(m_renderer)->findLegend());
> +        if (RenderObject* legend = findLegend(m_renderer))
> +            return axObjectCache()->getOrCreate(legend);

Need braces since the body of the isFieldset() if is now more than one line.

The old code did not check for 0, but the new code does. Is that OK?

[chris fleizach]

Created attachment 163944 [details]
patch

[chris fleizach]

(In reply to comment #8)
> (From update of attachment 163644 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163644&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        (WebCore):
> 
> Please remove bogus lines like this one.
> 
> > Source/WebCore/ChangeLog:19
> > +        (AccessibilityRenderObject):
> 
> Please remove bogus lines like this one.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.h:249
> > +    virtual RenderObject* findLegend(RenderObject*) const;
> 
> Why virtual?
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1054
> > +    // This is very similar to RenderFieldset::findLegend, except that it allows using legends
> > +    // that are positioned off-screen for accessibility.
> > +    for (RenderObject* legend = fieldset->firstChild(); legend; legend = legend->nextSibling()) {
> > +        if (legend->style()->visibility() == VISIBLE && legend->node() && legend->node()->hasTagName(legendTag))
> > +            return legend;
> > +    }
> > +    return 0;
> 
> We may want this new function to be a RenderFieldset member even though its use is accessibility-specific. I’m concerned that this code will get out of sync of RenderFieldset::findLegend if the logic has reason to change there.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1065
> >      if (isFieldset())
> > -        return axObjectCache()->getOrCreate(toRenderFieldset(m_renderer)->findLegend());
> > +        if (RenderObject* legend = findLegend(m_renderer))
> > +            return axObjectCache()->getOrCreate(legend);
> 
> Need braces since the body of the isFieldset() if is now more than one line.
> 
> The old code did not check for 0, but the new code does. Is that OK?

Thanks for the feedback. I updated to modify the existing findLegend() function so that it could support this need. That simplified the patch a bit

[WebKit Review Bot]

Comment on attachment 163944 [details]
patch 

Rejecting attachment 163944 [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:
ceeded at 1 with fuzz 3.
patching file LayoutTests/accessibility/hidden-legend.html
patching file LayoutTests/accessibility/hidden-legend-expected.txt
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 1398.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Beth Dakin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14461317

[chris fleizach]

http://trac.webkit.org/changeset/131908

[chris fleizach]

rdar://11769140

[Roger Fong]

Is accessibility/hidden-legend.html another test that should be skipped on Windows or get Windows specific results?

[chris fleizach]

Comment on attachment 163944 [details]
patch 

my guess is that it should be skipped on windows until titleUIElement() is implemented

[Roger Fong]

(In reply to comment #15)
> (From update of attachment 163944 [details])
> my guess is that it should be skipped on windows until titleUIElement() is implemented

Ok I'll stick it on the list.