[Webkit-unassigned] [Bug 16854] display title in tooltip onmouseover in SVG

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=16854





--- Comment #20 from Jeff Schiller <codedread at gmail.com>  2010-04-20 13:34:56 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.