Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARIA role attribute should be allowed on SVG elements #769

Closed
rdeltour opened this issue Jun 12, 2017 · 17 comments
Closed

ARIA role attribute should be allowed on SVG elements #769

rdeltour opened this issue Jun 12, 2017 · 17 comments
Labels
status: has PR The issue is being processed in a pull request type: bug The issue describes a bug
Milestone

Comments

@rdeltour
Copy link
Member

From @clapierre:

In my last EPUB with images in our SVG images we have

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="7.421ex" height="2.806ex" viewBox="0 -845 3195 1208.3" role="img" focusable="false”>

But getting the following EPUB Check errors

ERROR(RSC-005): mml-image.epub/OEBPS/Images/epub-image-x2y2.svg(1,175): Error while parsing file 'attribute "role" not allowed here;

@rdeltour rdeltour added the type: bug The issue describes a bug label Jun 12, 2017
@murata2makoto
Copy link
Contributor

Which element of the SVG elements should the role attribute be allowed? Every element?

@mattgarrish
Copy link
Member

By SVG 2, it's allowed on any renderable element.[1]

The HTML validator has backported this support to 1.1, so seems like we should do the same.

[1] https://www.w3.org/TR/SVG2/render.html#TermRenderableElement

@mattgarrish
Copy link
Member

mattgarrish commented Jul 7, 2017

I see now where allowing role comes from, since it's not defined in SVG 1.1. In HTML 5.1 it says under the SVG section:

SVG as implemented today follows neither SVG 1.1 nor SVG Tiny 1.2 precisely, instead implementing subsets of each. Although it is hoped that the in-progress SVG 2 specification is a more realistic target for implementations, until that specification is ready, user agents must implement the SVG 1.1 specification with the following willful violations and additions. [SVG11] [SVGTiny12] [SVG2]
...
The ARIA accessibility attributes are allowed on all SVG elements

So this issue is actually bigger than just role.

@murata2makoto
Copy link
Contributor

So, should we allow role and aria-* for all SVG elements?

Here is a list of all aria-* attributes copied from http://www.w3.org/MarkUp/DTD/aria-attributes-1.mod

aria-busy
aria-checked
aria-disabled
aria-dropeffect
aria-expanded
aria-grabbed
aria-hidden
aria-invalid
aria-pressed
aria-selected
aria-activedescendant
aria-atomic
aria-autocomplete
aria-controls
aria-describedby
aria-flowto
aria-haspopup
aria-label
aria-labelledby
aria-level
aria-live
aria-multiline
aria-multiselectable
aria-owns
aria-posinset
aria-readonly
aria-relevant
aria-required
aria-setsize
aria-sort
aria-valuemax
aria-valuemin
aria-valuenow
aria-valuetext

@mattgarrish
Copy link
Member

We should probably follow what SVG2 says, since that's what they're emulating.

I checked the w3c validator and it doesn't truly allow any attribute on any element. It looks like they follow the guidance of only allowing on renderable content, which makes sense since the user won't be interacting with non-renderable content (it throws errors on title, desc, style, etc.).

The list of attributes looks correct, but I haven't tried to match up each against the list in SVG2: https://www.w3.org/TR/SVG2/struct.html#WAIARIA-definitions

@murata2makoto
Copy link
Contributor

The list of attributes looks correct, but I haven't tried to match up each against the list in SVG2: https://www.w3.org/TR/SVG2/struct.html#WAIARIA-definitions

aria-current and aria-details (and several attributes) are added in the SVG2 list mentioned above.
But these attributes are not allowed in html5-aria-30.rnc of epubcheck.

Should we allow some new attributes only in SVG content documents, while disallowing them in HTML and embedded SVG?

@mattgarrish
Copy link
Member

No, we should probably omit from all. They're part of ARIA 1.1, which isn't a REC yet, so always a chance they could be dropped.

@murata2makoto
Copy link
Contributor

aria-describedat and aria-orientation are in html5-aria-30.rnc, but not in ARIA 1.0. Is this a mistake?

@mattgarrish
Copy link
Member

aria-describedat is dead and buried now, but it was prematurely referenced from 3.0.1. Our lesson in not taking anyone's word on what will make a final specification. I think there's a warning generated somewhere else because it's technically "deprecated".[1] @rdeltour ? (It's removed from 3.1.)

aria-orientation is in 1.0.[2] It can remain.

[1] http://www.idpf.org/epub/301/spec/epub-contentdocs.html#sec-xhtml-aria-describedat
[2] https://www.w3.org/TR/wai-aria/states_and_properties#aria-orientation

@murata2makoto
Copy link
Contributor

OK. We now know which ARIA attribute should be allowed. It remains to determine which SVG element can have such ARIA attributes. Some renderable elements of SVG2, audio, canvas, iframe, mesh, unknown, and video, do not exist in SVG 1.1. So, we have

  • a
  • circle
  • ellipse
  • foreignObject
  • g
  • image
  • line
  • path
  • polygon
  • polyline
  • rect
  • svg
  • switch
  • text
  • textPath
  • tspan
  • use

I will create a schema fix and submit a pull request.

@mattgarrish
Copy link
Member

Yes, that was one of the stranger spec requirements I've seen. Thanks.

