The default bug view has changed. See this FAQ.

display the <svg:title> as a tooltip

RESOLVED FIXED in Firefox 3.7a2

Status

()

Firefox
General
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Takeshi Kurosawa, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 3.7a2
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 16 obsolete attachments)

1.41 KB, application/xml
Details
8.92 KB, patch
dao
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1

SVG element which have a <svg:title> should display it's child <svg:title> as a tooltip.

Reproducible: Always

Steps to Reproduce:
1.Hover the svg element
2.
3.

Actual Results:  
Nothing. 

Expected Results:  
Display the element's child <svg:title> as a tooltip.
(Reporter)

Comment 1

11 years ago
Created attachment 213879 [details] [diff] [review]
Patch rv1.0

Patch rv1.0. It's only support <svg:title>. To support both <svg:title> and <svg:desc> like batik, we need to modify structure of <xul:tooltip id="aHTMLTooltip">.
(Reporter)

Updated

11 years ago
(Reporter)

Comment 2

11 years ago
Created attachment 213882 [details]
Testcase

Testcase.

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

11 years ago
Attachment #213879 - Flags: review?(jwatt)
(Reporter)

Comment 3

11 years ago
Created attachment 213986 [details]
Testcase

Testcase.
Attachment #213882 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #213879 - Flags: review?(jwatt)
(Reporter)

Comment 4

11 years ago
Created attachment 213991 [details] [diff] [review]
Patch rv 1.1

Patch 1.1.

In this patch, previous testcase's first and second texts show the tooltip "This is a title".
Is it need to show <svg:desc> as tooltip when the element contains only <svg:desc>  ?

And first and second links show the tooltip "This is a title".
last link shows the tooltip "This is an xlink:title attribute".
Which is better prioritizing <svg:title> over xlink:title and prioritizing xlink:title over <svg:title>?
Attachment #213879 - Attachment is obsolete: true
There are a couple of things that jump out. First we want to go up the parent chain checking each ancestor for a direct <title> child if the current element doesn't have one. Second, I don't know if we want the SVG <title> element to override xlink:title or not. A bonus third: the code in that function isn't great anyway, so it probably needs a bit of a rewrite if your patch isn't to make it worse.
(Reporter)

Comment 6

11 years ago
Created attachment 215767 [details] [diff] [review]
Patch rv 1.2

Sorry for long delays. This is a new patch using XPath.
It adds two functions(not global).

It goes up the parent chain checking. Because of that, SVG which root <svg> has <svg:title> shows the tooltip anytime. Is it better behavior ?
# I think it is better.

In also this patch <svg:title> overrides xlink:title.
Attachment #213986 - Attachment is obsolete: true
Attachment #213991 - Attachment is obsolete: true
(Reporter)

Comment 7

11 years ago
Created attachment 215768 [details]
Testcase

Test case.
Its root <svg:svg> has <svg:title> as its child.
(Reporter)

Comment 8

11 years ago
Created attachment 216515 [details] [diff] [review]
Patch rv 1.3

Patch rv 1.3

fix some variables and a function's name.
minor fix of Namespace Resolver for XPath(NSResolver).
Attachment #215767 - Attachment is obsolete: true
Attachment #216515 - Flags: review?(jwatt)
Comment on attachment 216515 [details] [diff] [review]
Patch rv 1.3

Taken, I'm sorry, I completely missed this review request. My appologies for the long delay.

>       titleText = tipElement.getAttribute("title");
>       XLinkTitleText = tipElement.getAttributeNS(XLinkNS, "title");
>+      SVGTitleText = getSVGTitle(tipElement);

This would mean we'd do a *lot* more work than we currently do, even for HTML that has no SVG in it whatsoever. getSVGTitle is a *much* more expensive call than a simple getAttribute call.

FillInHTMLTooltip really needs a complete rewrite, even down to changing its name to something more accurate such as FillInTooltip.

What I'd do is get rid of XLinkTitleText, rename your getSVGTitle to simply getTitle and then in the while loop just assign |titleText = getTitle(tipElement)|. In getTitle you should fork based on the namespace of the element, and even the |t.search(/\S/)| code should be moved into this function.

Please also note the comment above FillInHTMLTooltip. You seem to have missed that.
Attachment #216515 - Flags: review?(jwatt) → review-
(Reporter)

Comment 10

