Bug 115633 – [GTK] accessibility/meter-element.html is failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The accessibility/meter-element.html layout test has been failing since introduced in r149155
http://trac.webkit.org/changeset/149155

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmeter-element.html

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/meter-element-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/meter-element-actual.txt
@@ -4,45 +4,45 @@
 
 
 Meter1
-AXRole: AXProgressIndicator
-AXTitle: 6 blocks used (out of 8 total)
-AXDescription: 
+AXRole: AXUnknown
+AXTitle: 
+
 
 
 Meter2
-AXRole: AXProgressIndicator
-AXTitle: 75%
-AXDescription: 
+AXRole: AXUnknown
+AXTitle: 
+
 
 
 Meter3
-AXRole: AXProgressIndicator
+AXRole: AXUnknown
 AXTitle: 
-AXDescription: 
+
 
 
 Meter4
-AXRole: AXProgressIndicator
-AXTitle: 12cm
-AXDescription: 
+AXRole: AXUnknown
+AXTitle: 
+
 
 
 Meter5
-AXRole: AXProgressIndicator
-AXTitle: 2cm
-AXDescription: 
+AXRole: AXUnknown
+AXTitle: 
+
 
 
 Meter6
-AXRole: AXProgressIndicator
-AXTitle: 12cm
-AXDescription: 
+AXRole: AXGroup
+AXTitle: centimeters
+AXDescription: centimeters
 
 
 Meter7
-AXRole: AXProgressIndicator
-AXTitle: 2cm
-AXDescription: 
+AXRole: AXGroup
+AXTitle: centimeters
+AXDescription: centimeters
 
 
 PASS successfullyParsed is true

[Joanmarie Diggs]

Created attachment 278794 [details]
Patch

[Joanmarie Diggs]

