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.)
<rdar://problem/25689314>
Created attachment 276280 [details] Patch
Comment on attachment 276280 [details] Patch Chris: When you have a spare moment, please review. Thanks!
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
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()'??
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
Created attachment 276321 [details] Patch
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
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)
(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
Created attachment 276449 [details] Patch
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.)
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
Created attachment 276474 [details] patch for landing
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.
Comment on attachment 276474 [details] patch for landing Clearing flags on attachment: 276474 Committed r199588: <http://trac.webkit.org/changeset/199588>
All reviewed patches have been landed. Closing bug.