Bug 139690 – REGRESSION (r154769): Wrong <title> taken as a tooltip for SVG element

• Component: SVG
• Status: RESOLVED FIXED

[Eric Poitras]

Here a live sample:

http://jsfiddle.net/msj518e4/3/

Hover the large circle with the mouse and it will display "small circle!".

Here is the associated markup:
<svg>
    <g>
        <circle cx="10" cy="10" r="10" fill="red"> 
            <title>small circle!</title>
        </circle>
        <circle cx="50" cy="50" r="20" fill="red"> </circle>
    </g>
</svg>

Title should be picked up only if it's the first direct children of the node.

Tested in all other browsers (Even opera?) and it works fine.

Might be related as well to this other bug: https://bugs.webkit.org/show_bug.cgi?id=127305

[Said Abou-Hallawa]

Created attachment 244292 [details]
Patch

[Andreas Kling]

Comment on attachment 244292 [details]
Patch

r=me. It would be nice to have a test.

[Said Abou-Hallawa]

Created attachment 244387 [details]
Patch

[Said Abou-Hallawa]

Created attachment 244390 [details]
Patch

[Daniel Bates]

Comment on attachment 244390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244390&action=review

The change looks good and I'm happy that you wrote a test. I have some feedback on how we can make the test more straightforward to read and identify the expected result of each sub-test.

> Source/WebCore/ChangeLog:3
> +        Safari 7.1 picks the wrong title to display as a tooltip on SVG element.

This issue isn't specific to Safari. I updated the title of this bug.

There is an unwritten convention (we should document it) of prefixing bugs that are known to be regressions with "REGRESSION (r154769):" where r154769 is thet revision number that caused the regression. We also add the keyword Regression to keyword field on the Bugzilla bug. The benefit of both of these conventions is that it makes such bugs stand out in the ChangeLog/commit message and in Bugzilla among other benefits.

> Source/WebCore/ChangeLog:9
> +        was calling Traversal<SVGTitleElement>::firstWithin(this)) which is the same as calling

This was the same as firstChild()? I haven't looked at the code, but given the rest of your description below and the name of the class (Traversal) and member function (firstWithin) I get the impression that this function returned the first direct child of |this| that was a SVGTitleElement, which may not necessarily be the first child (since the first child may not be a SVGTitleElement.).

> LayoutTests/svg/hittest/svg-tooltip-expected.txt:4
> +	parent id:	main_g

Although you write PASS or FAILED at the end of the results it's difficult to make sense of what the respected result is for each of the sub-tests. See my remake below about making use of js-test-pre.js. The convenience functions in that JavaScript script can be used to make the output of this test more understandable.

> LayoutTests/svg/hittest/svg-tooltip.html:22
> +    {

Instead of duplicating logic to perform comparisons and logging we should make use of the functionality provided in LayoutTests/resources/js-test-pre.js.

> LayoutTests/svg/hittest/svg-tooltip.html:64
> +        if (!verifySVGToolTips(children[i]))

It would be better if we didn't stop at the first failure so as to not hide multiple regressions (even though we would hope that someone would take notice that this test failed as soon as it does).

> LayoutTests/svg/hittest/svg-tooltip.html:113
> +  <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="500" height="500" id="main_svg">

The xmlns attributes should be removed as this SVG a is embedded in an HTML document.

[Said Abou-Hallawa]

Created attachment 244471 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #5)
> Comment on attachment 244390 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244390&action=review
> 
> The change looks good and I'm happy that you wrote a test. I have some
> feedback on how we can make the test more straightforward to read and
> identify the expected result of each sub-test.
>

I changed the expected text file to include the word "PASS" only in case of success. In case of failure, all failed elements with the actual and the expected tooltips will be written instead.
 
> > Source/WebCore/ChangeLog:3
> > +        Safari 7.1 picks the wrong title to display as a tooltip on SVG element.
> 
> This issue isn't specific to Safari. I updated the title of this bug.
> 
> There is an unwritten convention (we should document it) of prefixing bugs
> that are known to be regressions with "REGRESSION (r154769):" where r154769
> is thet revision number that caused the regression. We also add the keyword
> Regression to keyword field on the Bugzilla bug. The benefit of both of
> these conventions is that it makes such bugs stand out in the
> ChangeLog/commit message and in Bugzilla among other benefits.
>

ChangeLogs are fixed to include the new title.
 
> > Source/WebCore/ChangeLog:9
> > +        was calling Traversal<SVGTitleElement>::firstWithin(this)) which is the same as calling
> 
> This was the same as firstChild()? I haven't looked at the code, but given
> the rest of your description below and the name of the class (Traversal) and
> member function (firstWithin) I get the impression that this function
> returned the first direct child of |this| that was a SVGTitleElement, which
> may not necessarily be the first child (since the first child may not be a
> SVGTitleElement.).
> 

