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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Comment 1

[Cameron McCormack (:heycam)]

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.

Comment 2

[Robert Longson [:longsonr]]

Attachment: new and improved

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.

Comment 3

[Cameron McCormack (:heycam)]

Sorry for the delay, I'll look at this soon.

Comment 4

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

(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

[Cameron McCormack (:heycam)]

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."

Comment 7

[Robert Longson [:longsonr]]

Attachment: updated

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

[Cameron McCormack (:heycam)]

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.

Comment 9

[Robert Longson [:longsonr]]

Isn't that already happening due to the existing unicode-bidi: inherit; thing? In what circumstances would I make it worse?

Comment 10

[Cameron McCormack (:heycam)]

unicode-bidi is on a different style struct.  So that is also causing a separate small amount of memory usage.

Comment 11

[Robert Longson [:longsonr]]

Ahh, OK I understand. I still like this approach though :-)

https://tbpl.mozilla.org/?tree=Try&rev=591be258e3fd

Comment 12

[Robert Longson [:longsonr]]

Fixing the reftest failure:

https://tbpl.mozilla.org/?tree=Try&rev=7eeabebc097e

Comment 13

[Robert Longson [:longsonr]]

Still checking reftests:

https://tbpl.mozilla.org/?tree=Try&rev=c28e9b460b37

Comment 14

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ee32eadc9f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a7ee32eadc9f