Bug 16854 – display title in tooltip onmouseover in SVG

• Component: SVG
• Status: RESOLVED FIXED

[jay]

visit the URI and mouseover the icons.

tooltip with title content should appear.

parity Opera

[David Kilzer (:ddkilzer)]

Note that you must hover over the drawn part of the icon; hovering over the blue background inside of a cloud doesn't display a tooltip on Opera 9.24.

[jay]

parity mozilla

parity Opera
please note: provided the cursor is on the window, Opera now updates the tooltip content as
focus changes

[Mark Rowe (bdash)]

*** Bug 17472 has been marked as a duplicate of this bug. ***

[jay]

a quick read of XML Linking Language (XLink) Version 1.0
http://www.w3.org/TR/xlink/

suggests that it is unlikely that xlink only refers to anchors, unless that is a constraint of svg1.1.

eg An XML element conforms to XLink if:...
http://www.w3.org/TR/xlink/#markup-reqs

appears to suggest that any xml element may have an xlink
unless I am mis-reading...

[jay]

#4

please ignore, wrong bug...
I hate the way I am directed to a new bug on commiting....

[Jeff Schiller]

Ok, I have a patch for this.  Let me put together the story for tooltips in SVG:

  * if user hovers over a linked element that has a xlink:title, the xlink:title becomes the tooltip
  * if not linked, then the element under the cursor (nearest enclosing DOM node) is checked for a <title> element and that is used
  * if no title is found, then go up to the parent and look for a title, etc

This is all taken care of by WebKit's HitTestResult and adding a title() method to SVGStyledElement.

Special cases:

  * if element is a <use>, then the use's title should be the tooltip.  If the <use> does not have a title, then look into the rendered shadow content (referenced DOM) for the title as per above

I should note that for the case of a <symbol> being referenced by a <use>, the <symbol> itself is not checked for a <title> element.  This is because the symbol is not rendered (unlike a <g>).  To solve this in the weather symbol file, I had to move wrap the contents of the <symbol> elements in a <g> and the tooltips showed up.  I'll also attached the modified weather file.

The patch which implements all of this but does not get one case in my test file correct:

<defs>
  <ellipse id="e1">
    <title>FAIL</title>
  </ellipse>
</defs>

<use xlink:href="#e1">
  <title>PASS</title>
</use>

Interestingly, if the <use> references a <symbol> in the above case, the tooltip displays properly.  I'm still investigating this case but I think it should be raised as a different bug.

Can someone review the patch?  Can someone suggest how this can be tested?  Since it's driven by manual user interaction I'm not sure the best approach.

[Jeff Schiller]

Created attachment 53315 [details]
Patch to implement tooltip for SVG elements with a <title> child

[Jeff Schiller]

Created attachment 53316 [details]
Slightly modified test file from Wikipedia

[WebKit Review Bot]

Attachment 53315 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGAElement.cpp:67:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeff Schiller]

Created attachment 53317 [details]
More concise test file that tests a variety of cases.  The middle case does not work (will open a separate bug).

[Jeff Schiller]

Created attachment 53318 [details]
Fix style error

[Dirk Schulze]

