Closed Bug 329212 Opened 18 years ago Closed 14 years ago

display the <svg:title> as a tooltip

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a2

People

(Reporter: takenspc, Assigned: longsonr)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 16 obsolete files)

1.41 KB, application/xml
Details
8.92 KB, patch
dao
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
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.
Attached patch Patch rv1.0 (obsolete) — Splinter Review
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">.
Attached file Testcase (obsolete) —
Testcase.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #213879 - Flags: review?(jwatt)
Attached file Testcase (obsolete) —
Testcase.
Attachment #213882 - Attachment is obsolete: true
Attachment #213879 - Flags: review?(jwatt)
Attached patch Patch rv 1.1 (obsolete) — Splinter Review
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.
Attached patch Patch rv 1.2 (obsolete) — Splinter Review
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
Attached file Testcase
Test case.
Its root <svg:svg> has <svg:title> as its child.
Attached patch Patch rv 1.3 (obsolete) — Splinter Review
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-
(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.
Attached patch Patch rv 1.4 (obsolete) — Splinter Review
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
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)
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.
Attached patch Patch rv1.5 (obsolete) — Splinter Review
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+
Attached patch Patch rv1.6 (obsolete) — Splinter Review
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)
(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.
Attached patch Patch rv1.6.1 (obsolete) — Splinter Review
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)
Attachment #222106 - Attachment is obsolete: true
Attachment #222106 - Flags: review?(bugs)
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)."
Additionally, does this bug cover the cases:

tooltips are not raised onfocus?

the tooltip is currently partially hidden by the pointer cursor?

Blocks: 410965
parity Opera
OS: Linux → All
Hardware: PC → All
Can someone please exlain the status of this patch/bug?  Can it make Firefox 3.1?  Thanks.
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. ;-)
QA Contact: ian → general
Attached patch Patch for Firefox (obsolete) — Splinter Review
Sorry for so long delay.
Patch for browser/
Attachment #403501 - Flags: review?(dao)
+      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...
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?
Component: SVG → General
Flags: wanted1.9.1+
Product: Core → Firefox
QA Contact: general → general
Attached patch Patch for Firefox v2 (obsolete) — Splinter Review
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.
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.
Attachment #404565 - Attachment is obsolete: true
Attached patch part 2 (see comment 10) (obsolete) — Splinter Review
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#.
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.
Depends on: 541672
Attached patch part 1 - address review comments (obsolete) — Splinter Review
Attachment #421968 - Attachment is obsolete: true
Attachment #423149 - Attachment description: address review comments → part 1 - address review comments
Attached patch part 2 - address review comments (obsolete) — Splinter Review
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+
Attached patch with testsSplinter Review
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
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".
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+
Attachment #424397 - Flags: superreview?(bzbarsky)
No longer depends on: 541672
Attachment #424397 - Flags: superreview?(bzbarsky) → superreview+
pushed http://hg.mozilla.org/mozilla-central/rev/2e9d8868efc6
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
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?)
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
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
Blocks: 557166
Depends on: 738233
You need to log in before you can comment on or make changes to this bug.