Last Comment Bug 773467 - Make SVG patterns ignore their patternContentUnits attribute when their viewBox attribute is set
: Make SVG patterns ignore their patternContentUnits attribute when their viewB...
Status: RESOLVED FIXED
[parity-opera][parity-chrome][parity-...
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Robert Longson
:
Mentors:
http://dev.w3.org/SVG/profiles/1.1F2/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-12 15:06 PDT by Jonathan Watt [:jwatt]
Modified: 2012-07-31 19:20 PDT (History)
1 user (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.44 KB, patch)
2012-07-21 07:00 PDT, Robert Longson
jwatt: review-
Details | Diff | Splinter Review
address review comments (6.30 KB, patch)
2012-07-22 10:30 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated (18.59 KB, patch)
2012-07-27 08:14 PDT, Robert Longson
jwatt: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2012-07-12 15:06:39 PDT
We should make SVG patterns ignore their patternContentUnits attribute when their viewBox attribute is set, as per the spec.
Comment 1 Robert Longson 2012-07-21 07:00:42 PDT
Created attachment 644635 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] 2012-07-21 08:07:46 PDT
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] 2012-07-21 08:08:29 PDT
You also need to |hg add viewBox-and-pattern-02.svg|
Comment 4 Robert Longson 2012-07-21 08:17:38 PDT
(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 2012-07-21 08:19:21 PDT
(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] 2012-07-21 08:54:46 PDT
(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 2012-07-21 09:01:15 PDT
Erm, quite possibly my build has your patch for that in too at the moment.
Comment 8 Robert Longson 2012-07-21 09:37:31 PDT
(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] 2012-07-21 15:33:06 PDT
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] 2012-07-21 15:39:31 PDT
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 2012-07-22 10:30:03 PDT
Created attachment 644768 [details] [diff] [review]
address review comments
Comment 12 Jonathan Watt [:jwatt] 2012-07-23 09:09:50 PDT
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] 2012-07-23 09:10:32 PDT
Splitting the logic out into a variable with a meaningful name would seem good though.
Comment 14 Jonathan Watt [:jwatt] 2012-07-23 09:32:10 PDT
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] 2012-07-23 09:36:41 PDT
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 2012-07-23 10:20:26 PDT
(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] 2012-07-23 10:26:53 PDT
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 2012-07-27 08:14:25 PDT
Created attachment 646588 [details] [diff] [review]
updated
Comment 19 Jonathan Watt [:jwatt] 2012-07-29 08:46:28 PDT
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] 2012-07-29 08:47:05 PDT
And I guess this might as well be marked as assigned to you too now.
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-07-31 19:20:16 PDT
https://hg.mozilla.org/mozilla-central/rev/21f9237472fa

Note You need to log in before you can comment on or make changes to this bug.