The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: Robert Longson)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-opera][parity-chrome][parity-safari], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
We should make SVG patterns ignore their patternContentUnits attribute when their viewBox attribute is set, as per the spec.
(Reporter)

Updated

5 years ago
Assignee: nobody → jwatt
(Assignee)

Updated

5 years ago
(Assignee)

Comment 1

5 years ago
Created attachment 644635 [details] [diff] [review]
patch
Attachment #644635 - Flags: review?(jwatt)
(Reporter)

Comment 2

5 years ago
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.
Attachment #644635 - Flags: review?(jwatt) → review-
(Reporter)

Comment 3

5 years ago
You also need to |hg add viewBox-and-pattern-02.svg|
(Assignee)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

5 years ago
(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
(Reporter)

Comment 6

5 years ago
(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?
(Assignee)

Comment 7

5 years ago
Erm, quite possibly my build has your patch for that in too at the moment.
(Assignee)

Comment 8

5 years ago
(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
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
Created attachment 644768 [details] [diff] [review]
address review comments
Attachment #644635 - Attachment is obsolete: true
Attachment #644768 - Flags: review?(jwatt)
(Reporter)

Comment 12

5 years ago
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.
(Reporter)

Comment 13

5 years ago
Splitting the logic out into a variable with a meaningful name would seem good though.
(Reporter)

Comment 14

5 years ago
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.
(Reporter)

Comment 15

5 years ago
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());
(Assignee)

Comment 16

5 years ago
(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.
(Reporter)

Comment 17

5 years ago
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.
(Assignee)

Comment 18

5 years ago
Created attachment 646588 [details] [diff] [review]
updated
Attachment #644768 - Attachment is obsolete: true
Attachment #644768 - Flags: review?(jwatt)
Attachment #646588 - Flags: review?(jwatt)
(Reporter)

Comment 19

5 years ago
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.
Attachment #646588 - Flags: review?(jwatt) → review+
(Reporter)

Comment 20

5 years ago
And I guess this might as well be marked as assigned to you too now.
Assignee: jwatt → longsonr
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f9237472fa
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/21f9237472fa
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.