Closed
Bug 868789
Opened 12 years ago
Closed 11 years ago
Name computation for SVG is wrong
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: takenspc, Assigned: takenspc)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
(deleted),
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter 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
Assignee | ||
Updated•12 years ago
|
Attachment #745601 -
Flags: review?(trev.saunders)
Comment 1•12 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.
Comment 2•12 years ago
|
||
In <svg> title children become tooltips, so the change to test_svg.html seems wrong.
Comment 3•12 years ago
|
||
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>.
Comment 4•12 years ago
|
||
(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•12 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.
Comment 6•12 years ago
|
||
(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•12 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.
Updated•12 years ago
|
Attachment #745601 -
Flags: review?(trev.saunders) → review-
Updated•12 years ago
|
Assignee | ||
Comment 8•12 years ago
|
||
Thanks all. I will try again at this weekend.
Updated•11 years ago
|
Blocks: 2013q4a11y
Comment 10•11 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•11 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•11 years ago
|
Attachment #745601 -
Flags: review- → review?(trev.saunders)
Comment 12•11 years ago
|
||
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•11 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•11 years ago
|
||
Comment 15•11 years ago
|
||
Assignee: nobody → taken.spc
Status: NEW → RESOLVED
Closed: 11 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.
Description
•