(In reply to comment #11)
> Created an attachment (id=53318) [details]
> Fix style error

You can check for the renderer to identify a use-element

RenderObject* renderer = renderer();
if (renderer && renderer->isSVGShadowTreeRootContainer())

Also the correct style is to use sentences with capital at the beginning and a point at the end. Please use String() or string.empty() for strings, not nullAtom. This would be more understandable.

I would like to ask Niko for taking a look at this patch.

[Jeff Schiller]

Created attachment 53603 [details]
Patch properly walking up to find the <use> shadow parent.  Update for reviewer comments

[Nikolas Zimmermann]

Comment on attachment 53603 [details]
Patch properly walking up to find the <use> shadow parent.  Update for reviewer comments

Hi Jeff,

nice patch, though it needs to be tightend up a bit:
 
>  String SVGAElement::title() const
>  {
> -    return getAttribute(XLinkNames::titleAttr);
> +    // If the xlink:title is set (non-empty string), use it.
> +    const AtomicString& title = getAttribute(XLinkNames::titleAttr);
> +    if (title != String())
Please use "if (!title.isEmpty())" here, instead of creating temporary String objects for comparision.


> +String SVGStyledElement::title() const
> +{
> +    // First we walk up the tree to see if we are inside a <use> shadow tree.
> +    Node* parent = const_cast<SVGStyledElement*>(this);
> +    while (parent) {
> +        if (parent->isShadowNode()) {
I'd suggest to use following code, in order to make the code more readable, less nested scopes:

if (!parent->isShadowNode()) {
    parent = parent->parentNode();
    continue;
}

> +            // Get the <use> element.
> +            Node* shadowParent = parent->shadowParentNode();
> +            if (shadowParent && shadowParent->nodeName() == SVGNames::useTag) {
I'd recommend to use a slightly more optimized check here:
if (shadowParent && shadowParent->hasTagName(SVGNames::useTag))
...

> +                SVGUseElement* useElem = static_cast<SVGUseElement*>(shadowParent);
> +                if (useElem) {
Please don't use abbrevations, use "useElement", as identifier, and combine it with the if statement.
if (SVGUseElement* useElement = ....) { ... }

> +                    // If the <use> title is empty we will keep walking up.
> +                    String useTitle(useElem->title());

> +                    if (useTitle != String())
> +                        return useTitle;
Same as above, just use "if (!useTitle.isEmpty())" instead of direct comparisions.

> +    // If we aren't an instance in a use, then find the first <title> child of this element.
> +    Element* child = firstElementChild();
> +    while (child && child->nodeName() != "title" && child->namespaceURI() != "http://www.w3.org/2000/svg")
> +        child = child->nextElementSibling();
Why not use a simple for() loop here, like:

for (Element* child = firstElementChild(); child; child = child->nextElementSibling()) {
    if (!child->isSVGElement())
        continue;
    if (!child->hasTagName(SVGNames::titleAttr))
        continue;
}

In general you should always try to avoid temporary strings and comparisions.

> +    // If found, return the text contents.
> +    // According to spec, the title on <svg> elements are never returned as a tooltip.
> +    if (child && nodeName() != "svg")
> +        return child->innerText();
if (child && !child->hasTagName(SVGNames::svgTag))
    return child->innerText();

Sorry for the lots of corrections, but I think this makes the code much nicer to read :-)
r- for now, happy to review the next patch.

[Jeff Schiller]

Thanks for your comments!

I'm a little busy today but hopefully tonight/tomorrow.

[Jeff Schiller]

Created attachment 53753 [details]
Patch addressing Niko's comments

titleElement needs to be declared outside of the for-loop because of scoping rules in C++

[jay]

Jeff,

probably a separate bug, but maybe you can fix now...

can you please check that the tooltip raised is not directly under the cursor, for a variety of cursors and font sizes?

maybe even place the tooltip above the cursor?
maybe not...

ie Opera the cursor frequently covers part or most of the tooltip, very much reducing its efficacy.
reported years ago, but iirc not fixed

apologies for the bug-spam

[Jeff Schiller]

jay - this patch/fix has nothing to do with where the tooltip is placed or how it's rendered, sorry.  Sounds like a different bug.

[Nikolas Zimmermann]

Comment on attachment 53753 [details]
Patch addressing Niko's comments

Hi Jeff,

almost there, some small comments:

>  
> +String SVGStyledElement::title() const
> +{
> +    // Else, we walk up the tree to see if we are inside a <use> shadow tree.
Slightly adjust the comment to sth. like:
"Walk up the tree, to find out whether we're inside a <use> shadow tree, to find the right title"

> +    Node* parent = const_cast<SVGStyledElement*>(this);
> +    while (parent) {
> +        if (!parent->isShadowNode()) {
> +            parent = parent->parentNode();
> +            continue;
> +        }
> +        
> +        // Get the <use> element.
> +        Node* shadowParent = parent->shadowParentNode();
> +        if (shadowParent && shadowParent->hasTagName(SVGNames::useTag)) {
> +            if (SVGUseElement* useElement = static_cast<SVGUseElement*>(shadowParent)) {
No need to do another if() here, just use
SVGUseElement* useElement = ..
the static_cast can not fail.

> +                // If the <use> title is empty we will keep walking up.
> +                String useTitle(useElement->title());
> +                if (!useTitle.isEmpty())
> +                    return useTitle;
Why do you want to keep walking up if the title is empty?
You're not using the result of 'parent' anywhere, and there are no nested shadow trees. So you can just stop here:
if (useTitle.isEmpty())
    break;
return useTitle;

> +    // If we aren't an instance in a use, then find the first <title> child of this element.
> +    Element* titleElement = firstElementChild();
> +    for ( ; titleElement; titleElement = titleElement->nextElementSibling()) {
You should remove the first space: for(; titleElement; ...)

> +        if (!titleElement->hasTagName(SVGNames::titleTag))
> +            continue;
> +        if (!titleElement->isSVGElement())
> +            continue;
I'd do the checks the other way round, because isSVGElement() is cheaper.

> +    // If a title child was found, return the text contents.
> +    // According to spec, we should not return titles when hovering over <svg> elements (those 
> +    // <title> elements are the title of the document, not a tooltip).
> +    if (titleElement && !this->hasTagName(SVGNames::svgTag))
> +        return titleElement->innerText();
You can omit the "this->" here.
Oh and you need to be careful, and recheck wheter titleElement is actually a SVGNames::titleTag element.
Say you're hovering over a node with a single child, which is not a <title> element, then "titleElement" will store a pointer to it, and you're just returning it's innerText below, no matter whether it's a <title> or not.
To summarize, I'd rewrite as follows:
if (titleElement && titleElement->hasTagName(SVGNames::titleTag))
    return titleElement->innerText();

This saves the svgTag check completly :-)

Sorry that I missed some parts during the last review! Do you have commit access btw? Or do you want to me to cq+ after you've uploaded the next version?

[Jeff Schiller]

Hi Niko,

Some responses to your comments:

> Why do you want to keep walking up if the title is empty?
> You're not using the result of 'parent' anywhere, and there are no nested
> shadow trees. So you can just stop here:
> if (useTitle.isEmpty())
>    break;
> return useTitle;

My assumption is that there were nested shadow trees, so thanks for this review comment.


>> +        if (!titleElement->hasTagName(SVGNames::titleTag))
>> +            continue;
>> +        if (!titleElement->isSVGElement())
>> +            continue;
> I'd do the checks the other way round, because isSVGElement() is cheaper.

Actually it's most likely that all children of SVG elements will themselves be SVG elements.  In this way, it makes more sense to check the tag name first, otherwise the isSVGElement() check would be done for every child.

If you still feel strongly about this though, I will swap them, just let me know :)


>> +    // If a title child was found, return the text contents.
>> +    // According to spec, we should not return titles when hovering over <svg> elements (those 
>> +    // <title> elements are the title of the document, not a tooltip).
>> +    if (titleElement && !this->hasTagName(SVGNames::svgTag))
>> +        return titleElement->innerText();
> You can omit the "this->" here.

Actually I had put that there intentionally because in your earlier review you had mistaken it for me checking whether the child was a <svg> element, which is not the intent.  The intent here is to ensure that the element we are hovering over ("this") is not a <svg>.  In my latest patch, I have moved this check to the very beginning to shortcut a lot of the processing and hopefully make it clearer.  In this change, I have removed the "this" because it would no longer confuse folks since the check is on its own.


> Oh and you need to be careful, and recheck wheter titleElement is actually a
SVGNames::titleTag element.
> Say you're hovering over a node with a single child, which is not a <title>
> element, then "titleElement" will store a pointer to it, and you're just
> returning it's innerText below, no matter whether it's a <title> or not.

Hm, I don't think so.  The for-loop keeps going until we either find a <title> or childElement is null.  There is no other condition.  Therefore I thought simply checking that childElement was non-null was sufficient.

On the other hand, I believe there was an error in your suggested logic because there could (though it's unlikely) be a <title> element in a different namespace.  Wouldn't that trip the hasTagName()?  I have reversed the logic and combined the two if's with boolean-and in the latest patch.  I think it is clearer.

[Jeff Schiller]

Created attachment 53881 [details]
Patch addressing Niko's comments #2

[Nikolas Zimmermann]

Comment on attachment 53881 [details]
Patch addressing Niko's comments #2

Hi Jeff,

the patch is almost perfect! Good that you've moved the svgTag check to the beginning, clearer now.

I still see a small issue with "titleElement", let me explain with your code:

> +    Element* titleElement = firstElementChild();
> +    for (; titleElement; titleElement = titleElement->nextElementSibling()) {
> +        if (titleElement->hasTagName(SVGNames::titleTag) && titleElement->isSVGElement())
> +            break;
> +    }
Ok, you are beginning with the firstElementChild() of a certain SVGStyledElement. You loop over all children, take this as example:

<rect>
   <animateColor/>
</rect>

The titleElement->hasTagName(SVGNames::titleTag) check won't be true, as it's an <animateColor> element. As there is no next sibling,
you will just exit the loop, w/o breaking early. That means "titleElement" now stores the pointer to the <animateColor> element.

> +    // If a title child was found, return the text contents.
> +    if (titleElement)
> +        return titleElement->innerText();

And you end up returning the "innerText" of the <animateColor> element here.
Hope you see the problem. You basically need to recheck the same condition before returning the innerText(), aka:
if (titleELement && titleElement->hasTagName(SVGNames::titleTag) ... )
     return titleElement->innerText();

This is a bit awkward though, so I suggest to rewrite the whole loop, as follows:

<suggestion>
bool foundTitleElement = false;
Element* titleElement = firstElementChild();
for (; titleElement; titleElement = titleElement->nextElementSibling()) {
    foundTitleElement = titleElement->hasTagName(SVGNames::titleTag) && titleElement->isSVGElement();
    if (foundTitleElement)
        break;
}

if (foundTitleElement) {
    ASSERT(titleElement);
    return titleElement->innerText();
}
</suggestion>

This way you save redoing the same check twice, with the drawback of an extra bool, that is negligable though.
Do you have commit access, btw?

Hope you're not getting bored by this patch :-)

[Jeff Schiller]

Hi Nico,

>> +    Element* titleElement = firstElementChild();
>> +    for (; titleElement; titleElement = titleElement->nextElementSibling()) {
>> +        if (titleElement->hasTagName(SVGNames::titleTag) && titleElement->isSVGElement())
>> +            break;
>> +    }
>Ok, you are beginning with the firstElementChild() of a certain
>SVGStyledElement. You loop over all children, take this as example:
>
><rect>
>   <animateColor/>
></rect>
>
>The titleElement->hasTagName(SVGNames::titleTag) check won't be true, as it's
>an <animateColor> element. As there is no next sibling,
>you will just exit the loop, w/o breaking early. That means "titleElement" now
>stores the pointer to the <animateColor> element.

Agreed, the loop will not break early because the if condition is not true.  However, the for-loop should then set titleElement = nextElementSibling() which would be null and the loop would stop executing.

i.e. the loop will keep on going until we reach a <svg:title> element
or there are no more children (in which case titleElement is null on
the last call to nextElementSibling()).

Am I missing something still? :)

[Nikolas Zimmermann]

Comment on attachment 53881 [details]
Patch addressing Niko's comments #2

Oops, I had a misunderstanding, good we resolved it now :-)

[WebKit Commit Bot]

Comment on attachment 53881 [details]
Patch addressing Niko's comments #2

Rejecting patch 53881 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
...
	M	WebCore/ChangeLog
	M	WebCore/svg/SVGAElement.cpp
	M	WebCore/svg/SVGStyledElement.cpp
	M	WebCore/svg/SVGStyledElement.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/1871011

[Nikolas Zimmermann]

Hmm, commit queue is confused by the patch. Is it because of "No new tests (OOPS!)" ??
Jeff, can you update the ChangeLog, to say something like "No new tests, not testable using DRT".
Now that I think about it, could you extend the patch, and add your testcase to "WebCore/manual-tests/svg-tooltips.svg"? That would be nice.

[Jeff Schiller]

Created attachment 54128 [details]
Patch adding manual test

[Nikolas Zimmermann]

Comment on attachment 54128 [details]
Patch adding manual test

Great work Jeff, r=me!

[WebKit Commit Bot]

Comment on attachment 54128 [details]
Patch adding manual test

Clearing flags on attachment: 54128

Committed r58160: <http://trac.webkit.org/changeset/58160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[jay]

jeff,

not sure how you 'slightly modified' the original testcase,

however, 

using the latest nightly, the original testcase does not raise a tooltip, your test does.

inclined to reopen bug, but could open another with your assistance.

[Jeff Schiller]

Jonathan,

Did you look at my slightly modified test case at https://bugs.webkit.org/attachment.cgi?id=53316 ?

As per comment #6 I had to wrap the contents of the <symbol> elements in a <g> and the tooltips showed up. So the following case does not work:

<defs>
  <symbol id="foo">
    <title>Foo</title>
    <rect .../>
    <circle .../>
  </symbol>
</defs>

<use xlink:href="#foo"/>

The user will not see a "Foo" tooltip when hovering over the content.  

To solve this, I had to modify the markup by wrapping the symbol geometry in a group as:

<defs>
  <symbol id="foo">
  <g>
    <title>Foo</title>
    <rect .../>
    <circle .../>
  </g>
  </symbol>
</defs>

<use xlink:href="#foo"/>


If you think the other case is important and that wrapping the geometry of a symbol in a <g> is not a sufficient solution, then please open a new WebKit bug with the title "Symbol tooltips not being displayed"

[jay]

jeff,

tx, done  bug 38198

did anyone check google chrome on pc so we could widen scope on this bug?