Closed Bug 868789 Opened 7 years ago Closed 6 years ago

Name computation for SVG is wrong

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: takenspc, Assigned: takenspc)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
In bug 459357 comment 5

> The idea is to map a child element <title> to the accessible name, and the <desc> to accessible description.

At this time however title is mapped to the description and desc is mapped to the name.
The attached patch makes title mapped to the name and desc mapped to the description.

Document Structure – SVG 1.1 (Second Edition)
http://www.w3.org/TR/SVG11/struct.html#DescriptionAndTitleElements
Blocks: 459357
Attachment #745601 - Flags: review?(trev.saunders)
seems right, I don't recall why I mapped them in opposite order. Spec says (http://www.w3.org/TR/SVG-access/#Equivalent):

title
    Provides a human-readable title for the element that contains it. The title element may be rendered by a graphical user agent as a tooltip. It may be rendered as speech by a speech synthesizer.

desc
    Provides a longer more complete description of an element that contains it. Authors should provide descriptions for complex or other content that has functional meaning. 

Mick said the same in bug 534873 comment #3.
In <svg> title children become tooltips, so the change to test_svg.html seems wrong.
Comment on attachment 745601 [details] [diff] [review]
Patch

>diff --git a/accessible/src/generic/Accessible.cpp b/accessible/src/generic/Accessible.cpp
>--- a/accessible/src/generic/Accessible.cpp
>+++ b/accessible/src/generic/Accessible.cpp
>@@ -266,17 +266,17 @@ Accessible::Name(nsString& aName)
>       aName.CompressWhitespace();
>       return eNameFromTooltip;
>     }
>   } else if (mContent->IsSVG()) {
>     // If user agents need to choose among multiple ‘desc’ or ‘title’ elements
>     // for processing, the user agent shall choose the first one.
>     for (nsIContent* childElm = mContent->GetFirstChild(); childElm;
>          childElm = childElm->GetNextSibling()) {
>-      if (childElm->IsSVG(nsGkAtoms::title)) {
>+      if (childElm->IsSVG(nsGkAtoms::desc)) {
>         nsTextEquivUtils::AppendTextEquivFromContent(this, childElm, &aName);

it seems like you should leave this check as is so name always uses <title>.
(In reply to alexander :surkov from comment #1)
> seems right, I don't recall why I mapped them in opposite order. Spec says

yeah, I don't want to know what I was on when I reviewed that :/
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >@@ -266,17 +266,17 @@ Accessible::Name(nsString& aName)
> >       aName.CompressWhitespace();
> >       return eNameFromTooltip;
> >     }
> >   } else if (mContent->IsSVG()) {
> >     // If user agents need to choose among multiple ‘desc’ or ‘title’ elements
> >     // for processing, the user agent shall choose the first one.
> >     for (nsIContent* childElm = mContent->GetFirstChild(); childElm;
> >          childElm = childElm->GetNextSibling()) {
> >-      if (childElm->IsSVG(nsGkAtoms::title)) {
> >+      if (childElm->IsSVG(nsGkAtoms::desc)) {
> >         nsTextEquivUtils::AppendTextEquivFromContent(this, childElm, &aName);
> 
> it seems like you should leave this check as is so name always uses <title>.

it's a fallback, the name shouldn't be ever empty so if we failed to get a name from <title> then we should try <desc>. But this part of code is tooltip fallback so checking a desc here is incorrect.
(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > >@@ -266,17 +266,17 @@ Accessible::Name(nsString& aName)
> > >       aName.CompressWhitespace();
> > >       return eNameFromTooltip;
> > >     }
> > >   } else if (mContent->IsSVG()) {
> > >     // If user agents need to choose among multiple ‘desc’ or ‘title’ elements
> > >     // for processing, the user agent shall choose the first one.
> > >     for (nsIContent* childElm = mContent->GetFirstChild(); childElm;
> > >          childElm = childElm->GetNextSibling()) {
> > >-      if (childElm->IsSVG(nsGkAtoms::title)) {
> > >+      if (childElm->IsSVG(nsGkAtoms::desc)) {
> > >         nsTextEquivUtils::AppendTextEquivFromContent(this, childElm, &aName);
> > 
> > it seems like you should leave this check as is so name always uses <title>.
> 
> it's a fallback, the name shouldn't be ever empty so if we failed to get a
> name from <title> then we should try <desc>. But this part of code is
> tooltip fallback so checking a desc here is incorrect.

yeah, if we want to fall back to <desc> I think it would be better to generically fall back to Description() if Name() returns empty.  Should we just remove that code then?
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> yeah, if we want to fall back to <desc> I think it would be better to
> generically fall back to Description() if Name() returns empty.

not always, for example, aria-describedby is never mapped to name

>  Should we
> just remove that code then?

this part should be removed but desc should be added into NativeName() I think. But Accessible::Description() code should be revised as well because it's a mapping of Accessible::Name() tooltip part.
Attachment #745601 - Flags: review?(trev.saunders) → review-
Thanks all. I will try again at this weekend.
Duplicate of this bug: 935935
(In reply to Takeshi Kurosawa from comment #8)
> Thanks all. I will try again at this weekend.

Tekeshi, do you have plans on this bug?
(In reply to alexander :surkov from comment #7)

> >  Should we
> > just remove that code then?
> 
> this part should be removed but desc should be added into NativeName() I
> think. But Accessible::Description() code should be revised as well because
> it's a mapping of Accessible::Name() tooltip part.

you know the change would complicated the logic, proably it'd be easier if we closed eyes that desc is not tooltip (we could add a small comment)
Attachment #745601 - Flags: review- → review?(trev.saunders)
Comment on attachment 745601 [details] [diff] [review]
Patch

I feel like we've flipped back and forth on this before, but if is what you want to do I have no objection
Attachment #745601 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> I feel like we've flipped back and forth on this before, 

right, my point is as long as we have resources to continue the work we should do it, if we run into lack of resources (like assignee disappeared for a while) it makes sense to take what we have as long as it a good improvement overall.
https://hg.mozilla.org/mozilla-central/rev/85c5264ddc0b
Assignee: nobody → taken.spc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.