Closed
Bug 876450
Opened 9 years ago
Closed 9 years ago
non-scaling-stroke doesn't work with svg.text.css-frames.enabled
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(2 files, 1 obsolete file)
6.81 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
6.92 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Attachment #754484 -
Attachment is patch: true
Attachment #754484 -
Flags: review?(jwatt)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → longsonr
Comment 1•9 years ago
|
||
Comment on attachment 754484 [details] [diff] [review] patch Review of attachment 754484 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/nsSVGUtils.cpp @@ +1437,5 @@ > { > + if (aFrame->IsSVGText()) { > + while (aFrame->GetType() != nsGkAtoms::svgTextFrame2) { > + aFrame = aFrame->GetParent(); > + } You can use nsLayoutUtils::GetClosestFrameOfType instead of the loop.
Assignee | ||
Updated•9 years ago
|
Attachment #754484 -
Flags: review?(jwatt)
Assignee | ||
Comment 2•9 years ago
|
||
Since non-scaling-stroke is a reset property it seems I can only get the right result when I check the correct frame. I need to skip any anonymous block frames that may be in the way. Is this the best way to do it? DominantBaseline is handled in the frame iterator for text so that sidesteps the issue as presumably the text frame iterator knows how to ignore anonymous block frames.
Attachment #754484 -
Attachment is obsolete: true
Attachment #756225 -
Flags: review?(cam)
Comment 3•9 years ago
|
||
Sorry for the delay, I'll look at this soon.
Comment 4•9 years ago
|
||
I wonder why vector-effect is non-inherited. That's different from all the other stroke-* properties. I guess if we were going to extend vector-effect with other effects, those might make sense to be not inherited. Might be another indication that the plan to extend vector-effect with a bunch of different things is a bad idea.
Comment 5•9 years ago
|
||
(In reply to Robert Longson from comment #2) > > Since non-scaling-stroke is a reset property it seems I can only get the > right result when I check the correct frame. I need to skip any anonymous > block frames that may be in the way. Is this the best way to do it? I'm not sure if it is or isn't the best way to do it. I had a similar issue with unicode-bidi, which is also not inherited, and I handled that by having ::moz-svg-text { unicode-bidi: inherit; } in svg.css. That was easier than modifying everywhere that looked at unicode-bidi under layout/generic/ to maybe look up past the anonymous SVG text block frame. > DominantBaseline is handled in the frame iterator for text so that sidesteps > the issue as presumably the text frame iterator knows how to ignore > anonymous block frames. Yeah, I don't think dominant-baseline has this problem.
Comment 6•9 years ago
|
||
Comment on attachment 756225 [details] [diff] [review] new and improved Review of attachment 756225 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: layout/svg/nsSVGUtils.cpp @@ +1443,5 @@ > + // skip any anonymous block frames as vector effects are reset > + // CSS properties > + while (aFrame->GetParent()->GetContent() == aFrame->GetContent()) { > + aFrame = aFrame->GetParent(); > + } Since the only anonymous frame under the nsSVGTextFrame2 we'll get is the anonymous block frame child, so how about we check for that explicitly. So something like: parent = aFrame->GetParent(); if (parent->GetParent() is the nsSVGTextFrame2) parent = parent->GetParent(); Also please make the comment something like "If the parent of aFrame is the anonymous block child of the nsSVGTextFrame2, then we want to look at the style of the nsSVGTextFrame2 instead. This is because vector-effect is a reset property and won't have been inherited into the anonymous block frame."
Attachment #756225 -
Flags: review?(cam) → review+
Assignee | ||
Comment 7•9 years ago
|
||
If I use the suggestion from comment 2 I don't need the dancing around the anonymous block code at all so I've just removed it.
Comment 8•9 years ago
|
||
That's fine, though be aware it means you'll use a small amount of memory for the additional nsStyleSVGReset for the anonymous block frame.
Assignee | ||
Comment 9•9 years ago
|
||
Isn't that already happening due to the existing unicode-bidi: inherit; thing? In what circumstances would I make it worse?
Comment 10•9 years ago
|
||
unicode-bidi is on a different style struct. So that is also causing a separate small amount of memory usage.
Assignee | ||
Comment 11•9 years ago
|
||
Ahh, OK I understand. I still like this approach though :-) https://tbpl.mozilla.org/?tree=Try&rev=591be258e3fd
Assignee | ||
Comment 12•9 years ago
|
||
Fixing the reftest failure: https://tbpl.mozilla.org/?tree=Try&rev=7eeabebc097e
Assignee | ||
Comment 13•9 years ago
|
||
Still checking reftests: https://tbpl.mozilla.org/?tree=Try&rev=c28e9b460b37
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ee32eadc9f
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a7ee32eadc9f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•