Yes firstWithin() is the same as firstChild for "Elements". At the top of ElementTraversal.h, the following comment is there.

// For Elements firstWithin is always the same as first child.

I changed the ChangeLog to explain the difference between firstWithin() and the descendantsOfType<SVGTitleElement>(this).begin().

> > LayoutTests/svg/hittest/svg-tooltip-expected.txt:4
> > +	parent id:	main_g
> 
> Although you write PASS or FAILED at the end of the results it's difficult
> to make sense of what the respected result is for each of the sub-tests. See
> my remake below about making use of js-test-pre.js. The convenience
> functions in that JavaScript script can be used to make the output of this
> test more understandable.
> 

I could have used "debug()" to log the actual and the expected tooltips in case of failure.

> > LayoutTests/svg/hittest/svg-tooltip.html:22
> > +    {
> 
> Instead of duplicating logic to perform comparisons and logging we should
> make use of the functionality provided in
> LayoutTests/resources/js-test-pre.js.
> 

The comparison and the logging in case of failure is now in a single function named: verifyElementToolTip().

> > LayoutTests/svg/hittest/svg-tooltip.html:64
> > +        if (!verifySVGToolTips(children[i]))
> 
> It would be better if we didn't stop at the first failure so as to not hide
> multiple regressions (even though we would hope that someone would take
> notice that this test failed as soon as it does).
> 

Done.

> > LayoutTests/svg/hittest/svg-tooltip.html:113
> > +  <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="500" height="500" id="main_svg">
> 
> The xmlns attributes should be removed as this SVG a is embedded in an HTML
> document.

Done.

[Daniel Bates]

Comment on attachment 244471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244471&action=review

> LayoutTests/svg/hittest/svg-tooltip-expected.txt:2
> +PASS
> +

This output is not useful given that this test is testing tooltips, which are user-visible text. Such simplistic output would be reasonable for a test that verified a fix for a crash or assertion failure since the presence of any page content ("PASS") indicates that we did not crash.

> LayoutTests/svg/hittest/svg-tooltip.html:34
> +    // Searches the tree elements upward till it finds an element an SVGTitleElement child
> +    function titleElementFromElementUpward(element)
> +    {
> +      for (; element; element = element.parentElement) {
> +                  
> +        // No tooltip for the root SVG element    
> +        if (element.tagName == "svg")
> +          break;
> +
> +        var children = element.childNodes;
> +
> +        // Search immediate children and if any of them is a title element return it
> +        for (var i = 0; i < children.length; i++) {
> +          if (children[i].tagName == "title")
> +            return children[i];
> +        }
> +      }
> +      
> +      return null;
> +    }

We should not be mimicking the C++ algorithm as a way to test this bug. Mimicking the C++ algorithm in JavaScript makes updating this test more fragile and error prone than having hardcoded string literals of the expected results because the algorithm in this function will need to be kept synchronized with the algorithm in WebCore. Computing the expected tooltip text instead of having hardcoded expected tooltip text also makes it time intensive for a person to verify the correctness of the expected result as they will need to execute this algorithm in there head (or run the test) to verify correctness.

I envision using hardcoded string literals for the expected tooltip text together with shouldBeEqualToString() (defined in js-test-pre.js) and the Internals method you added (window.internals.toolTipFromElement) to verify that the actual tooltip text is identical to the hardcoded string literal. You may also want to consider adding a window.internals.toolTipAtPoint() function that takes an (x, y) coordinate pair and returns the tooltip for the element at the specified point and/or expose a method to return the tooltip for a shadow DOM element such that we can write a test for the visible contents of the SVG <use> element (e.g. test that hovering over the clone of big_circle associated with <use id="use_without_title"> displays the tooltip "nested circle!" - (see remark (*))). Notice that it is not sufficient to use document.elementFromPoint() for get a reference to such an element since it is not part of the user-visible DOM.

(*) We can get away with using internals.toolTipFromElement() on <circle id="big_circle"> defined in <defs> to test this case, but it would be more representative to go through the hit-test machinery with a (x, y)-coordinate pair for this case and window.internals.toolTipAtPoint() would allow us to get the tooltip for other shadow DOM elements.

> LayoutTests/svg/hittest/svg-tooltip.html:46
> +      if (!titleElement && toolTip.length == 0 || toolTip == titleElement.textContent)

Take element := document.getElementById("main_svg"). Suppose there is a regression such that internals.toolTipFromElement(element) is equal to the empty string. Then a JavaScript type error will occur when we access titleElement.textContent (since titleElement == null). While the JavaScript error does indicate that the sub-test failed it causes this entire test to fail (i.e. we do not run any other sub-test) and the output of such an error is not very meaningful since it is a JavaScript error and a person will need to read the code to determine that internals.toolTipFromElement(element) returned the empty string.

Moreover, the first conjunct encodes a valid sub-test: the root <svg> element never has a tooltip. We should explicitly have such a sub test.

> LayoutTests/svg/hittest/svg-tooltip.html:84
> +    // Verifies the tooltip text for an SVG element and all its descendants
> +    function verifyElementTreeToolTips(element)
> +    {
> +      var children = element.childNodes;
> +      var result = verifyElementToolTip(element);
> +
> +      // Verify the tooltips of the children elements
> +      for (var i = 0; i < children.length; i++) {
> +        if (children[i].nodeType != 1)
> +          continue;
> +          
> +        if (children[i].tagName == "defs" || children[i].tagName == "title")
> +          continue;
> +          
> +        result &= verifyElementTreeToolTips(children[i]);
> +      }
> +      
> +      return result;
> +    }
> +    
> +    function runTest()
> +    {
> +      if (window.testRunner) {
> +        testRunner.dumpAsText();
> +        debug(verifyElementTreeToolTips(document.getElementsByTagName("svg")[0], 0) ? "PASS" : "FAILED");
> +       }
> +     }

We're underutilizing the convenience functions in js-test-pre.js. We should use shouldBeEqualToString() to test a sub-test. Moreover, we should use description() to print a textual description of this test instead of putting the description of this test in an HTML comment (lines 4 - 10). It's unnecessary to call testRunner.dumpAsText() as js-test-pre.js and instead of defining runTest() here and using the HTML onload attribute on <body> (line 87) we should just inline the content of runTest(), and move all associated logic, to a <script> placed before </body>. Then we can remove the HTML onload attribute on <body>. See my remark below (**) on how I would write this test in terms of shouldBeEqualToString() and description().

> LayoutTests/svg/hittest/svg-tooltip.html:113
> +      <circle id="big_circle" cx="80" cy="80" r="20" fill="red">

This markup is invalid because the value of the attribute id is not unique by <http://www.w3.org/TR/SVG/struct.html#IDAttribute>. The id "big_circle" was defined above (line 101).

> LayoutTests/svg/hittest/svg-tooltip.html:117
> +        <rect id="lime_rect" x="20" y="20" width="40" height="40" fill="lime"/>

Similarly, the id "lime_rect" is not unique as it was defined above (line 100).

> LayoutTests/svg/hittest/svg-tooltip.html:127
> +    <use id="use_without_title" xlink:href="#shape" x="0" y="240" />

Nit: This is the only self-closing element that has a space character before the '/'. We should use a consistent style throughout the test.

> LayoutTests/svg/hittest/svg-tooltip.html:128
> +  </svg>

(**) After </svg>, I would code of the form (may not be correct):

<script>
function tooltipForElementByIdShouldBeEqualToString(id, expectedTooltip)
{
  shouldBeEqualToString('internals.toolTipFromElement(document.getElementById("' + id + '"))', expectedTooltip);
}

description("This test ensures that the tooltip text for an SVG element is calculated correctly.");

if (!window.testRunner || !window.internals)
  testFailed("Test must be run in DumpRenderTree/WebKitTestRunner with support for window.internals.");
else {
  tooltipForElementByIdShouldBeEqualToString("main_svg", "");
  tooltipForElementByIdShouldBeEqualToString("main_g", "main g element");
  tooltipForElementByIdShouldBeEqualToString("yellow_rect", "main g element");
  tooltipForElementByIdShouldBeEqualToString("small_circle", "small circle!");
  tooltipForElementByIdShouldBeEqualToString("sub_g", "main g element");
  tooltipForElementByIdShouldBeEqualToString("nested_circle", "nested circle!");
  tooltipForElementByIdShouldBeEqualToString("use_with_title", "use element!");
  // Add test for use_without_title, big_circle (child of <g id="main_g">; defined on line 113), and lime_rect (child of <g id="sub_g">; defined on line 117)
}
document.body.removeChild(document.getElementById("main_svg")); // Remove SVG subtree so as to make the result less cluttered when viewed in the browser.
</script>

> LayoutTests/svg/hittest/svg-tooltip.html:129
> +  <script src="../../resources/js-test-pre.js"></script>

js-test-pre.js => js-test-post.js

[Daniel Bates]

(In reply to comment #7)
> (In reply to comment #5)
> > Comment on attachment 244390 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=244390&action=review
> > 
> > The change looks good and I'm happy that you wrote a test. I have some
> > feedback on how we can make the test more straightforward to read and
> > identify the expected result of each sub-test.
> >
> 
> I changed the expected text file to include the word "PASS" only in case of
> success. In case of failure, all failed elements with the actual and the
> expected tooltips will be written instead.
>  

This output is not useful. See my remark at the top of comment 8 for more details.

> [...]
> I changed the ChangeLog to explain the difference between firstWithin() and
> the descendantsOfType<SVGTitleElement>(this).begin().
> 

The updated ChangeLog description reads well.

> > > LayoutTests/svg/hittest/svg-tooltip-expected.txt:4
> > > +	parent id:	main_g
> > 
> > Although you write PASS or FAILED at the end of the results it's difficult
> > to make sense of what the respected result is for each of the sub-tests. See
> > my remake below about making use of js-test-pre.js. The convenience
> > functions in that JavaScript script can be used to make the output of this
> > test more understandable.
> > 
> 
> I could have used "debug()" to log the actual and the expected tooltips in
> case of failure.
> 

I suggest that we make use of the comparison functions in js-test-pre.js, including shouldBeEqualToString(). See my remark in comment 8 for more details and an example.

[Daniel Bates]

Comment on attachment 244471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244471&action=review

> Source/WebCore/ChangeLog:28
> +        (WebCore::Internals::toolTipFromElement):

Nit: Tooltip is one word. So, this method should be named, tooltipFromElement. This misspelling occurs throughout the patch.

[Daniel Bates]

(In reply to comment #8)
> It's unnecessary to call testRunner.dumpAsText() as js-test-pre.js and instead of
> defining runTest() here and using the HTML onload attribute on <body> (line
> 87) we should just inline the content of runTest(), and move all associated
> logic, to a <script> placed before </body>.

This should read:

It's unnecessary to call testRunner.dumpAsText() as js-test-pre.js does this for us. Additionally, instead of defining runTest() here and using the HTML onload attribute on <body> (line 87) we should just inline the content of runTest() and move all associated logic to a <script> placed before </body>.

> (**) After </svg>, I would code of the form (may not be correct):

This should read:

> (**) After </svg>, I would include code of the form (may not be correct):

[Said Abou-Hallawa]

Created attachment 244485 [details]
Patch

[Daniel Bates]

Comment on attachment 244485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244485&action=review

r-. You didn't address my remarks in comment 8.

> Source/WebCore/ChangeLog:30
> +        Add a new internal function toolTipFromElement() which returns the tooltip text for a

Nit: toolTipFromElement => tooltipFromElement

(since tooltip is one word)

> LayoutTests/svg/hittest/svg-tooltip-expected.txt:5
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS
> +

You didn't address my remarks in comment 8.

[Said Abou-Hallawa]

Created attachment 244629 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 244471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244471&action=review

>> Source/WebCore/ChangeLog:28
>> +        (WebCore::Internals::toolTipFromElement):
> 
> Nit: Tooltip is one word. So, this method should be named, tooltipFromElement. This misspelling occurs throughout the patch.

The chrome.cpp has the function Chrome::setToolTip() and in it all the variables are named as if tooltip were two parts. Other examples are Settings::showsURLsInToolTips(), HitTestResult::spellingToolTip(), etc.  I was just trying to be consistent with the C++ code.

>> LayoutTests/svg/hittest/svg-tooltip-expected.txt:2
>> +
> 
> This output is not useful given that this test is testing tooltips, which are user-visible text. Such simplistic output would be reasonable for a test that verified a fix for a crash or assertion failure since the presence of any page content ("PASS") indicates that we did not crash.

The test outputs "PASS" only if it passes. If it fails it outputs the mismatched actual and the expected tooltips.

>> LayoutTests/svg/hittest/svg-tooltip.html:34
>> +    }
> 
> We should not be mimicking the C++ algorithm as a way to test this bug. Mimicking the C++ algorithm in JavaScript makes updating this test more fragile and error prone than having hardcoded string literals of the expected results because the algorithm in this function will need to be kept synchronized with the algorithm in WebCore. Computing the expected tooltip text instead of having hardcoded expected tooltip text also makes it time intensive for a person to verify the correctness of the expected result as they will need to execute this algorithm in there head (or run the test) to verify correctness.
> 
> I envision using hardcoded string literals for the expected tooltip text together with shouldBeEqualToString() (defined in js-test-pre.js) and the Internals method you added (window.internals.toolTipFromElement) to verify that the actual tooltip text is identical to the hardcoded string literal. You may also want to consider adding a window.internals.toolTipAtPoint() function that takes an (x, y) coordinate pair and returns the tooltip for the element at the specified point and/or expose a method to return the tooltip for a shadow DOM element such that we can write a test for the visible contents of the SVG <use> element (e.g. test that hovering over the clone of big_circle associated with <use id="use_without_title"> displays the tooltip "nested circle!" - (see remark (*))). Notice that it is not sufficient to use document.elementFromPoint() for get a reference to such an element since it is not part of the user-visible DOM.
> 
> (*) We can get away with using internals.toolTipFromElement() on <circle id="big_circle"> defined in <defs> to test this case, but it would be more representative to go through the hit-test machinery with a (x, y)-coordinate pair for this case and window.internals.toolTipAtPoint() would allow us to get the tooltip for other shadow DOM elements.

We talked in person about this issue and we agreed on keeping the mimicking approach since the scope of the fix has changed to include another fix for the rootmost SVG element since we were following the specs for this case. I had to test the case of inline SVG and embedded SVG and I had also to include a test for the stand-alone SVG case. In the stand-alone SVG case, js-test-pre.js can't be used.

>> LayoutTests/svg/hittest/svg-tooltip.html:46
>> +      if (!titleElement && toolTip.length == 0 || toolTip == titleElement.textContent)
> 
> Take element := document.getElementById("main_svg"). Suppose there is a regression such that internals.toolTipFromElement(element) is equal to the empty string. Then a JavaScript type error will occur when we access titleElement.textContent (since titleElement == null). While the JavaScript error does indicate that the sub-test failed it causes this entire test to fail (i.e. we do not run any other sub-test) and the output of such an error is not very meaningful since it is a JavaScript error and a person will need to read the code to determine that internals.toolTipFromElement(element) returned the empty string.
> 
> Moreover, the first conjunct encodes a valid sub-test: the root <svg> element never has a tooltip. We should explicitly have such a sub test.

Fixed. Thanks for pointing this out.

>> LayoutTests/svg/hittest/svg-tooltip.html:84
>> +     }
> 
> We're underutilizing the convenience functions in js-test-pre.js. We should use shouldBeEqualToString() to test a sub-test. Moreover, we should use description() to print a textual description of this test instead of putting the description of this test in an HTML comment (lines 4 - 10). It's unnecessary to call testRunner.dumpAsText() as js-test-pre.js and instead of defining runTest() here and using the HTML onload attribute on <body> (line 87) we should just inline the content of runTest(), and move all associated logic, to a <script> placed before </body>. Then we can remove the HTML onload attribute on <body>. See my remark below (**) on how I would write this test in terms of shouldBeEqualToString() and description().

As mentioned above js-test-pre.js can't be used with the stand-alone case, so I was limited in using its functions.

>> LayoutTests/svg/hittest/svg-tooltip.html:113
>> +      <circle id="big_circle" cx="80" cy="80" r="20" fill="red">
> 
> This markup is invalid because the value of the attribute id is not unique by <http://www.w3.org/TR/SVG/struct.html#IDAttribute>. The id "big_circle" was defined above (line 101).

Fixed. All the elements inside the <defs> section do not have ID's.

>> LayoutTests/svg/hittest/svg-tooltip.html:117
>> +        <rect id="lime_rect" x="20" y="20" width="40" height="40" fill="lime"/>
> 
> Similarly, the id "lime_rect" is not unique as it was defined above (line 100).

Fixed

>> LayoutTests/svg/hittest/svg-tooltip.html:127
>> +    <use id="use_without_title" xlink:href="#shape" x="0" y="240" />
> 
> Nit: This is the only self-closing element that has a space character before the '/'. We should use a consistent style throughout the test.

Fixed.

>> LayoutTests/svg/hittest/svg-tooltip.html:128
>> +  </svg>
> 
> (**) After </svg>, I would code of the form (may not be correct):
> 
> <script>
> function tooltipForElementByIdShouldBeEqualToString(id, expectedTooltip)
> {
>   shouldBeEqualToString('internals.toolTipFromElement(document.getElementById("' + id + '"))', expectedTooltip);
> }
> 
> description("This test ensures that the tooltip text for an SVG element is calculated correctly.");
> 
> if (!window.testRunner || !window.internals)
>   testFailed("Test must be run in DumpRenderTree/WebKitTestRunner with support for window.internals.");
> else {
>   tooltipForElementByIdShouldBeEqualToString("main_svg", "");
>   tooltipForElementByIdShouldBeEqualToString("main_g", "main g element");
>   tooltipForElementByIdShouldBeEqualToString("yellow_rect", "main g element");
>   tooltipForElementByIdShouldBeEqualToString("small_circle", "small circle!");
>   tooltipForElementByIdShouldBeEqualToString("sub_g", "main g element");
>   tooltipForElementByIdShouldBeEqualToString("nested_circle", "nested circle!");
>   tooltipForElementByIdShouldBeEqualToString("use_with_title", "use element!");
>   // Add test for use_without_title, big_circle (child of <g id="main_g">; defined on line 113), and lime_rect (child of <g id="sub_g">; defined on line 117)
> }
> document.body.removeChild(document.getElementById("main_svg")); // Remove SVG subtree so as to make the result less cluttered when viewed in the browser.
> </script>

As mentioned above js-test-pre.js can't be used with the stand-alone case, so I was limited in using its functions.

>> LayoutTests/svg/hittest/svg-tooltip.html:129
>> +  <script src="../../resources/js-test-pre.js"></script>
> 
> js-test-pre.js => js-test-post.js

Fixed.

[Said Abou-Hallawa]

Comment on attachment 244485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244485&action=review

>> Source/WebCore/ChangeLog:30
>> +        Add a new internal function toolTipFromElement() which returns the tooltip text for a
> 
> Nit: toolTipFromElement => tooltipFromElement
> 
> (since tooltip is one word)

I just wanted to be consistent with the C++ code which treats the word 'tooltip' as if it were two words.

>> LayoutTests/svg/hittest/svg-tooltip-expected.txt:5
>> +
> 
> You didn't address my remarks in comment 8.

I did upload this patch before reading your comments on the previous patch. Sorry about that.

[Daniel Bates]

Comment on attachment 244629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244629&action=review

> LayoutTests/svg/hittest/svg-standalone-tooltip.svg:25
> +  <!-- This title should only appear in the browser chrome and not as a tooltip -->
> +  <title>outermost svg</title>

This file is OK as-is. For your information, we can test that this text is the document title by querying document.title in JavaScript (*). No need to make this change though before landing this patch.

(*) Obviously some care needs to be taken such that we only ensure document.title == "outermost svg" when this file is loaded as a standalone SVG.

[WebKit Commit Bot]

Comment on attachment 244629 [details]
Patch

Clearing flags on attachment: 244629

Committed r178459: <http://trac.webkit.org/changeset/178459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This broke the Windows build:

     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: __thiscall WebCore::HitTestResult::HitTestResult(void)" (??0HitTestResult@WebCore@@QAE@XZ) referenced in function "public: class WTF::String __thiscall WebCore::Internals::toolTipFromElement(class WebCore::Element *,int &)const " (?toolTipFromElement@Internals@WebCore@@QBE?AVString@WTF@@PAVElement@2@AAH@Z)
     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::HitTestResult::setInnerNode(class WebCore::Node *)" (?setInnerNode@HitTestResult@WebCore@@QAEXPAVNode@2@@Z) referenced in function "public: class WTF::String __thiscall WebCore::Internals::toolTipFromElement(class WebCore::Element *,int &)const " (?toolTipFromElement@Internals@WebCore@@QBE?AVString@WTF@@PAVElement@2@AAH@Z)
     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: class WTF::String __thiscall WebCore::HitTestResult::title(enum WebCore::TextDirection &)const " (?title@HitTestResult@WebCore@@QBE?AVString@WTF@@AAW4TextDirection@2@@Z) referenced in function "public: class WTF::String __thiscall WebCore::Internals::toolTipFromElement(class WebCore::Element *,int &)const " (?toolTipFromElement@Internals@WebCore@@QBE?AVString@WTF@@PAVElement@2@AAH@Z)

We probably need declarations added to the Windows "WebKitExports.def.in" file.

[Brent Fulgham]

Windows build fix landed in r178470.