Investigate whether the bbox for outer-<svg> should be the frame bounds for everything except JS calls

RESOLVED FIXED in Firefox 55

Status

()

Core
SVG
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwatt, Assigned: u459114)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
+++ This bug was initially created as a clone of Bug #1320036 +++

Spinning this off from bug 1320036 comment 35.

I agree with that bug that we treat objectBoundingBox specially for filters applied to outer-<svg>, but it also seems like we should be doing the same for other things too. Specifically for mask and for SVG gradient and SVG pattern background-color. If we do that we may want to change the eUseFrameBoundsForOuterSVG enum value to an eForJSCall value and pass eForJSCall from the two nsSVGUtils::GetBBox() calls in SVGTransformableElement.cpp.

Before making any changes we should check what other browsers do, but they may not do anything sensible in the background-color cases in particular.
(Reporter)

Comment 1

a year ago
C.J., since you are working on bug 1320036, would you be interested in looking into this once that's done?
Flags: needinfo?(cku)
(Assignee)

Comment 2

a year ago
sure, I will head back to bug 1320036 and this one next week
Assignee: nobody → cku
Flags: needinfo?(cku)
(Assignee)

Comment 3

a year ago
Created attachment 8846995 [details]
Test case for maskUnits
(Assignee)

Comment 4

a year ago
Created attachment 8847008 [details]
Test case for maskUnits
Attachment #8846995 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
Created attachment 8847009 [details]
Test case for filterUnits
(Assignee)

Comment 6

a year ago
1. maskUnits/filterUnits/gradientUnits/patternUnits="objectBoundingBox"

SVG mask:
Verify attachment 8847008 [details] on different browsers.
maskUnits="objectBoundingBox"
Edge and Safari: mask area is evaluated by element's bound. So you will see a blue rect on screen.
Chrome and FF: mask area is evaluated by bounds of the element's children. So you won't see a blue rect on screen.
I don't think we should follow chrome here, unless we want mask and filter region computed in different way.
We can fix this problem right in this bug.

SVG filter:
Verify attachment 8847009 [details]  on different browsers.
filterUnits="objectBoundingBox"
Edge, Safari and Chrome: filter area is evaluated by element's bound. So you will see a blue rect on screen, except FF.
FF: filter area is evaluated by bounds of the element's children
We will be consistent with the other 3 after bug 1320036 fixed.

SVG gradient/ pattern:
I have no clue of how to make these two elements take effect on <svg> outer element itself. jwatt, have any clue?

2. element.getBBox always returns the tight bounding box in current user space (i.e., after application of the ‘transform’ attribute, if any) on the geometry of all contained graphics elements on all browsers. We probably do not have to do anything on it.
Flags: needinfo?(jwatt)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #6)
> SVG mask:
> I don't think we should follow chrome here, unless we want mask and filter
> region computed in different way.
> We can fix this problem right in this bug.

I agree. The behavior is not expected or useful. We should fix it and side with Edge and Safari.

> We will be consistent with the other 3 after bug 1320036 fixed.

\o/

> SVG gradient/ pattern:
> I have no clue of how to make these two elements take effect on <svg> outer
> element itself. jwatt, have any clue?

It may or may not be possible via 'background-image', but don't worry about it since I can't imagine many people do that. Let's just make the change to the code to make things consistent and move on to more important things.
Flags: needinfo?(jwatt)
(Reporter)

Comment 10

a year ago
mozreview-review
Comment on attachment 8847026 [details]
Bug 1345946 - Part 1. Make SVG masks, gradients and patterns use the frame bounds when applied to outer-<svg>.

https://reviewboard.mozilla.org/r/120062/#review125682

::: commit-message-9b6f0:1
(Diff revision 2)
> +Bug 1345946 - Part 1. Bring eUseFrameBoundsForOuterSVG into SVG mask/gradeint/pattern.

Make the message:

Make SVG masks, gradients and patterns use the frame bounds when applied to outer-<svg>
Attachment #8847026 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8850810 - Flags: review?(jwatt)
(Reporter)

Comment 13

a year ago
mozreview-review
Comment on attachment 8850810 [details]
Bug 1345946 - Part 2. Test that an SVG mask applied to an outer-<svg> uses its frame bounds.

https://reviewboard.mozilla.org/r/123318/#review125720

::: commit-message-4e3fd:1
(Diff revision 1)
> +Bug 1345946 - Part 2. Reftest.

Make this "Test that an SVG mask applied to an outer-<svg> uses its frame bounds"

::: layout/reftests/svg/mask-region.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" version="1.0" mask="url(#mask1)">

This name is far too generic. Call this file mask-root-svg.svg
Attachment #8850810 - Flags: review?(jwatt) → review+
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a85770a265a6
Part 1. Make SVG masks, gradients and patterns use the frame bounds when applied to outer-<svg>. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/4439cd2362dd
Part 2. Test that an SVG mask applied to an outer-<svg> uses its frame bounds. r=jwatt

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a85770a265a6
https://hg.mozilla.org/mozilla-central/rev/4439cd2362dd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.