Closed
Bug 1189307
Opened 10 years ago
Closed 10 years ago
Use GetClosestFrameOfType to get svgTextFrame
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
INVALID
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(1 obsolete file)
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → longsonr
Attachment #8641062 -
Flags: review?(dholbert)
Comment 2•10 years ago
|
||
heycam is probably a better reviewer here, since he understands the invariants around SVG text frames better than I do.
Can you provide some context to explain what's changing here & why? Note that this does seem to be a functional change -- the existing code just looks up 1-2 frames and stops -- in contrast, the GetClosestFrameOfType() call you're swapping in will walk up the tree indefinitely.
(Maybe that's still no functional change, if we know there must be a svgTextFrame at most 2 hops away? I don't know.)
Comment 3•10 years ago
|
||
Comment on attachment 8641062 [details] [diff] [review]
closest.txt
One purely-stylistic nit, though:
>+++ b/layout/svg/nsSVGEffects.cpp
>+ frame = nsLayoutUtils::GetClosestFrameOfType
>+ (frame->GetParent(), nsGkAtoms::svgTextFrame);
This wrapping style is a bit unconventional. Better not to have a newline after the function-name / open-paren, unless absolutely necessary to avoid going over the 80 character line limit. And we're not at risk of hitting that limit here -- this could just be:
frame = nsLayoutUtils::GetClosestFrameOfType(frame->GetParent(),
nsGkAtoms::svgTextFrame);
Canceling review flag; please tag heycam for review after you've given a bit more background per comment 2 (since I suspect that'll help heycam understand what's going on, too).
Attachment #8641062 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•10 years ago
|
||
See bug 1130888 for why I think this may be required.
Assignee | ||
Updated•10 years ago
|
Attachment #8641062 -
Flags: review?(cam)
Comment 5•10 years ago
|
||
I'm not sure this is right -- will this cause <tspan fill="url(...)"> to instead use the ancestor <text> element's paint server reference instead?
Can you provide an example document showing, or explain how, the existing code doesn't work? I'm not so sure the problem in bug 1130888 is analagous here.
Flags: needinfo?(longsonr)
Assignee | ||
Comment 6•10 years ago
|
||
These are the frame trees from bug 1130888 (it has text and a textPath)
nsTextFrame = aTargetFrame
nsInlineFrame
nsInlineFrame <- the textPath frame
nsBlockFrame <- an intermediate block frame
SVGTextFrame <- the text frame
nsTextFrame = aTargetFrame
nsBlockFrame <- an intermediate block frame
SVGTextFrame <- the text frame
so the existing code is correct as is that for bug 113088 (since SVG state bits only exist on SVG frames)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(longsonr)
Resolution: --- → INVALID
Assignee | ||
Updated•10 years ago
|
Attachment #8641062 -
Attachment is obsolete: true
Attachment #8641062 -
Flags: review?(cam)
You need to log in
before you can comment on or make changes to this bug.
Description
•