Daniel Holbert [:dholbert] Reporter
	Description • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 8 years ago

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).

	Daniel Holbert [:dholbert] Reporter
	Comment 2 • 8 years ago

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.)

	Jonathan Watt [:jwatt]
	Comment 3 • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 4 • 8 years ago

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.)

	Jonathan Watt [:jwatt]
	Comment 5 • 8 years ago

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'.

	Daniel Holbert [:dholbert] Reporter
	Comment 7 • 8 years ago

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.

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61414/

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 9 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61416/

	Daniel Holbert [:dholbert] Reporter
	Comment 10 • 8 years ago

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/

	Daniel Holbert [:dholbert] Reporter
	Comment 11 • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 12 • 8 years ago

(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!)

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 13 • 8 years ago

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/

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 14 • 8 years ago

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/

	Daniel Holbert [:dholbert] Reporter
	Comment 15 • 8 years ago

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.

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 16 • 8 years ago

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/

	Fariskhi Vidyan [:fvidyan] Assignee
	Comment 17 • 8 years ago

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/

	Daniel Holbert [:dholbert] Reporter
	Comment 18 • 8 years ago

Looks good, thanks! I think this is ready to land. --> Autolanding.

	Daniel Holbert [:dholbert] Reporter
	Comment 19 • 8 years ago

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.)

	Pulsebot
	Comment 20 • 8 years ago

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

	Gregory Szorc [:gps]
	Comment 21 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ab640a3b2894
https://hg.mozilla.org/mozilla-central/rev/f8330947f700

	Kohei Yoshino [:kohei]
	Comment 22 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/svg-preserveaspectratio-no-longer-supports-defer-option/

	Florian Scholz (Open Web Docs)
	Comment 23 • 8 years ago

Mentioned on 
https://developer.mozilla.org/en-US/Firefox/Releases/50#SVG

Removed from
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/preserveAspectRatio

	Sebastian Zartner [:sebo]
	Comment 24 • 8 years ago

Also mentioned it at https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla.

Sebastian