11 years ago
(In reply to comment #9)
> Please also note the comment above FillInHTMLTooltip. You seem to have missed
> that.
> 

I am sorry but i can't write C++.
If once this bug fixes with JavaScript side, I'll file bug for embedded clients.
# Perphaps it is DefaultTooltipTextProvider::GetNodeText.
Okay, if you get the JavaScript side of it, I can help out with the C++. Probably best that both pieces happen in the same bug and get check in together though.
(Reporter)

Comment 12

11 years ago
Created attachment 220007 [details] [diff] [review]
Patch rv 1.4

Patch 1.4.

Rename FillInHTMLTooltip to FillInTooltip. Is it need to rename <xul:tooltip id="aHTMLTooltip"> ?
# Either, some extensions will be affected.

There is a minor difference browser/base/content/browser.js's and xpfe/browser/resources/content/browser.js's. So it is usage const kXULNS in browser/'s

local functions rename to start by _.
Attachment #216515 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #220007 - Flags: review?(jwatt)
Comment on attachment 220007 [details] [diff] [review]
Patch rv 1.4

The patch needs to check the type of aElement. For example, we don't want to use the 'title' attribute of aElement if it's not an HTML element.

>+    t = aElement.getAttribute('title');

Wrap that call using:

  if (aElement instanceof HTMLElement) {
    t = aElement.getAttribute('title');
  }

and change the |if| statement below to an |else| statement with a { } block that wraps everything down to just below the |t.search| line below.

>+    if (!t)
>+      t = aElement.getAttributeNS(XLinkNS, 'title');
>+
>+    if (!t) {

Make this |if (!t && aElement instance of SVGElement)|. We should only  be looking for a <title> element for SVG elements.

>+      function _NSResolver(aPrefix)
>+      {
>+        switch (aPrefix) {
>+          case "svg":
>+            return "http://www.w3.org/2000/svg";
>+          default:
>+            return null;
>+        }

I'm not sure about using XPath, but assuming we do, I'd just make this function body:

  if (aPrefix == 'svg')
    return 'http://www.w3.org/2000/svg';
  return null;

It's up to you though.
Attachment #220007 - Flags: review?(jwatt)

Updated

11 years ago
Assignee: general → taken.spc
As mentioned on IRC, we don't want to use the <svg:title> attribute if it's a child of the root element because we put that title in the title bar. So make that check:

  if (!t && aElement instanceof SVGElement &&
      !(aElement.parentNode instanceof Document))

or something similar.
(Reporter)

Comment 15

11 years ago
Created attachment 221883 [details] [diff] [review]
Patch rv1.5

Patch rv1.5.
# sorry for the delay

In this patch, I revert the function name to |FillInHTMLTooltip|. IMHO, when it move into XBL binding, we would change its name.
# The change may affect many extensions, but the bug is so big like that ?

<svg:a> isn't recognized as SVGElement. But I think it's a small issue. Because xlink:title attr is shown if it has.

The tooltip for SVG is needed only SVG enabled build, so |ifdef MOZ_SVG| is needed ?
Attachment #220007 - Attachment is obsolete: true
Attachment #221883 - Flags: review?(jwatt)
Comment on attachment 221883 [details] [diff] [review]
Patch rv1.5

It'd prefer the XLinkNS and SVGNS variables to be made global alongside kXULNS.

Also, come to think of it I was wrong to suggest putting the |t.search(/\S/)| line in _getTitle since that will slow down iteration. Please move it back out.

Apart from that, this looks good from a behaviour point of view. I'm not sure I really like the cost implications for each mouseover that occurs though. Ugh. But I don't see a way around that ATM.

So, to the crux. I'm not a peer for this code, so my review only confirms that this should provide the behavior we want for SVG. Because of that I'm marking my review using the addl-review field. You will require review and superreview from module owners before checking this in.
Attachment #221883 - Flags: review?(jwatt) → review+
Comment on attachment 221883 [details] [diff] [review]
Patch rv1.5

Bah. Apparently using the addl-review field will transfer the review to the normal review field if it isn't already set. :/ I'm removing that until you get module owner/peer review so that no one gets confused, but my review still stands.
Attachment #221883 - Flags: review+
(Reporter)

Comment 18

11 years ago
Created attachment 222032 [details] [diff] [review]
Patch rv1.6

Patch rv1.6.

Back out |t.search(/\S/)|, add #ifedf,  XLinkNS and SVGNS to global.
Attachment #221883 - Attachment is obsolete: true
Attachment #222032 - Flags: review?(gavin.sharp)
(Reporter)

Comment 19

11 years ago
(In reply to comment #18)
> Created an attachment (id=222032) [edit]
> Patch rv1.6

gavin, could you review my humble patch ?
Comment on attachment 222032 [details] [diff] [review]
Patch rv1.6

Unfortunately not. I'm not very familiar with this code, and am not a browser peer. I suggest mconnor or Ben. You may want to email them directly or ask them on IRC.
Attachment #222032 - Flags: review?(gavin.sharp)
I tried asking the SVG group what exactly should be considered the title, but I didn't get a reply. In case anyone was wondering. So I guess we don't have to worry about things like <html:noembed> or <svg:switch> in this context.
(Reporter)

Comment 22

11 years ago
Created attachment 222106 [details] [diff] [review]
Patch rv1.6.1

Patch rv1.6.1.

minor changes.
# |#ifdef| for |kSVGNS|, |k*NS| to |*NS| on xpfe/

Ben, could you review my humble patch ?
Attachment #222032 - Attachment is obsolete: true
Attachment #222106 - Flags: review?(bugs)
(Reporter)

Updated

11 years ago
Attachment #222106 - Attachment is obsolete: true
Attachment #222106 - Flags: review?(bugs)

Comment 23

10 years ago
jwatt could you please describe the intention of the patch for this bug in simple English?

title, desc and xlink:title are at least three possible alt contents that might be displayed in a tooltip.
is there a priority?
would the others be displayed in status bar?
where lese might they be displayed**?

http://www.w3.org/TR/SVG-access/#Links
"Unless SVG User Agents make this textual information available, authors will need to include text-based links to content as well (refer to [WCAG10] checkpoint 1.5, although this should not apply to newer user agents such as those designed for SVG)."

Comment 24

10 years ago
Additionally, does this bug cover the cases:

tooltips are not raised onfocus?

the tooltip is currently partially hidden by the pointer cursor?

Updated

9 years ago
Blocks: 410965

Comment 25

9 years ago
parity Opera
OS: Linux → All
Hardware: PC → All

Comment 26

9 years ago
Can someone please exlain the status of this patch/bug?  Can it make Firefox 3.1?  Thanks.

Comment 27

8 years ago
I have today received a request from an informed member of the public as to why this is not functioning. ie this person is a botanical professor, but not expert in SVG.
(In reply to comment #27)
> I have today received a request from an informed member of the public as to why
> this is not functioning.

I make this question myself every few months. If every time I'd insert a rant here... ;-) BTW: have you already voted for this bug? Gathering a higher number of votes could help pushing this into the high priority stack.

Marking as "wanted-1.9.1?" to call for attention, specially considering this would boost both usability and accessibility with minor/mostly safe code changes. I've reviewed the code and it generally looked good, but unfortunately I'm not familiar enough with Mozzila's code base to give stronger/more informed feedback.
Flags: wanted1.9.1?
jwatt, you are a peer for this code now, so maybe you could push it along :-)
Flags: wanted1.9.1? → wanted1.9.1+
I can push it along after we get through the blockers, but what code have I newly become a peer of? My name isn't showing up under anything new on www.mozilla.org/owners.html
"Peer" includes "owner". In comment #16 you weren't an owner or peer, now you are :-).
The code I was referring to not being a peer of in comment 16 is the browser code since that's what the patch toughes. That's not changed. Anyway, I'm sure I can help move this along regardless once I get to it. ;-)
Oops indeed.
QA Contact: ian → general
(Reporter)

Comment 34

8 years ago
Created attachment 403501 [details] [diff] [review]
Patch for Firefox

Sorry for so long delay.
Patch for browser/
Attachment #403501 - Flags: review?(dao)

Comment 35

8 years ago
+      if (titleElement && titleElement instanceof SVGElement)
+        SVGTitleText = titleElement.textContent;


Shouldn't this be 

+      if (SVGTitleText == null && titleElement && titleElement instanceof SVGElement)
+        SVGTitleText = titleElement.textContent;


Otherwise, you are overwriting the SVGTitleText with ancestor titles...

Comment 36

8 years ago
Ah, never mind, I missed this:

+  while (!titleText && !XLinkTitleText && !SVGTitleText && tipElement) {
Comment on attachment 403501 [details] [diff] [review]
Patch for Firefox

>+      var titleElement = tipElement.querySelector("title:not(:empty)");
>+      if (titleElement && titleElement instanceof SVGElement)
>+        SVGTitleText = titleElement.textContent;

nit: use let instead of var

Any reason why you're not testing if titleElement instanceof SVGElement before doing querySelector?

Updated

8 years ago
Component: SVG → General
Flags: wanted1.9.1+
Product: Core → Firefox
QA Contact: general → general
(Reporter)

Comment 38

8 years ago
Created attachment 404565 [details] [diff] [review]
Patch for Firefox v2

Find svg:title only if tipElement is an SVGElement.


(In reply to comment #37)
> (From update of attachment 403501 [details] [diff] [review])
> >+      var titleElement = tipElement.querySelector("title:not(:empty)");
> >+      if (titleElement && titleElement instanceof SVGElement)
> >+        SVGTitleText = titleElement.textContent;
> 
> nit: use let instead of var


Fixed.


> Any reason why you're not testing if titleElement instanceof SVGElement before
> doing querySelector?


querySelector doesn't support resolving namespaces, so |titleElement| can be <html:title> or <foo:title>. We must manually check |titleElment| is an |SVGTitleElement|.
Attachment #403501 - Attachment is obsolete: true
Attachment #404565 - Flags: review?(dao)
Attachment #403501 - Flags: review?(dao)
(In reply to comment #38)
> > Any reason why you're not testing if titleElement instanceof SVGElement before
> > doing querySelector?
> 
> 
> querySelector doesn't support resolving namespaces, so |titleElement| can be
> <html:title> or <foo:title>. We must manually check |titleElment| is an
> |SVGTitleElement|.

Right, I didn't mean to drop the second check.

Updated

8 years ago
Attachment #404565 - Flags: review?(dao) → review+
Comment on attachment 404565 [details] [diff] [review]
Patch for Firefox v2

>+        // XXX Since querySelector() doesn't support namespaces we manually
>+        // check if titleElement is an SVGTitleElement 

Since this isn't a bug, remove XXX. Also remove the trailing space.
(Assignee)

Comment 41

7 years ago
Created attachment 421968 [details] [diff] [review]
update to tip and address comment 40
Attachment #404565 - Attachment is obsolete: true
(Assignee)

Comment 42

7 years ago
Created attachment 421971 [details] [diff] [review]
part 2 (see comment 10)
(Assignee)

Comment 43

7 years ago
Comment on attachment 421971 [details] [diff] [review]
part 2 (see comment 10)

This patch is just the embedding version of the browser.js patch
Attachment #421971 - Flags: review?(benjamin)
Attachment #421971 - Flags: review?(benjamin) → review?(bzbarsky)
Comment on attachment 421971 [details] [diff] [review]
part 2 (see comment 10)

This doesn't look like it'll compile in non-libxul builds, unless I'm missing something...
Oh, and has someone filed a followup but on making this do the right thing once querySelector has namespace support?  I'd prefer some FIXME comments in both patches with a bug#.
(Assignee)

Comment 46

7 years ago
Comment on attachment 421971 [details] [diff] [review]
part 2 (see comment 10)

Need to address comment 44 in some way.
Attachment #421971 - Flags: review?(bzbarsky)
> Need to address comment 44 in some way.

You could, say, use DOM apis instead of nsContentUtils.
(Assignee)

Updated

7 years ago
Depends on: 541672
(Assignee)

Comment 48

7 years ago
Created attachment 423149 [details] [diff] [review]
part 1 - address review comments
Attachment #421968 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #423149 - Attachment description: address review comments → part 1 - address review comments
(Assignee)

Comment 49

7 years ago
Created attachment 423150 [details] [diff] [review]
part 2 - address review comments
Attachment #421971 - Attachment is obsolete: true
Attachment #423150 - Flags: review?(bzbarsky)
Comment on attachment 423150 [details] [diff] [review]
part 2 - address review comments

r=bzbarsky
Attachment #423150 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 51

7 years ago
Created attachment 424397 [details] [diff] [review]
with tests

When writing tests for this I discovered the javascript code that I'd based the C++ code on wasn't logically right.

<svg>
  <text><title>1</title></title>
  <text id="id2"></text>
</svg>

given the markup above, when looking for a title for id2, the previous algorithm would check out the id2 text node and find nothing, then it would check out the svg node and the queryselect would find that it had a grandchild with a title and use that which is not what's wanted.
Assignee: taken.spc → longsonr
Attachment #423149 - Attachment is obsolete: true
Attachment #423150 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #424397 - Flags: review?(dao)
(In reply to comment #51)
> When writing tests for this I discovered the javascript code that I'd based the
> C++ code on wasn't logically right.
> 
> <svg>
>   <text><title>1</title></title>
>   <text id="id2"></text>
> </svg>
> 
> given the markup above, when looking for a title for id2, the previous
> algorithm would check out the id2 text node and find nothing, then it would
> check out the svg node

I don't see where it would "check out the svg node".
(Assignee)

Comment 53

7 years ago
You start from id2 =tipElement and find nothing. Then you go to the parent (tipElement = tipElement.parentNode) which is the svg node. A querySelect on the SVG node finds the first title element grandchild in the above markup.
Comment on attachment 424397 [details] [diff] [review]
with tests

Ah, so the issue is that we can't express the child relationship with querySelector.
Attachment #424397 - Flags: review?(dao) → review+
(Assignee)

Updated

7 years ago
Attachment #424397 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

7 years ago
No longer depends on: 541672
Attachment #424397 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 55

7 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/2e9d8868efc6
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → Firefox 3.7a2
The changes seem rather safe so I'd ask if this could be backported to the 1.9.2 branch...?

(Didn't see any applicable flag for marking "wanted1.9.2.x"... Am I missing something?)
(Assignee)

Comment 57

7 years ago
The patch should be able to land as-is, the question is whether it is worth the risk as normally branches take stability and/or security fixes and this isn't.

Have you tested it yourself? Does it do what you expect on your pages?

Bearing all that in mind... If you want to ask for approval for 1.9.2 you would go to details on the patch and set the approval 1.9.2.<x> to ?
(In reply to comment #57)
> The patch should be able to land as-is, the question is whether it is worth the
> risk as normally branches take stability and/or security fixes and this isn't.

Yes, I'm aware of that. I'd just like to see this simple-yet-very-useful small feature with apparently zero risk to users in a near future, given the Firefox.next schedule is still somehow uncertain after the drop of 3.7 in favor of 4.0 [1] [2]. ;-)


> Have you tested it yourself? Does it do what you expect on your pages?

Sorry for missing that in my previous comment. Yes, I've tested it and, I'd vote for marking this issue "verified".


> If you want to ask for approval for 1.9.2 you would
> go to details on the patch and set the approval 1.9.2.<x> to ?

Just got a "You are not authorized to edit attachment xxxxxx." message... :-(


[1] https://wiki.mozilla.org/Firefox#Firefox_major_release_tracking
[2] http://en.wikipedia.org/wiki/Mozilla_Firefox#Version_3.7
> after the drop of 3.7

The mozilla wiki link is a proposal, not decided on.  The wikipedia article you linked to seems accrate and doesn't say anything about "drop".
(In reply to comment #59)
> The mozilla wiki link is a proposal, not decided on.  The wikipedia article you
> linked to seems accrate and doesn't say anything about "drop".

Right, sorry for the noise. I was only trying to suggest that it was a good idea to push this upstream, in a better-sooner-than-later fashion. I admit I should have posed it more wisely... :-)
Further testing showed that apparently the current behavior may require a bit of love... :-)

Specifically, I've noticed that the outermost title is being used every time a given element doesn't specify any. In the test case above, just leaving the mouse parked somewhere displays "This is a root SVG element's title" which doesn't feel expected behavior IMHO: AFAIK, the top-level title/desc should be handled specially -- it's intended to provide generic information about the document itself:
 * The outermost "title" element would (only) be used as window title, as already was in previous versions;
 * The outermost "desc" element would be used as document information, possibly in page info document properties dialog (maybe displayed like meta tags in HTML pages), something which was never supported.

Nevertheless, I'm not sure if this should be analyzed and (maybe) lead to reopening this issue or reporting a new issue, blocked by this one, to improve the currently available behavior (latter would better, IMO).

Note that there's an algorithm [1], proposed by the SVG IG, as implementation proposal.

[1] http://www.w3.org/Graphics/SVG/IG/wiki/Accessibility_Activity#Title_As_Tooltip
(Assignee)

Comment 62

7 years ago
Please raise a new bug for the outermost title element and cc me on it.
Blocks: 547854
(In reply to comment #62)
> Please raise a new bug for the outermost title element and cc me on it.

Done, just reported bug 547854. :-)
Added a note to the <title> description on https://developer.mozilla.org/En/SVG_in_Firefox
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 557166
(Assignee)

Updated

7 years ago
Duplicate of this bug: 561105

Updated

5 years ago
Depends on: 738233
Blocks: 601091
You need to log in before you can comment on or make changes to this bug.