Bug 156774 – AX: <svg> elements with labels and no accessible contents are exposed as empty AXGroups

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

SVG shapes with labels should be exposed in AX tree.

<rdar://problem/22530811>

[Nan Wang]

Created attachment 276787 [details]
Initial patch

[Joanmarie Diggs]

Nan: Can you show me where in the SVG spec it states that the property on the parent should cause a shape to be exposed?

[Nan Wang]

(In reply to comment #2)
> Nan: Can you show me where in the SVG spec it states that the property on
> the parent should cause a shape to be exposed?

I don't think the spec has included this case. But we have bugs that svg root is labelled but shape in it is not exposed. 

So you think we should not expose it this way?

[Joanmarie Diggs]

(In reply to comment #3)
> (In reply to comment #2)
> > Nan: Can you show me where in the SVG spec it states that the property on
> > the parent should cause a shape to be exposed?
> 
> I don't think the spec has included this case. But we have bugs that svg
> root is labelled but shape in it is not exposed. 
> 
> So you think we should not expose it this way?

I think we should follow the spec. If you don't like what the spec says (I have had those occasions myself), then you can raise an issue with the task force (which I've done myself). :)

The particular case I question is Circle2. Looking at the specs which govern this implementation:

5.1.1 Excluding Elements from the Accessibility Tree
https://rawgit.com/w3c/aria/master/svg-aam/svg-aam.html#exclude_elements

States the following:

     The following elements in the SVG namespace should not be included
     in the accessibility tree, except as described under Including Elements
     in the Accessibility Tree:

     * shape elements (circle, ellipse, line, path, polygon, polyline, rect)

Looking at the referenced section: 5.1.2 Including Elements in the Accessibility Tree
https://rawgit.com/w3c/aria/master/svg-aam/svg-aam.html#include_elements

It lists criteria for inclusion. But having an *ancestor* with a global ARIA property isn't one of them. That section then points to the Core AAM, but Core AAM doesn't say that an ancestor with a global ARIA property requires insertion.

So unless I'm missing something, you're introducing a change which will cause our implementation to not comply with the W3C specification we are implementing.

Again, that doesn't mean the spec is right. And if you have a valid reason for thinking they are wrong, now is the time to tell them (i.e. while they're still working on the spec). :)

[Build Bot]

Comment on attachment 276787 [details]
Initial patch

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

New failing tests:
accessibility/w3c-svg-elements-not-exposed.html

[Build Bot]

Created attachment 276792 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 276787 [details]
Initial patch

Attachment 276787 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1188956

New failing tests:
accessibility/w3c-svg-elements-not-exposed.html

[Build Bot]

Created attachment 276800 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 276787 [details]
Initial patch

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

New failing tests:
accessibility/w3c-svg-elements-not-exposed.html

[Build Bot]

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

[Fred Esch]

Hi Nan,

I believe I found the SVG for the failed test and have the SVG below. If I didn't find the right SVG for the failed test, please show me the correct SVG so we can discuss the SVG and expected result.

In the following SVG, the path element would not be included in the accessibility tree as it does not include any global aria properties or have a title element child or desc element child. Ancestor elements do not affect the path elements inclusion/exclusion in the accessibility tree.

<svg aria-label="some">
  <path id="test24" d="M 100 100 L 300 100 L 200 300 z" fill="red" stroke="blue" stroke-width="3"/>
</svg>

Likewise, in the following, none of the circles would be included in the accessibility tree.

<svg aria-label="Bar" height="100" width="100">
  <circle id="circle1" cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />	
</svg>	

<svg height="100" width="100">
  <circle id="circle2" aria-label="Bar" cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />	
</svg>


<svg height="100" width="100">
  <circle id="circle3" cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
</svg>


