773467 - Make SVG patterns ignore their patternContentUnits attribute when their viewBox attribute is set

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should make SVG patterns ignore their patternContentUnits attribute when their viewBox attribute is set, as per the spec.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 644635 [details] [diff] [review]
patch

Review of attachment 644635 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/base/src/nsSVGPatternFrame.cpp
@@ +546,3 @@
>    // The objectBoundingBox conversion must be handled in the CTM:
> +  if (viewBox.IsExplicitlySet() ||
> +      GetEnumValue(nsSVGPatternElement::PATTERNCONTENTUNITS) ==

If |viewBox.IsExplicitlySet()| is true, we do not want to use callerBBox, but we do need to scale by MaxExpansion.

Comment 3

[Jonathan Watt [:jwatt]]

You also need to |hg add viewBox-and-pattern-02.svg|

Comment 4

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> You also need to |hg add viewBox-and-pattern-02.svg|

I don't, I did a hg cp from viewBox-and-pattern-01.svg

Comment 5

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Comment on attachment 644635 [details] [diff] [review]
> patch
> 
> Review of attachment 644635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/svg/base/src/nsSVGPatternFrame.cpp
> @@ +546,3 @@
> >    // The objectBoundingBox conversion must be handled in the CTM:
> > +  if (viewBox.IsExplicitlySet() ||
> > +      GetEnumValue(nsSVGPatternElement::PATTERNCONTENTUNITS) ==
> 
> If |viewBox.IsExplicitlySet()| is true, we do not want to use callerBBox,
> but we do need to scale by MaxExpansion.

We wouldn't pass http://dev.w3.org/SVG/profiles/1.1F2/test/svg/pservers-pattern-02-f.svg if we took the else branch

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> (In reply to Jonathan Watt [:jwatt] from comment #3)
> > You also need to |hg add viewBox-and-pattern-02.svg|
> 
> I don't, I did a hg cp from viewBox-and-pattern-01.svg

Gah, the bugzilla diff view sucks.

(In reply to Robert Longson from comment #5)
> We wouldn't pass
> http://dev.w3.org/SVG/profiles/1.1F2/test/svg/pservers-pattern-02-f.svg if
> we took the else branch

Looks like nsSVGPatternFrame::GetPatternMatrix also needs to be fixed. Do we pass if you fix that too?

Comment 7

[Robert Longson [:longsonr]]

Erm, quite possibly my build has your patch for that in too at the moment.

Comment 8

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to Robert Longson from comment #4)
> > (In reply to Jonathan Watt [:jwatt] from comment #3)
> > > You also need to |hg add viewBox-and-pattern-02.svg|
> > 
> > I don't, I did a hg cp from viewBox-and-pattern-01.svg
> 
> Gah, the bugzilla diff view sucks.
> 
> (In reply to Robert Longson from comment #5)
> > We wouldn't pass
> > http://dev.w3.org/SVG/profiles/1.1F2/test/svg/pservers-pattern-02-f.svg if
> > we took the else branch
> 
> Looks like nsSVGPatternFrame::GetPatternMatrix also needs to be fixed. Do we
> pass if you fix that too?

No, we need to use the if branch whether or not nsSVGPatternFrame::GetPatternMatrix is fixed

Comment 9

[Jonathan Watt [:jwatt]]

Hmm, I need to think about this some more. The patch I have for this is on top of a pretty extensive (unfinished) refactoring, so I'm not sure why this works and mine works, since your patch is not doing what mine is doing.

Comment 10

[Jonathan Watt [:jwatt]]

Ah, so we do need the bbox transform if the _patternUnits_ attribute is set to objectBoundingBox since the pattern x,y,w,h are then in objectBoundingBox space, and the viewBox is relative to that rect. However, we don't want the bbox transform if the viewBox is set and the patternUnits attribute is set to userSpaceOnUse. So something like the following should do the right thing:

  if ((GetEnumValue(nsSVGPatternElement::PATTERNUNITS) ==
         nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX &&
       viewBox.IsExplicitlySet()) ||
      (GetEnumValue(nsSVGPatternElement::PATTERNCONTENTUNITS) ==
         nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) &&
       !viewBox.IsExplicitlySet()) {
    tCTM.Scale(callerBBox.Width(), callerBBox.Height());
  } else {

Can you also add a test that will fail with the current r-'ed patch?

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 12

[Jonathan Watt [:jwatt]]

I'm not really liking the unitType thing. :/ It may may the code look simpler on the surface, but it actually further obfuscates the already tricky to follow logic from comment 10 which makes it worse for anyone coming along in future who needs to _understand_ the logic here.

Comment 13

[Jonathan Watt [:jwatt]]

Splitting the logic out into a variable with a meaningful name would seem good though.

Comment 14

[Jonathan Watt [:jwatt]]

How about using a bool 'includeBBoxScale' with a comment along the lines of:

    // The spec says that the 'patternContentUnits' attribute "has no effect if
    // attribute ‘viewBox’ is specified". We still need to include a bbox scale
    // if the viewBox is specified and _patternUnits_ is set to or defaults to
    // objectBoundingBox though, since in that case the viewBox is relative to
    // the bbox.

Comment 15

[Jonathan Watt [:jwatt]]

Maybe with the logic matching the order of things in the comment?

  bool includeBBoxScale =
    (GetEnumValue(nsSVGPatternElement::PATTERNCONTENTUNITS) ==
       nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX &&
     !viewBox.IsExplicitlySet()) ||
    (GetEnumValue(nsSVGPatternElement::PATTERNUNITS) ==
       nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX &&
     viewBox.IsExplicitlySet());

Comment 16

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> 
>   bool includeBBoxScale =
>     (GetEnumValue(nsSVGPatternElement::PATTERNCONTENTUNITS) ==
>        nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX &&
>      !viewBox.IsExplicitlySet()) ||
>     (GetEnumValue(nsSVGPatternElement::PATTERNUNITS) ==
>        nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX &&
>      viewBox.IsExplicitlySet());

GetEnumValue is more expensive to call than viewBox.IsExplicitlySet as it's potentially recursive and with your logic we might end up calling it twice.

Comment 17

[Jonathan Watt [:jwatt]]

I wouldn't really mind putting the viewBox.IsExplicitlySet() calls before the GetEnumValue() calls. My refactoring patch makes it so that we only call GetEnumValue() once in PaintPattern for each attribute and pass the results into other callers as appropriate, but I'm not sure when I'm going to be done with that patch.

Comment 18

[Robert Longson [:longsonr]]

Attachment: updated

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 646588 [details] [diff] [review]
updated

Review of attachment 646588 [details] [diff] [review]:
-----------------------------------------------------------------

When I said my refactoring passed in the attributes, I meant that you shouldn't worry about the perf, not that your patch should do the same. That and the member->static movement of some methods is going to make it really painful for me to redo my refactoring from scratch. I'd be much happier with your last, much shorter, patch with just the two |if|s changed as requested.

::: layout/svg/base/src/nsSVGPatternFrame.cpp
@@ +148,5 @@
> +// The SVG specification says that the 'patternContentUnits' attribute "has no effect if
> +// attribute viewBox is specified". We still need to include a bbox scale
> +// if the viewBox is specified and _patternUnits_ is set to or defaults to
> +// objectBoundingBox though, since in that case the viewBox is relative to the bbox
> +static bool

Mark this inline, please.

Comment 20

[Jonathan Watt [:jwatt]]

And I guess this might as well be marked as assigned to you too now.

Comment 21

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21f9237472fa

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21f9237472fa