non-scaling-stroke doesn't work with svg.text.css-frames.enabled

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
mozilla24
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
No description provided.
Attachment #754484 - Attachment is patch: true
Attachment #754484 - Flags: review?(jwatt)
Assignee: nobody → longsonr
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.
Attachment #754484 - Flags: review?(jwatt)
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)
Sorry for the delay, I'll look at this soon.
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.
(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 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+
Posted patch updatedSplinter Review
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.
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.
Isn't that already happening due to the existing unicode-bidi: inherit; thing? In what circumstances would I make it worse?
unicode-bidi is on a different style struct.  So that is also causing a separate small amount of memory usage.
Ahh, OK I understand. I still like this approach though :-)

https://tbpl.mozilla.org/?tree=Try&rev=591be258e3fd
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a7ee32eadc9f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.