SVGTextFrame::ShouldRenderAsPath using the wrong ancestor frame?

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8874268 [details]
the frame tree from the reftest for the record
(Assignee)

Comment 3

2 years ago
Created attachment 8874485 [details] [diff] [review]
fix
Attachment #8874485 - Flags: review?(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.
Attachment #8874485 - Flags: review?(longsonr) → review-
(Assignee)

Comment 5

2 years ago
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
Ahh yes you're right.
Attachment #8874485 - Flags: review- → review+
getting the nearest ancestor svgTextFrame to the aFrame will just give you the this pointer you already have.

Comment 8

2 years ago
Pushed by mpalmgren@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4af93deae530
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.