Bug 156519 – AX: Presentational role on SVG elements is trumped by child 'title' and 'desc' elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

There is an SVG Task Force resolution stating that:

  Where an element has role=none or role=presentation and has a
  child title or desc element, user agents MUST ignore the role.

See https://lists.w3.org/Archives/Public/public-svg-a11y/2016Apr/0016.html

(Patch shall be forthcoming.)

[Radar WebKit Bug Importer]

<rdar://problem/25689314>

[Joanmarie Diggs]

Created attachment 276280 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 276280 [details]
Patch

Chris: When you have a spare moment, please review. Thanks!

[chris fleizach]

Comment on attachment 276280 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:73
> +SVGDescElement* AccessibilitySVGElement::descElementWithLanguage(String language) const

what does this have to do with language?

should probably be called descriptionElementWith...

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:88
> +SVGTitleElement* AccessibilitySVGElement::titleElementWithLanguage(String language) const

String& language

[Joanmarie Diggs]

Comment on attachment 276280 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:73
>> +SVGDescElement* AccessibilitySVGElement::descElementWithLanguage(String language) const
> 
> what does this have to do with language?
> 
> should probably be called descriptionElementWith...

Ok re 'description' versus 'desc'.

As for what it has to do with language: In SVG2, you can have multiple 'title' or 'desc' element children with 'lang' attributes optionally specified. At the moment, the SVG AAM (and associated spec-finalizing discussions) have the following requirements:

1. If there's any 'title' or 'desc' child: It trumps the presentational role (i.e. the change in this patch).

2. When performing the alternative text computation, if there are multiple 'title'/'desc' to choose from, choose the one whose language matches. (This is existing behavior. But I wanted a helper function.)

Maybe 'languageAwareDescriptionElement()'??

[chris fleizach]

Comment on attachment 276280 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:73
>>> +SVGDescElement* AccessibilitySVGElement::descElementWithLanguage(String language) const
>> 
>> what does this have to do with language?
>> 
>> should probably be called descriptionElementWith...
> 
> Ok re 'description' versus 'desc'.
> 
> As for what it has to do with language: In SVG2, you can have multiple 'title' or 'desc' element children with 'lang' attributes optionally specified. At the moment, the SVG AAM (and associated spec-finalizing discussions) have the following requirements:
> 
> 1. If there's any 'title' or 'desc' child: It trumps the presentational role (i.e. the change in this patch).
> 
> 2. When performing the alternative text computation, if there are multiple 'title'/'desc' to choose from, choose the one whose language matches. (This is existing behavior. But I wanted a helper function.)
> 
> Maybe 'languageAwareDescriptionElement()'??

can you add comments to that effect in the code

i would start the method name with descriptionElement... maybe
descriptionElementWithLanguage() is OK with the comments explaining whats going on

[Joanmarie Diggs]

Created attachment 276321 [details]
Patch

[chris fleizach]

Comment on attachment 276321 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:80
> +    String childLanguageCode;

doesn't seem necessary to create this outside the loop

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:100
> +    String childLanguageCode;

ditto

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:140
> +        languageCode = defaultLanguage().substring(0, 2);

do we check anywhere that the language is at least 2 characters long?

> Source/WebCore/accessibility/AccessibilitySVGElement.h:52
> +    virtual AccessibilityRole determineAriaRoleAttribute() const;

can this use "override" keyword

[Joanmarie Diggs]

Comment on attachment 276321 [details]
Patch

(In reply to comment #8)
 
> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:140
> > +        languageCode = defaultLanguage().substring(0, 2);
> do we check anywhere that the language is at least 2 characters long?

substring() does do sanity checking regarding length.

And I was assuming that defaultLanguage() would have done all the work to get the platform user agent language, ensure it's valid, BCP-47ed (if appropriate), etc. 
Otherwise, all callers would have to do so. In which case, it would be at least 2 characters long. But making assumptions is, admittedly, stupid. I'll look into it.

On a related note: It turns out that we're doing absolutely no sanity checking whatsoever in AccessibilityObject::language(). Perhaps we should move defaultLanguage() as the final fallback of language() and add sanity checking (and BCP-47izing??) there....

I'll dig some more and then propose a new patch. Thanks!

(Clearing review flag)

[Joanmarie Diggs]

(In reply to comment #9)
> I'll dig some more

Talking mostly to myself: It seems that sanity-checking/validating the value of the 'lang' attribute is something user agents should not do. https://www.w3.org/TR/html5/dom.html#the-lang-and-xml:lang-attributes

[Joanmarie Diggs]

Created attachment 276449 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 276449 [details]
Patch

Chris: What do you think about this approach?

(My port's EWS orange state is due to a recent commit breaking the build; not this patch.)

[chris fleizach]

Comment on attachment 276449 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:92
> +        String lang = child.getAttribute(SVGNames::langAttr);

can we use getFastAttribute()

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:103
> +    size_t index = indexOfBestMatchingLanguageInList(languageCode, childLanguageCodes, exactMatch);

should exactMatch be &exactMatch

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:104
> +    if (exactMatch || index < childLanguageCodes.size())

do we need to check exactMatch? would index < size() if it is an exact match as well?

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:135
> +    auto titles = childrenOfType<SVGTitleElement>(*element());

titleElements instead of "titles"

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:136
> +    if (Element* childTitle = childElementWithMatchingLanguage(titles))

auto titleChild

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:183
> +    if (Element* childDescription = childElementWithMatchingLanguage(descriptions))

auto descriptionChild

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:202
> +    if (Element* childTitle = childElementWithMatchingLanguage(titles)) {

ditto

[Joanmarie Diggs]

Created attachment 276474 [details]
patch for landing

[Joanmarie Diggs]

Comment on attachment 276449 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:92
>> +        String lang = child.getAttribute(SVGNames::langAttr);
> 
> can we use getFastAttribute()

Done.

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:103
>> +    size_t index = indexOfBestMatchingLanguageInList(languageCode, childLanguageCodes, exactMatch);
> 
> should exactMatch be &exactMatch

I don't believe so.

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:104
>> +    if (exactMatch || index < childLanguageCodes.size())
> 
> do we need to check exactMatch? would index < size() if it is an exact match as well?

We don't need to check exactMatch. Removed.

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:135
>> +    auto titles = childrenOfType<SVGTitleElement>(*element());
> 
> titleElements instead of "titles"

Done. Also did equivalent for "descriptions".

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:183
>> +    if (Element* childDescription = childElementWithMatchingLanguage(descriptions))
> 
> auto descriptionChild

Done.

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:202
>> +    if (Element* childTitle = childElementWithMatchingLanguage(titles)) {
> 
> ditto

Done.

[WebKit Commit Bot]

Comment on attachment 276474 [details]
patch for landing

Clearing flags on attachment: 276474

Committed r199588: <http://trac.webkit.org/changeset/199588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.