Closed
Bug 329212
Opened 18 years ago
Closed 14 years ago
display the <svg:title> as a tooltip
Categories
(Firefox :: General, defect)
Firefox
General
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.
Reporter | ||
Comment 1•18 years ago
|
||
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•18 years ago
|
Reporter | ||
Comment 2•18 years ago
|
||
Testcase.
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•18 years ago
|
Attachment #213879 -
Flags: review?(jwatt)
Reporter | ||
Updated•18 years ago
|
Attachment #213879 -
Flags: review?(jwatt)
Reporter | ||
Comment 4•18 years ago
|
||
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
Comment 5•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
Test case. Its root <svg:svg> has <svg:title> as its child.
Reporter | ||
Comment 8•18 years ago
|
||
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 9•18 years ago
|
||
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•18 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.
Comment 11•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #220007 -
Flags: review?(jwatt)
Comment 13•18 years ago
|
||
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•18 years ago
|
Assignee: general → taken.spc
Comment 14•18 years ago
|
||
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•18 years ago
|
||
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 16•18 years ago
|
||
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 17•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
(In reply to comment #18) > Created an attachment (id=222032) [edit] > Patch rv1.6 gavin, could you review my humble patch ?
Comment 20•18 years ago
|
||
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)
Comment 21•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #222106 -
Attachment is obsolete: true
Attachment #222106 -
Flags: review?(bugs)
Comment 23•17 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•17 years ago
|
||
Additionally, does this bug cover the cases: tooltips are not raised onfocus? the tooltip is currently partially hidden by the pointer cursor?
Comment 26•16 years ago
|
||
Can someone please exlain the status of this patch/bug? Can it make Firefox 3.1? Thanks.
Comment 27•16 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.
Comment 28•16 years ago
|
||
(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+
Comment 30•16 years ago
|
||
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 :-).
Comment 32•16 years ago
|
||
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.
Updated•15 years ago
|
QA Contact: ian → general
Reporter | ||
Comment 34•15 years ago
|
||
Sorry for so long delay. Patch for browser/
Attachment #403501 -
Flags: review?(dao)
Comment 35•15 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•15 years ago
|
||
Ah, never mind, I missed this: + while (!titleText && !XLinkTitleText && !SVGTitleText && tipElement) {
Comment 37•15 years ago
|
||
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•15 years ago
|
Component: SVG → General
Flags: wanted1.9.1+
Product: Core → Firefox
QA Contact: general → general
Reporter | ||
Comment 38•15 years ago
|
||
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)
Comment 39•15 years ago
|
||
(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•15 years ago
|
Attachment #404565 -
Flags: review?(dao) → review+
Comment 40•15 years ago
|
||
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•14 years ago
|
||
Attachment #404565 -
Attachment is obsolete: true
Assignee | ||
Comment 42•14 years ago
|
||
Assignee | ||
Comment 43•14 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)
Updated•14 years ago
|
Attachment #421971 -
Flags: review?(benjamin) → review?(bzbarsky)
Comment 44•14 years ago
|
||
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...
Comment 45•14 years ago
|
||
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•14 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)
Comment 47•14 years ago
|
||
> Need to address comment 44 in some way.
You could, say, use DOM apis instead of nsContentUtils.
Assignee | ||
Comment 48•14 years ago
|
||
Attachment #421968 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #423149 -
Attachment description: address review comments → part 1 - address review comments
Assignee | ||
Comment 49•14 years ago
|
||
Attachment #421971 -
Attachment is obsolete: true
Attachment #423150 -
Flags: review?(bzbarsky)
Comment 50•14 years ago
|
||
Comment on attachment 423150 [details] [diff] [review] part 2 - address review comments r=bzbarsky
Attachment #423150 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 51•14 years ago
|
||
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•14 years ago
|
Attachment #424397 -
Flags: review?(dao)
Comment 52•14 years ago
|
||
(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•14 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 54•14 years ago
|
||
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•14 years ago
|
Attachment #424397 -
Flags: superreview?(bzbarsky)
Updated•14 years ago
|
Attachment #424397 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 55•14 years ago
|
||
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
Updated•14 years ago
|
Target Milestone: --- → Firefox 3.7a2
Comment 56•14 years ago
|
||
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•14 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 ?
Comment 58•14 years ago
|
||
(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
Comment 59•14 years ago
|
||
> 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".
Comment 60•14 years ago
|
||
(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... :-)
Comment 61•14 years ago
|
||
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•14 years ago
|
||
Please raise a new bug for the outermost title element and cc me on it.
Comment 63•14 years ago
|
||
(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. :-)
Comment 64•14 years ago
|
||
Added a note to the <title> description on https://developer.mozilla.org/En/SVG_in_Firefox
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•