Name computation for SVG is wrong

RESOLVED FIXED in mozilla28

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Takeshi Kurosawa, Assigned: Takeshi Kurosawa)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 745601 [details] [diff] [review]
Patch

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
(Assignee)

Updated

5 years ago
Blocks: 459357
(Assignee)

Updated

5 years ago
Attachment #745601 - Flags: review?(trev.saunders)

Comment 1

5 years ago
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 :/

Comment 5

5 years ago
(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?

Comment 7

5 years ago
(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-

Updated

5 years ago
Blocks: 459353, 534873
(Assignee)

Comment 8

5 years ago
Thanks all. I will try again at this weekend.

Updated

4 years ago
Duplicate of this bug: 935935

Updated

4 years ago
Blocks: 923199

Comment 10

4 years ago
(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?

Comment 11

4 years ago
(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)

Updated

4 years ago
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+

Comment 13

4 years ago
(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.

Comment 14

4 years ago
relanding (fixing user name):

http://hg.mozilla.org/integration/mozilla-inbound/rev/38798122d6b4 
http://hg.mozilla.org/integration/mozilla-inbound/rev/85c5264ddc0b
https://hg.mozilla.org/mozilla-central/rev/85c5264ddc0b
Assignee: nobody → taken.spc
Status: NEW → RESOLVED
Last Resolved: 4 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.