1370116 - SVGTextFrame::ShouldRenderAsPath using the wrong ancestor frame?

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Mats Palmgren (inactive)]

I was shuffling around some private nsBlockFrame frame state bits
and discovered that this broke some SVG tests.  The error appears
to be this line in SVGTextFrame::ShouldRenderAsPath:
http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/layout/svg/SVGTextFrame.cpp#5201

In the test layout/reftests/svg/text/ignore-float-first-letter.svg
the ancestor frame there is a nsBlockFrame, so when I made this change:
+FRAME_STATE_BIT(Block, 21, NS_BLOCK_HAS_FIRST_LETTER_CHILD)
it made the above line think it was this bit:
FRAME_STATE_BIT(SVG, 21, NS_STATE_SVG_CLIPPATH_CHILD)

This line comes from bug 1078031.  The odd thing is that what actually
landed is different from the reviewed patch in the bug, which looks
like this instead:
+  if (GetStateBits() & NS_STATE_SVG_CLIPPATH_CHILD) {
https://bugzilla.mozilla.org/attachment.cgi?id=8500171&action=diff#a/layout/svg/SVGTextFrame.cpp_sec8

Comment 1

[Mats Palmgren (inactive)]

Attachment: the frame tree from the reftest for the record

Comment 2

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ec236c3793c80abc6d2043a5bac026b1c3cb6f9

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix

Comment 4

[Robert Longson [:longsonr]]

I don't think this is right. The logic should look like that in currently in SVGTextDrawPathCallbacks::IsClipPathChild i.e. https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGTextFrame.cpp#2772 

Using some common code for identifying whether we're a clipPath child would be even better.

Comment 5

[Mats Palmgren (inactive)]

I saw that solution before filing this patch, but I just don't see how the nearest
SVGText can be something other than |this| here.  Can you explain why you think
that can happen?

There's only one caller of SVGTextFrame::ShouldRenderAsPath:
http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/layout/svg/SVGTextFrame.cpp#3693

Comment 6

[Robert Longson [:longsonr]]

Ahh yes you're right.

Comment 7

[Robert Longson [:longsonr]]

getting the nearest ancestor svgTextFrame to the aFrame will just give you the this pointer you already have.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af93deae530
Use the correct ancestor frame (i.e. |this|) of aFrame for checking NS_STATE_SVG_CLIPPATH_CHILD.  r=longsonr

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4af93deae530