I believe Joanie gave you some pointers to the specs. We also have a wiki you can check https://www.w3.org/wiki/SVG_Accessibility/Testing/Test_Assertions_with_Tables and https://www.w3.org/wiki/SVG_Accessibility/Testing/Test_Assertions/Language_Tables. If you find any inconsistencies in the specs and/or the wiki pages, please bring them to my attention and I will get the task force to address the issue.

Fred Esch
Co-lead SVG accessibility task force
[e-mail]

[Nan Wang]

(In reply to comment #11)
> <svg aria-label="some">
>   <path id="test24" d="M 100 100 L 300 100 L 200 300 z" fill="red"
> stroke="blue" stroke-width="3"/>
> </svg>

Sorry I have accidentally included this for debugging.


> <svg height="100" width="100">
>   <circle id="circle2" aria-label="Bar" cx="50" cy="50" r="40"
> stroke="black" stroke-width="3" fill="red" />	
> </svg>

Should this be included? Since there's a label directly describing the circle.

Thanks Joanmarie and Fred, I'll try to read those specs and wiki carefully and let you know.

[Fred Esch]

Hi Nan,

You are correct a circle with an aria attribute like aria-label would be included in the accessibility tree.  Thanks for pointing that out. 

Fred

[Nan Wang]

Fred and Joanmarie,

I think both of you are right about the specs, that there's no rules saying we should expose a shape based on the parent's attribute.

However, the reason we are excluding shapes from the spec is that:
"Many SVG elements—although rendered to the screen—do not have an intrinsic semantic meaning. Instead, they represent components of the visual presentation of the document. To simplify the accessible representation of the document, these purely presentational elements would normally be omitted from the accessibility tree, unless the author explicitly provides semantic content."

To my understanding, when author provides some semantic content on the svg root parent, he's intention is to make the entire group accessible, otherwise he should just label the individual children. Maybe to make my scenario more reasonable, I think we can say when the svg parent has a semantic description and there's no other accessible children in it, we should expose the shape.

Let me know what you think. Thanks!

[Joanmarie Diggs]

(In reply to comment #14)
 
> To my understanding, when author provides some semantic content on the svg
> root parent, he's intention is to make the entire group accessible,
> otherwise he should just label the individual children. Maybe to make my
> scenario more reasonable, I think we can say when the svg parent has a
> semantic description and there's no other accessible children in it, we
> should expose the shape.
> 
> Let me know what you think. Thanks!

If we do that, we'll expose the shape which has no useful information. All ATs will get is a nameless, descriptionless, helptextless object with a role of AXImage. As a result, the screen reader user will just hear "image."

I think hearing "image" without any indication whatsoever about what the image is offers no valuable information to the user. It's just noise. It's like an HTML 'img' element without an 'alt' attribute.

[Fred Esch]

Hi Nan,

To add to what Joanie said. All shapes are not semantically important. When an author believes a shape is important then they can put a property or child title/desc which will get the shape included in the accessibility tree and hopefully provides a useful name for the user. 

When we create charts, I would not want the neat lines, borders and backgrounds announced to the user simply because they are created from shapes. The key idea is to allow the author explicit control of what gets into the accessibility tree.

Fred

[chris fleizach]

(In reply to comment #16)
> Hi Nan,
> 
> To add to what Joanie said. All shapes are not semantically important. When
> an author believes a shape is important then they can put a property or
> child title/desc which will get the shape included in the accessibility tree
> and hopefully provides a useful name for the user. 
> 
> When we create charts, I would not want the neat lines, borders and
> backgrounds announced to the user simply because they are created from
> shapes. The key idea is to allow the author explicit control of what gets
> into the accessibility tree.
> 
> Fred

I think in this case we're talking about someone adding an aria-label/aria-labelledby to a svg root and there's no other accessible children. in that case it seems like we should expose that element right? the author probably wanted to expose something to the user

[Joanmarie Diggs]

(In reply to comment #17)
> I think in this case we're talking about someone adding an
> aria-label/aria-labelledby to a svg root and there's no other accessible
> children. in that case it seems like we should expose that element right?
> the author probably wanted to expose something to the user

If the svg root has an aria-label/aria-labelledby [*], we already expose that svg root.

I believe the question at hand is if that exposed svg root has a child shape (e.g. a circle element), should we include that child shape?

From the newly-proposed test and patch and comments, I believe Nan is saying "yes". Fred and I are saying "only if that child shape itself has attributes like [*] aria-label/aria-labelledby." The latter is what we're already doing.

[*] Or a title element child, or a desc element child, or is focusable, etc.

[Nan Wang]

(In reply to comment #18)
> (In reply to comment #17)
> > I think in this case we're talking about someone adding an
> > aria-label/aria-labelledby to a svg root and there's no other accessible
> > children. in that case it seems like we should expose that element right?
> > the author probably wanted to expose something to the user
> 
> If the svg root has an aria-label/aria-labelledby [*], we already expose
> that svg root.
> 
> I believe the question at hand is if that exposed svg root has a child shape
> (e.g. a circle element), should we include that child shape?
> 
> From the newly-proposed test and patch and comments, I believe Nan is saying
> "yes". Fred and I are saying "only if that child shape itself has attributes
> like [*] aria-label/aria-labelledby." The latter is what we're already doing.
> 
> [*] Or a title element child, or a desc element child, or is focusable, etc.

Thanks for the clarification. I think the problem is that we are ignoring the empty group even if it's exposed. Maybe we should fix it in the screenreader side.

[Joanmarie Diggs]

(In reply to comment #19)
> Thanks for the clarification. I think the problem is that we are ignoring
> the empty group even if it's exposed. Maybe we should fix it in the
> screenreader side.

Either that or the role mapping should be changed.

I just did a quick test case on my Mac:

<html>
<head></head>
<body>
<svg aria-label="hello world">
  <circle cx="50"cy="100" r="30"/>
</svg>
</body>
</html>

When I look at that using the Accessibility Inspector, I get the following hierarchy:

-> AXScrollArea
   -> AXWebArea
      -> AXGroup
         -> AXGroup with AXDescription "hello world"

And it's a similar situation on my platform:

-> SCROLL_PANE
   -> DOCUMENT_WEB
      -> PANEL
         -> PANEL with accessible name "hello world"

So the one thing with accessible information IS there.

That said.... On my platform (and I'm guessing on yours), group-like roles are expected to have stuff in them. :) So a group of nothing is admittedly not great. And if that's the problem you're trying to solve, I don't think the way to solve it is by given it a <fingerquotes>useless</fingerquotes> child. But I'll grant you solving it is probably a good idea. :)

What about mapping exposed SVG root elements which lack accessible children as AXImage on each of our platforms? NOTE: SVG root elements which have accessible children should, I think still be AXGroup (my platform's PANEL). Because if there are children, we'd really have a group.

Would that work? (And does that make sense to everyone?)

[Joanmarie Diggs]

(Sorry for being spammy)

@Nan: If you (and everyone else) agrees with the changed role mapping to address this particular case, let's add new test cases to LayoutTests/accessibility/w3c-svg-roles.html.

[Nan Wang]

(In reply to comment #20) 
> What about mapping exposed SVG root elements which lack accessible children
> as AXImage on each of our platforms? NOTE: SVG root elements which have
> accessible children should, I think still be AXGroup (my platform's PANEL).
> Because if there are children, we'd really have a group.
> 
> Would that work? (And does that make sense to everyone?)

I think that makes sense to me. And it'll cover most cases.

[Joanmarie Diggs]

(In reply to comment #22)
> (In reply to comment #20) 
> > What about mapping exposed SVG root elements which lack accessible children
> > as AXImage on each of our platforms? NOTE: SVG root elements which have
> > accessible children should, I think still be AXGroup (my platform's PANEL).
> > Because if there are children, we'd really have a group.
> > 
> > Would that work? (And does that make sense to everyone?)
> 
> I think that makes sense to me. And it'll cover most cases.

I raised the idea in this discussion: https://github.com/w3c/aria/issues/347

The impression that I get is that it doesn't make sense to the task force. As a result, I think this bug is a WONTFIX and we need to handle it within our respective assistive technologies.

Fred, please confirm.

[James Craig]

I can confirm that the shapes should not be exposed on their own, but I don't think this bug is complete.

However, going back to the original test cases, this bug (rather the Radar associated with this) was not about SVG shape elements. It was about the containing <svg> element.


In the following test case:

<svg aria-label="foo" height="100" width="100">
  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red"></circle>
</svg>

The circle itself should not be exposed, but the author labeled the image, so the image should be exposed. Since there are no accessible contents of the SVG, I'd expose this as a single AXImage with a label "foo".

In the next test case:

<svg height="100" width="100">
  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red"></circle>
</svg>

Neither the <svg> or the <circle> is labeled, so you could either expose this as an unlabeled AXImage, or as an ignored element. I think it's safe to leave this one ignored.

Ignore the test cases that tried to use the <label> element to point to SVG. Those don't make any sense.

[James Craig]

For <svg> elements with accessible contents, we decided to expose them as AXGroups because VoiceOver on OS X currently treats AXImages as leaf nodes (it won't traverse into descendants of an AXImage). If that were to change in a future version, it may be better to expose these as an AXImage with a subrole — I think we discussed AXImage:AXInteractiveImage or something to that effect.

For simple SVG elements with no accessible descendants (like the test cases above), there is no benefit to keeping the AXGroup role, so I think we should expose those as flat AXImages.

[James Craig]

<rdar://problem/22530811>

[Joanmarie Diggs]

James: I'm a little bit confused. You changed the summary to "AX: <svg> elements with labels are not exposed in the AX API". However, if you look at what I stated in comment #20, <svg> elements with labels ARE exposed in the AX API." (They just happen to be exposed with as AXGroup.)

Please clarify. Thanks!

[Nan Wang]

I think James' idea is to expose the svg root as AXImage if it's labelled. Which we have already discussed before and the task force didn't like that.

[James Craig]

(In reply to comment #27)
> James: I'm a little bit confused. You changed the summary to "AX: <svg>
> elements with labels are not exposed in the AX API". However, if you look at
> what I stated in comment #20, <svg> elements with labels ARE exposed in the
> AX API." (They just happen to be exposed with as AXGroup.)
> 
> Please clarify. Thanks!

Further clarifying the name:
AX: <svg> elements with labels and no accessible contents are exposed as empty AXGroups

(In reply to comment #28)
> I think James' idea is to expose the svg root as AXImage if it's labelled.
> Which we have already discussed before and the task force didn't like that.

If I'm reading the SVGATF comments correctly, they were talking about the ARIA group role, not the OS X AXGroup role. They are used differently.

It seems to me like most are in agreement that a labeled SVG with no accessible descendants can and should be exposed as an image on the platforms where empty generic groups are ignored by default. Joanie first proposed this, and I echoed it later independently (sorry for missing your first comment, Joanie)...

Also:
- If the SVG has accessible contents, one of the platform container roles should be used.
- If an image role with descendants also works, that is preferred.

So, on the OS X implementation:

- unlabeled SVG without accessible descendants will be ignored.
- labeled SVG without accessible descendants will be exposed as a labeled AXImage.
- unlabeled SVG with accessible descendants will have the descendants exposed directly.
- labeled SVG with accessible descendants will be exposed as an AXGroup (not equiv to role=group).

Is this a fair representation of the discussion across multiple bug trackers? Please correct me if I've missed some conflicting viewpoint. Thanks.

[Eric Liang]

Created attachment 365904 [details]
patch

[Eric Liang]

Created attachment 365905 [details]
Patch

[Eric Liang]

Created attachment 365906 [details]
Patch

[EWS Watchlist]

Comment on attachment 365906 [details]
Patch

Attachment 365906 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11664960

New failing tests:
accessibility/svg-shape-labelled.html

[EWS Watchlist]

Created attachment 365914 [details]
Archive of layout-test-results from ews102 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 365906 [details]
Patch

Attachment 365906 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11664986

New failing tests:
accessibility/svg-shape-labelled.html

[EWS Watchlist]

Created attachment 365918 [details]
Archive of layout-test-results from ews113 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 365906 [details]
Patch

Attachment 365906 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11669997

New failing tests:
accessibility/svg-shape-labelled.html

[EWS Watchlist]

Created attachment 365951 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Eric Liang]

Created attachment 365973 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 365973 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Labelled SVGs without accessible descendants are exposed as AXImage rather than groups. Unlabelled equivalents are not exposed. Otherwise, SVGs with accessible descendants are exposed as AXGroup

This is an awfully long line. Please hard-wrap the line at the end of the first sentence.

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

This line should appear after the URL but before the long description.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3280
> +        if (!children().size())

Can we just check hasChildren() or do we need to call updateChildrenIfNecessary()?

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:253
> +            if (parent->hasAttributesRequiredForInclusion())
> +                return false;

Why should hasAttributesRequiredForInclusion returning true on some ancestor affect
whether this SVG shape element will have an accessibility object or not?
That doesn't seem right.

If anything, we need to be checking that descendent nodes are labeled.

[Eric Liang]

It should check the parent because if the parent svg element is labelled, then its descendants, even not labelled, need to be accessible. For SVGs, computing isIgnore upwards is needed because by default, non-labelled svg shapes are ignored.

[Eric Liang]

Created attachment 366027 [details]
Patch

[Ryosuke Niwa]

(In reply to Eric Liang from comment #41)
> It should check the parent because if the parent svg element is labelled,
> then its descendants, even not labelled, need to be accessible. For SVGs,
> computing isIgnore upwards is needed because by default, non-labelled svg
> shapes are ignored.

But what if some parent had visibility: hidden, etc... such that the content shouldn't be displayed? It doesn't seem to make much sense to always include a SVG shape element as long as *some* ancestor has a label.

Also, we don't seem to have any tests with SVG use elements. We definitely need to test that.

[chris fleizach]

Comment on attachment 366027 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3279
> +    if (roleValue() == AccessibilityRole::SVGRoot) {

we should cache the role

auto role = roleValue();

then this should be 

else if (role == AccessibilityRole::SVGRoot && !hasChildren())
   m_role =

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:251
> +        for (const AccessibilityObject* parent = this; parent; parent = parent->parentObject()) {

can you use the matchedParent closure style

[Eric Liang]

(In reply to Ryosuke Niwa from comment #43)
> (In reply to Eric Liang from comment #41)
> > It should check the parent because if the parent svg element is labelled,
> > then its descendants, even not labelled, need to be accessible. For SVGs,
> > computing isIgnore upwards is needed because by default, non-labelled svg
> > shapes are ignored.
> 
> But what if some parent had visibility: hidden, etc... such that the content
> shouldn't be displayed? It doesn't seem to make much sense to always include
> a SVG shape element as long as *some* ancestor has a label.
> 
> Also, we don't seem to have any tests with SVG use elements. We definitely
> need to test that.

In those case, the computed children is ignored, but the root element is hidden, so then even though children are not ignored, the parent is, thus making the children hidden.

[Eric Liang]

Created attachment 366943 [details]
Patch

[Eric Liang]

Comment on attachment 366027 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:251
>> +        for (const AccessibilityObject* parent = this; parent; parent = parent->parentObject()) {
> 
> can you use the matchedParent closure style

Sure. though the check has to run twice.

[WebKit Commit Bot]

Comment on attachment 366943 [details]
Patch

Clearing flags on attachment: 366943

Committed r244029: <https://trac.webkit.org/changeset/244029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.