Closed Bug 1280425 Opened 4 years ago Closed 4 years ago

Drop support for SVG <image preserveAspectRatio="defer ...">

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dholbert, Assigned: fvidyan)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files)

The "defer" keyword for preserveAspectRatio has been removed in SVG 2. The updated definition is:

> Name                  Value                    Initial value    Animatable
> preserveAspectRatio   <align> <meetOrSlice>?   MidYMid meet     yes
https://svgwg.org/svg2-draft/coords.html#PreserveAspectRatioAttribute

(There's some other cruft in the draft that currently mistakenly includes the "defer" keyword, but the official definition linked/quoted above is notably missing the "defer" keyword.)

We support this keyword right now, but we should probably remove it to stay in sync with the spec and to simplify our code.  This will let us simplify our SVG-as-an-image drawing codepath a bit.
The SVG Working Group resolved on this a year ago, BTW -- for the context around that decision, see the "pAR defer" section in the meeting notes here (pAR is an abbreviation for "preserveAspectRatio"):
 https://lists.w3.org/Archives/Public/www-svg/2015Jun/0057.html

Quoting the meeting notes there:
 #  heycam: so in Edge, defer is not supported, in Firefox it is,
 #  in Blink the effect is basically "always defer"
 #  ... i.e. pAR on <image> is ignored when referencing an SVG
 #  image

So, this bug would bring us into alignment with what Edge does (and with what the SVG2 spec now calls for).
I'd like to assign this bug to an intern, Fariskhi Vidyan, who's starting next week.

I believe this bug can be thought of as several separate steps:

(i) REMOVE THE PARSING CODE. Remove the SetDefer() call from the ToPreserveAspectRatio() parsing function, here:
https://hg.mozilla.org/mozilla-central/annotate/3ce53bd1e25b93140484d3933c9339a829e0c1eb/dom/svg/SVGAnimatedPreserveAspectRatio.cpp#l149
... and remove the GetDefer() code branch that follows it. (Adjust the code to behave as if GetDefer() always returned false [which it will, because it's going away].)  And remove the symmetric AppendLiteral("defer ") call further down in the file.

(ii) FIX UP TESTS.  Run "./mach reftest reftests/svg/image/" and find the reftests that are broken as a result of this change. (specifically the ones that use "defer"; you may also have failures in other tests that are due to minor differences between your configuration & our test boxes; ignore those.)  Fix up any tests that fail as a result of this change, to no longer use the "defer" attribute.  (This might mean just removing a chunk of the test, or removing the test entirely.)  We probably need to add a test to verify that previously-valid values that involve "defer" are now rejected, too. 

(iii) REMOVE SUPPORTING CODE THAT NOW BECOMES OBSOLETE.  Specifically, remove "mDefer", "GetDefer()", etc. from these files and their .cpp versions:
  https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGPreserveAspectRatio.h
  https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGAnimatedPreserveAspectRatio.cpp#258
...and search for similar calls in neighboring files, and fix any build breakage that may result as a result of these APIs going away.  There's also an early-return in SVGSVGElement.cpp that can go away:
  https://hg.mozilla.org/mozilla-central/annotate/3ce53bd1e25b93140484d3933c9339a829e0c1eb/dom/svg/SVGSVGElement.cpp#l1120


Additional notes:
If we split this work into multiple patches (and I think we should), it might make sense to perform parts (i) and (ii) in a single patch, and part (iii) in a second patch.  (This splits the work into nice bite-size chunks, while still leaving the codebase in a state where it builds & passes tests at each intermediate point in the commit history.)
Ideally I think we'd go further and remove support for 'preserveAspectRatio' from <image> (what Blink does), and have authors use 'object-fit' and 'object-position'. Those properties should function in contexts other than just SVG's <image>, and should be "the" way to do what pAR is intended for.
Hmm, ok. Do you think it's still worth unsupporting "defer" as an intermediate stage, then? (i.e. comment 2) Or should we push back on the SVG WG resolution, and try to get them to drop preserveAspectRatio for <image> altogether in SVG2?

(I don't believe we currently support "object-fit"/"object-position" for the SVG <image> element, BTW, though it's probably not too much work for us to add that support.)
Flags: needinfo?(jwatt)
If someone has the cycles to push the SVG WG to remove pAR from <image> that would be great. (The fact that Blink doesn't support it will prevent most authors from coming to rely on it, so I don't think there's a huge urgency.) Either way, removing "defer" still seems like a positive step in the right direcition. If at some point people want "defer" behavior hopefully the CSS WG can be persuaded to add a new value to 'object-fit'.
Flags: needinfo?(jwatt)
Attached image testcase 1
Here's heycam's testcase (taken from http://mcc.id.au/temp/defer.svg ), which he mentioned in the meeting notes that I linked in comment 1.

When this bug is fixed, I believe there should be red visible on the left, but not on the right.
Assignee: nobody → farislab
Comment on attachment 8766591 [details]
Bug 1280425 part 1: Remove support for deprecated value "defer" in SVG preserveAspectRatio.

https://reviewboard.mozilla.org/r/61414/#review58418

This is nearly there! Just a few nits.

::: dom/svg/SVGAnimatedPreserveAspectRatio.cpp:148
(Diff revision 1)
>    }
>    const nsAString &token = tokenizer.nextToken();
>  
>    nsresult rv;
>    SVGPreserveAspectRatio val;
> -
> +  

Looks like this patch is inserting a few space characters on this otherwise-blank line -- please remove those.

::: layout/reftests/svg/as-image/defer-unsupported-1-helper.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100" viewBox="0 0 50 50" preserveAspectRatio="xMaxYMax">

Here as well, please wrap before "viewBox", to keep this under 80 characters.

::: layout/reftests/svg/as-image/defer-unsupported-1-ref.svg:5
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink">
> +
> +  <rect width="100" height="100" fill="blue"/>
> +  <image xlink:href="defer-unsupported-1-helper.svg" width="200" height="100" preserveAspectRatio="non-keyword xMaxYMax"/>

Here (in the reference case), let's just drop the "preserveAspectRatio" attributre entirely.

Right now, with the valid-looking xMaxYMax part of the attribute in particular, it's not entirely clear to someone unfamiliar with the spec what the expectation is.  Since the point is that the attribute should just be ignored in the testcase, it's simpler & clearer to just not have the attribute at all in the reference case.

::: layout/reftests/svg/as-image/defer-unsupported-1.svg:5
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink">
> +
> +  <rect width="100" height="100" fill="blue"/>
> +  <image xlink:href="defer-unsupported-1-helper.svg" width="200" height="100" preserveAspectRatio="defer xMaxYMax"/>

Two changes for this line:

(1) Please use "xMinYMin" here (not xMaxYMax) -- in particular, use a different keyword from what the helper-file uses (and different from the default behavior as well).  That way, if the test fails (due to us incorrectly honoring one of the preserveAspectRatio values), it will be obvious *which element's* preserveAspectRatio is being incorrectly honored.  And that will make the test-failure easier to debug.  (Right now, with both tags having xMaxYMax, it'd be harder to reason about a failure in this test.)

(2) We try to keep lines of text to a maximum of 80 characters, for readability when files are being viewed side-by-side.

So -- please add an explicit linebreak in this line before the 80-character mark -- e.g.  somewhere (e.g. maybe after |height="100"|), so that it ends up like this (hopefully mozreview doesn't hork the formatting):

  `<image xlink:href="defer-unsupported-1-helper.svg" width="200" height="100"
         preserveAspectRatio="defer xMaxYMax"/>`

::: layout/reftests/svg/as-image/reftest.list:244
(Diff revision 1)
>  != nonuniform-scale-3d.html?0.3&1.0&0.3  nonuniform-scale-3d.html?0.3&0.3&0.3
>  != nonuniform-scale-3d.html?0.3&1.0&0.3  nonuniform-scale-3d.html?1.0&1.0&1.0
>  != nonuniform-scale-3d.html?1.0&0.3&0.3  nonuniform-scale-3d.html?0.3&0.3&0.3
>  != nonuniform-scale-3d.html?1.0&0.3&0.3  nonuniform-scale-3d.html?1.0&1.0&1.0
> +
> +# Test for preserveAspectRatio with unsupported defer

s/unsupported defer/no-longer-supported "defer" keyword/
Comment on attachment 8766592 [details]
Bug 1280425 part 2: Remove code associated with now-unsupported "defer" in SVG.

https://reviewboard.mozilla.org/r/61416/#review58428

Part 2 looks good! r+ on that part.
(So: all that remains is for you to fix your "part 1" patch to address my nits from comment 10, and then re-push the stack of patches to mozreview as you did before.

If you like, you can explicitly mark each review comment as "addressed" in MozReview UI, too -- that's useful, though it doesn't really matter.  After you've pushed, I think Bugzilla will re-prompt me for review on part 1, and I'll grant it if everything looks good, and then we can get this landed!)
Comment on attachment 8766591 [details]
Bug 1280425 part 1: Remove support for deprecated value "defer" in SVG preserveAspectRatio.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61414/diff/1-2/
Attachment #8766591 - Flags: review- → review?(dholbert)
Comment on attachment 8766592 [details]
Bug 1280425 part 2: Remove code associated with now-unsupported "defer" in SVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61416/diff/1-2/
Comment on attachment 8766591 [details]
Bug 1280425 part 1: Remove support for deprecated value "defer" in SVG preserveAspectRatio.

https://reviewboard.mozilla.org/r/61414/#review58512

Looks good -- r=me with two things fixed:

First, I think you might've missed my final review nit from comment 10, about the comment in reftest.list -- please clarify that comment as noted there.

::: layout/reftests/svg/as-image/defer-unsupported-1.svg:5
(Diff revisions 1 - 2)
>  <svg xmlns="http://www.w3.org/2000/svg"
>       xmlns:xlink="http://www.w3.org/1999/xlink">
>  
>    <rect width="100" height="100" fill="blue"/>
> -  <image xlink:href="defer-unsupported-1-helper.svg" width="200" height="100" preserveAspectRatio="defer xMaxYMax"/>
> +  <image xlink:href="defer-unsupported-1-helper.svg" width="200" height="100" 

There's a stray space character at the end of this line -- please delete that space character.
Attachment #8766591 - Flags: review?(dholbert) → review+
Comment on attachment 8766591 [details]
Bug 1280425 part 1: Remove support for deprecated value "defer" in SVG preserveAspectRatio.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61414/diff/2-3/
Comment on attachment 8766592 [details]
Bug 1280425 part 2: Remove code associated with now-unsupported "defer" in SVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61416/diff/2-3/
Looks good, thanks! I think this is ready to land. --> Autolanding.
Er, on second thought, let's do one final Try build first. (I think you got a Try run on an earlier patch version, but worth checking that none of the test tweaks accidentally broke the test.)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab640a3b2894
part 1: Remove support for deprecated value "defer" in SVG preserveAspectRatio. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f8330947f700
part 2: Remove code associated with now-unsupported "defer" in SVG. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/ab640a3b2894
https://hg.mozilla.org/mozilla-central/rev/f8330947f700
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.