Chris: This is another bug whose summary starts with [GTK], but which also includes a proposed change to WebCore Accessibility (and a corresponding one to your port's wrapper).

[chris fleizach]

Comment on attachment 278794 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        autors can supply the numeric unit as the value of title.

autors -> authors

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:73
> +    if (m_renderer->isMeter()) {

We should early return here

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:83
> +            for (const auto& child : childrenOfType<HTMLImageElement>(*meter)) {

Can we use textUnderElement() here?
Is there another alternative we can create in a different function (textFromImageChildren()?)
Also what if they use aria-label or aria-labelledby on the image?

> Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:156
> +    if (is<HTMLMeterElement>(node))

Should this be in WebCore land? Might it apply to all platforms?

[Joanmarie Diggs]

Thanks for the review! Regarding this:

> Can we use textUnderElement() here?

It doesn't "just work." We could modify textUnderElement() to handle meters and then we could use it. But do we actually want textUnderElement() to work for meters?

I'll grant you that given:

    <meter ...>text alternative</meter>

"Text alternative" IS the text under the meter element. But for all intents and purposes, it's functioning like the value of an alt attribute.

At least in my mind, textUnderElement() has always implied rendered text. And looking at AccessibilityNodeObject::visibleText() there also seems to be that implication: visibleText() calls textUnderElement().

Lastly if we want to expose this text via valueDescription(), are we in danger of it being doubly-exposed if we also make the text available via textUnderElement()?

All of this said, I'm not fundamentally opposed to exposing it via textUnderElement() if you think that it's what we want to do.

[chris fleizach]

(In reply to comment #4)
> Thanks for the review! Regarding this:
> 
> > Can we use textUnderElement() here?
> 
> It doesn't "just work." We could modify textUnderElement() to handle meters
> and then we could use it. But do we actually want textUnderElement() to work
> for meters?
> 
> I'll grant you that given:
> 
>     <meter ...>text alternative</meter>
> 
> "Text alternative" IS the text under the meter element. But for all intents
> and purposes, it's functioning like the value of an alt attribute.
> 
> At least in my mind, textUnderElement() has always implied rendered text.
> And looking at AccessibilityNodeObject::visibleText() there also seems to be
> that implication: visibleText() calls textUnderElement().
> 
> Lastly if we want to expose this text via valueDescription(), are we in
> danger of it being doubly-exposed if we also make the text available via
> textUnderElement()?
> 
> All of this said, I'm not fundamentally opposed to exposing it via
> textUnderElement() if you think that it's what we want to do.

It sounds like textUnderElement() is not right, but we should probably have some helper method to get the text from accessible images that handles alt, and aria-*

[Joanmarie Diggs]

(In reply to comment #5)

> but we should probably have
> some helper method to get the text from accessible images that handles alt,
> and aria-*

Definitely. I think that's a great idea and will do so.

Thanks again!

[Joanmarie Diggs]

Created attachment 279079 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 279079 [details]
Patch

(In reply to comment #3)
> > Source/WebCore/ChangeLog:17
> > +        autors can supply the numeric unit as the value of title.
> 
> autors -> authors

Fixed.

> > Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:73
> > +    if (m_renderer->isMeter()) {
> 
> We should early return here

Done.

> > Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:83
> > +            for (const auto& child : childrenOfType<HTMLImageElement>(*meter)) {
> 
> Can we use textUnderElement() here?

Not done as per our discussion in subsequent comments.

> Is there another alternative we can create in a different function
> (textFromImageChildren()?)

Added AccessibilityNodeObject:accessibilityDescriptionForChildren().

In coming up with new test cases (7 have subsequently been added), it occurred to me that maybe authors might get more creative than a single image or plain text. For this reason, the alternative handles more than images.

Related to the above, AccessibilityProgressIndicator::valueDescription() now calls the new accessibilityDescriptionForChildren() and then falls back on textContent(). If the author-provided alternative is a series of colored Unicode box chars in a div with a proper ARIA alternative, we don't want the value description to be the colored Unicode box chars....

> Also what if they use aria-label or aria-labelledby on the image?

Indeed. Those are amongst the seven new test cases, along with an instance where both are used so we can verify the latter trumps the former.

> > Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:156
> > +    if (is<HTMLMeterElement>(node))
> 
> Should this be in WebCore land? Might it apply to all platforms?

Moved to WebCore. It doesn't apply to your platform (I haven't debugged why/where it's getting pruned on the Mac). But what was triggering it to show up on mine is that the title attribute is one of the things checked by hasAttributesRequiredForInclusion(). So it *could* potentially apply to all platforms (or at least more than just mine).

Thanks again!

[chris fleizach]

Comment on attachment 279079 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1904
> +        if (AccessibilityObject* axObject = node->document().axObjectCache()->getOrCreate(child)) {

I think the nodes will all be in the same document, and just called axObjectCache() on the AX object should be sufficient

> Source/WebCore/accessibility/AccessibilityNodeObject.h:126
> +    String accessibilityDescriptionForChildren() const;

Is there a reason not to be in this in AXObject?
It would save us a check and a cast later on

[Joanmarie Diggs]

> I think the nodes will all be in the same document, and just called
> axObjectCache() on the AX object should be sufficient

In the case of meters, I think you're right. I'll make that change.

> > Source/WebCore/accessibility/AccessibilityNodeObject.h:126
> > +    String accessibilityDescriptionForChildren() const;
> 
> Is there a reason not to be in this in AXObject?

Because (in my mind) it is only applicable to nodes which might not be rendered. I figured that if and when we had a need for such a method for render objects, we could add it to AccessibilityRenderObject, similar to the situation we have with textUnderElement().

Beyond that, accessibilityDescriptionForChildren() does its work based on accessibleNameForNode() and appendNameToStringBuilder(). Moving the latter, along with shouldAddSpaceBeforeAppingingNextElement(), to AccessibilityObject makes sense to me because that functionality is not node-specific. Do we have a need/desire to make accessibleNameForNode() public?

All of this said, I don't object to making these changes; I'm just checking that is indeed something we want to do.

[chris fleizach]

(In reply to comment #10)
> > I think the nodes will all be in the same document, and just called
> > axObjectCache() on the AX object should be sufficient
> 
> In the case of meters, I think you're right. I'll make that change.
> 
> > > Source/WebCore/accessibility/AccessibilityNodeObject.h:126
> > > +    String accessibilityDescriptionForChildren() const;
> > 
> > Is there a reason not to be in this in AXObject?
> 
> Because (in my mind) it is only applicable to nodes which might not be
> rendered. I figured that if and when we had a need for such a method for
> render objects, we could add it to AccessibilityRenderObject, similar to the
> situation we have with textUnderElement().
> 
> Beyond that, accessibilityDescriptionForChildren() does its work based on
> accessibleNameForNode() and appendNameToStringBuilder(). Moving the latter,
> along with shouldAddSpaceBeforeAppingingNextElement(), to
> AccessibilityObject makes sense to me because that functionality is not
> node-specific. Do we have a need/desire to make accessibleNameForNode()
> public?

> 
> All of this said, I don't object to making these changes; I'm just checking
> that is indeed something we want to do.

I think it's probably OK to leave in AXNodeObject.

[Joanmarie Diggs]

Created attachment 279213 [details]
patch for landing

[Build Bot]

Comment on attachment 279213 [details]
patch for landing

Attachment 279213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1340830

New failing tests:
imported/blink/storage/indexeddb/blob-basics-metadata.html

[Build Bot]

Created attachment 279218 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joanmarie Diggs]

Created attachment 279248 [details]
Patch

[Joanmarie Diggs]

Created attachment 279253 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 279253 [details]
Patch

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

Chris: I'm waiting for EWS, but this patch has changes necessitating a new review. They are pointed out and explained inline.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1358
> -    case ProgressIndicatorRole:

This change is new. If visibleText() is meant to return text which is visible, I do not think ProgressIndicatorRole should be included in the roles which use textUnderElement(). There is no rendered/visible text. At least not functionally. Looking at the render tree for meter-element.html, I see RenderText objects with a text run value of " ". While I've not fully debugged it yet, I think that this may be causing us to insert the non-empty ChildrenText into the text order which has a side effect. (See next inline comment.)

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1407
> +            textOrder.append(AccessibilityText(title, HelpText));

This block is new. And I will admit, it's a hack. But the reason it was needed is that ATK is now defaulting to WebCore Accessibility's AccessibilityText. And if the only text alternative provided is in the title tag, it winds up becoming the name as per the HTML AAM spec. This, combined with the fact that the HTML spec suggests authors use the title attribute as a means to express the meter's units, makes me think that all platforms would want to treat a meter's TitleTagText as HelpText.

Also, if you accept the change I'm proposing to visibleText() and this hack is not in place, the TitleTagText shows up in your AXDescription rather than your AXHelp. Because I'm in a meeting now, I've not yet debugged it. But my guess is that, as described above, the Mac is finding the single-space RenderText contents and taking that as evidence of visible text existing for the element. And that visible text causes the title attribute value to instead be exposed via your AXHelp. My suspicion is that if the render tree were updated so that meters lacked RenderText objects, your platform would start having meter's named "centimeters".

[chris fleizach]

Comment on attachment 279253 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1358
>> -    case ProgressIndicatorRole:
> 
> This change is new. If visibleText() is meant to return text which is visible, I do not think ProgressIndicatorRole should be included in the roles which use textUnderElement(). There is no rendered/visible text. At least not functionally. Looking at the render tree for meter-element.html, I see RenderText objects with a text run value of " ". While I've not fully debugged it yet, I think that this may be causing us to insert the non-empty ChildrenText into the text order which has a side effect. (See next inline comment.)

This seems right based on your debugging

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1407
>> +            textOrder.append(AccessibilityText(title, HelpText));
> 
> This block is new. And I will admit, it's a hack. But the reason it was needed is that ATK is now defaulting to WebCore Accessibility's AccessibilityText. And if the only text alternative provided is in the title tag, it winds up becoming the name as per the HTML AAM spec. This, combined with the fact that the HTML spec suggests authors use the title attribute as a means to express the meter's units, makes me think that all platforms would want to treat a meter's TitleTagText as HelpText.
> 
> Also, if you accept the change I'm proposing to visibleText() and this hack is not in place, the TitleTagText shows up in your AXDescription rather than your AXHelp. Because I'm in a meeting now, I've not yet debugged it. But my guess is that, as described above, the Mac is finding the single-space RenderText contents and taking that as evidence of visible text existing for the element. And that visible text causes the title attribute value to instead be exposed via your AXHelp. My suspicion is that if the render tree were updated so that meters lacked RenderText objects, your platform would start having meter's named "centimeters".

Hmm, I think in this case we would want the TitleTagText to be exposed as either the description or valueDescription field if its meant for units. The HelpText is basically text that won't be sent to the user frequently.. At least on *OS we delay about 2-3 seconds before outputing any help text. But in this case it seems like that information is more pertinent

[Joanmarie Diggs]

(In reply to comment #18)

> Hmm, I think in this case we would want the TitleTagText to be exposed as
> either the description or valueDescription field if its meant for units. The
> HelpText is basically text that won't be sent to the user frequently.. At
> least on *OS we delay about 2-3 seconds before outputing any help text. But
> in this case it seems like that information is more pertinent

The current status (i.e. without my patch) is that the TitleTagText on meters is exposed at AXHelp. Is the existing behavior wrong?

It is, of course, your platform. And I'm happy to do a new patch to change the behavior on your platform to whatever you see fit. But first please let me make a case for why what your platform already does is correct. :)

<case>
Consider the two examples provided in the spec, the first of which can be found here: https://www.w3.org/TR/html51/single-page.html#example-b91dfe3c

I think the valueDescription should always contain the author-provided alternative text. This is what we're already doing on your platform. From the meters in the first example, that would be:

* 6 blocks used (out of 8 total)
* 75%
* (nothing)
* 12cm
* 2cm

Then the spec states:

-------------------------
There is no explicit way to specify units in the meter element, but the units may be specified in the title attribute in free-form text.

CODE EXAMPLE:
The example above could be extended to mention the units:
<dl>
  <dt>Radius: <dd> <meter min=0 max=20 value=12 title="centimeters">12cm</meter>
  <dt>Height: <dd> <meter min=0 max=10 value=2 title="centimeters">2cm</meter>
</dl>
-------------------------

So the value descriptions would still be:
* 12cm
* 2cm

And VoiceOver presents that. The title's "centimeters" is redundant to us and our users. The title attribute is helpful to sighted users who just see a graphical representation and are able to hover the mouse pointer over it.

We of course can't just do a blanket ignore of the title attribute because we have no idea what it contains. But given that the spec provides attributes and techniques and examples that will hopefully result in the accessible value description being meaningful and complete without the title attribute, I think exposing it via AXHelp (again, like you already do) makes good sense.
</case>

If you're not convinced, AXDescription? Append it to the valueDescription? Something else?

[chris fleizach]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > Hmm, I think in this case we would want the TitleTagText to be exposed as
> > either the description or valueDescription field if its meant for units. The
> > HelpText is basically text that won't be sent to the user frequently.. At
> > least on *OS we delay about 2-3 seconds before outputing any help text. But
> > in this case it seems like that information is more pertinent
> 
> The current status (i.e. without my patch) is that the TitleTagText on
> meters is exposed at AXHelp. Is the existing behavior wrong?
> 
> It is, of course, your platform. And I'm happy to do a new patch to change
> the behavior on your platform to whatever you see fit. But first please let
> me make a case for why what your platform already does is correct. :)
> 
> <case>
> Consider the two examples provided in the spec, the first of which can be
> found here: https://www.w3.org/TR/html51/single-page.html#example-b91dfe3c
> 
> I think the valueDescription should always contain the author-provided
> alternative text. This is what we're already doing on your platform. From
> the meters in the first example, that would be:
> 
> * 6 blocks used (out of 8 total)
> * 75%
> * (nothing)
> * 12cm
> * 2cm
> 
> Then the spec states:
> 
> -------------------------
> There is no explicit way to specify units in the meter element, but the
> units may be specified in the title attribute in free-form text.
> 
> CODE EXAMPLE:
> The example above could be extended to mention the units:
> <dl>
>   <dt>Radius: <dd> <meter min=0 max=20 value=12
> title="centimeters">12cm</meter>
>   <dt>Height: <dd> <meter min=0 max=10 value=2
> title="centimeters">2cm</meter>
> </dl>
> -------------------------
> 
> So the value descriptions would still be:
> * 12cm
> * 2cm
> 
> And VoiceOver presents that. The title's "centimeters" is redundant to us
> and our users. The title attribute is helpful to sighted users who just see
> a graphical representation and are able to hover the mouse pointer over it.
> 
> We of course can't just do a blanket ignore of the title attribute because
> we have no idea what it contains. But given that the spec provides
> attributes and techniques and examples that will hopefully result in the
> accessible value description being meaningful and complete without the title
> attribute, I think exposing it via AXHelp (again, like you already do) makes

Hmm, so it seems like here title still behaves as a tooltip. In which case I think it makes sense to expose as help text

> good sense.
> </case>
> 
> If you're not convinced, AXDescription? Append it to the valueDescription?
> Something else?

[WebKit Commit Bot]

Comment on attachment 279253 [details]
Patch

Clearing flags on attachment: 279253

Committed r201087: <http://trac.webkit.org/changeset/201087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.