Bug 11686 – WebKit draws Carto.net tabgroup example completely incorrectly (ff does fine)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

WebKit draws Carto.net tabgroup example completely incorrectly (ff does fine)

http://www.carto.net/papers/svg/gui/tabgroup/index.svg

It looks like there is an issue measuring the text which is causing the tabs to be so small.  Then some text drawing issues, as well as some hit detection issues. The hit detection issues might be related to the measuring.

[Rob Buis]

Hi Eric,

(In reply to comment #0)
> WebKit draws Carto.net tabgroup example completely incorrectly (ff does fine)
> 
> http://www.carto.net/papers/svg/gui/tabgroup/index.svg
> 
> It looks like there is an issue measuring the text which is causing the tabs to
> be so small.  Then some text drawing issues, as well as some hit detection
> issues. The hit detection issues might be related to the measuring.

I looked into it and getBBox is wrong on the text element that gets created dynamically. It is
basically an empty rect (0, 0, 0, 0), causing indeed measuring problems. It seems the render object
is created but not layouted. Not sure how to handle this yet...
Cheers,

Rob.

[Eric Seidel (no email)]

> I looked into it and getBBox is wrong on the text element that gets created
> dynamically. It is
> basically an empty rect (0, 0, 0, 0), causing indeed measuring problems. It
> seems the render object
> is created but not layouted. Not sure how to handle this yet...


I expect that getBBox just needs to force a layout, similar to how offsetTop works for HTML.

[Rob Buis]

Created attachment 11719 [details]
First attempt

This could be a hack, but it works. Hyatt mentioned layoutIfNeeded being used a lot like this, but I couldn't really find it. Also I dislike the const_casts. I am wondering whether html text has the same problem as svg text, where the text is not layouted yet but the text dimensions are queried.
Cheers,

Rob.

Comment on attachment 11719 [details]
First attempt

You shouldn't try to force layout of a subtree like that, nor should the check be pushed down to the renderer.

The way it's done in HTML is that the DOM method that needs layout information ensures that the document is laid out before querying the renderer. See Element::offsetTop() for example.

[Rob Buis]

Created attachment 11721 [details]
Improved patch

Just what I was looking for, thnx Mitz!
Cheers,

Rob.

[Rob Buis]

Created attachment 11722 [details]
Also test paths and images

Mitz was right, same problem can happen with images. This patch fixes it as well as does better testing.
Cheers,

Rob.

Comment on attachment 11722 [details]
Also test paths and images

I doubt that anything that doesn't apply to all SVGLocatables is correct and robust. For example, what if your element's renderer is a RenderSVGContainer? Can't it have text and image children? It could be my lack of familiarity with this code, but why would the straightforward solution of ensuring layout in SVGLocatable::getBBox not work?

Minor style comments: please avoid "layouted" and use Sentence capitalization in comments.

[Rob Buis]

Created attachment 11735 [details]
Improved testcase + patch

As Mitz pointed out, may as well make sure there is a layout done for all svg render objects.
Cheers,

Rob.

[Rob Buis]

Created attachment 11744 [details]
Improved testcase

This patch has a fix in the comment.
Furthermore it always performs the updateLayoutIgnorePendingStylesheets call, since potentially the element could have a parent that just had a display property changed from none.
Cheers,

Rob.

[Rob Buis]

Created attachment 11745 [details]
Improved patch

Moved the call back into the if since render style updating means the renderer() must be there.
Apart from that some rewording of the WebCore/ChangeLog.
Cheers,

Rob.

Comment on attachment 11745 [details]
Improved patch

r=me if you uppercase "svg" and change "are layout" to "are laid out" in the ChangeLog. I still don't like that sentence :-)

[Mark Rowe (bdash)]

Landed by Rob in r18032.