@murata2makoto
Copy link
Contributor

I agree that the role attribute should be allowed only for renderable elements. But how about other aria-* attributes? The last para of 5.13.3. State and property attributes (all aria- attributes)

WAI-ARIA State and Property attributes can be used on any element.

@mattgarrish
Copy link
Member

Right, but that's followed by:

They are not always meaningful, however, and in such cases user agents might not perform any processing aside from including them in the DOM.

It looks like w3c did a thorough job of determining which attributes are meaningful on which elements for the validator. The commit is here:

validator/validator@3530d33

We're heading towards the permissive mess they've cleaned up.

Are we still expecting to align with those schemas? It would be better if we were only ever adjusting for epub peculiarities rather than having to patch core issues in parallel.

@murata2makoto
Copy link
Contributor

I can agree to mimick what Validator.Nu is doing. But I do not want to copy their schemas without incorporating their Java validation codes, which are intended to complement their schemas. In other words, either whole-sale reengineering based on Validator.Nu or parallel patching is good to me.

@tofi86
Copy link
Collaborator

tofi86 commented Jul 16, 2017

During review of PR #780 which adds aria support for SVG elements, I tried to add a unit test with the above example by @rdeltour / @clapierre:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="7.421ex" height="2.806ex" viewBox="0 -845 3195 1208.3" role="img" focusable="false">

While the patch by @murata0204 works well for the role attribute, I still get an Error for the focusable attribute:

ERROR(RSC-005): ./src/test/resources/30/single/svg/valid/issue769.xhtml(10,94): Error while parsing file: attribute "focusable" not allowed here; expected attribute "alignment-baseline", "aria-activedescendant", "aria-atomic", "aria-autocomplete", "aria-busy", "aria-checked", "aria-controls", "aria-describedby", "aria-disabled", "aria-dropeffect", "aria-expanded", "aria-flowto", "aria-grabbed", "aria-haspopup", "aria-hidden", "aria-invalid", "aria-label", "aria-labelledby", "aria-level", "aria-live", "aria-multiline", "aria-multiselectable", "aria-orientation", "aria-owns", "aria-posinset", "aria-pressed", "aria-readonly", "aria-relevant", "aria-required", "aria-selected", "aria-setsize", "aria-sort", "aria-valuemax", "aria-valuemin", "aria-valuenow", "aria-valuetext", "baseProfile", "baseline-shift", "class", "clip", "clip-path", "clip-rule", "color", "color-interpolation", "color-interpolation-filters", "color-profile", "color-rendering", "contentScriptType", "contentStyleType", "cursor", "direction", "display", "dominant-baseline", "enable-background", "externalResourcesRequired", "fill", "fill-opacity", "fill-rule", "filter", "flood-color", "flood-opacity", "font-family", "font-size", "font-size-adjust", "font-stretch", "font-style", "font-variant", "font-weight", "glyph-orientation-horizontal", "glyph-orientation-vertical", "id", "image-rendering", "kerning", "letter-spacing", "lighting-color", "marker-end", "marker-mid", "marker-start", "mask", "ns:type", "onabort", "onactivate", "onclick", "onerror", "onfocusin", "onfocusout", "onload", "onmousedown", "onmousemove", "onmouseout", "onmouseover", "onmouseup", "onresize", "onscroll", "onunload", "onzoom", "opacity", "overflow", "pointer-events", "preserveAspectRatio", "requiredExtensions", "requiredFeatures", "shape-rendering", "stop-color", "stop-opacity", "stroke", "stroke-dasharray", "stroke-dashoffset", "stroke-linecap", "stroke-linejoin", "stroke-miterlimit", "stroke-opacity", "stroke-width", "style", "systemLanguage", "text-anchor", "text-decoration", "text-rendering", "unicode-bidi", "version", "visibility", "word-spacing", "writing-mode", "x", "xml:base", "xml:lang", "xml:space", "y" or "zoomAndPan" (with xmlns:ns="http://www.idpf.org/2007/ops")

Is this attribute supposed to throw an error or is this reported by mistake/bug?

@mattgarrish
Copy link
Member

The focusable attribute is only part of SVG 1.2 Tiny, I believe. In SVG 2 they recommend setting tabindex=0.

The tabindex attribute is allowed on HTML5 elements per that weird SVG implementation section.[1] I haven't looked at the EPUB schema in any detail, but I imagine all those exclusions/additions need to be factored in, not just the ARIA attributes.

[1] https://www.w3.org/TR/html51/semantics-embedded-content.html#svg

@tofi86
Copy link
Collaborator

tofi86 commented Jul 16, 2017

Thanks Matt! So I'll leave it out from the test.

tofi86 added a commit to Advanced-Publishing-Laboratory/epubcheck that referenced this issue Jul 16, 2017
@tofi86 tofi86 closed this as completed in 49412e0 Jul 16, 2017
@tofi86 tofi86 added this to the 4.1.0 milestone Jul 16, 2017
@tofi86 tofi86 self-assigned this Jul 16, 2017
@tofi86 tofi86 added the status: has PR The issue is being processed in a pull request label Jul 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: has PR The issue is being processed in a pull request type: bug The issue describes a bug
Projects
None yet
Development

No branches or pull requests

4 participants