1473450 - remove <angle> values from image-orientation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

The <angle> values have been deprecated in the spec, and we are the only browser to implement them anyway.

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f3929ba5607750706c30a3d3a87837103533fe

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e10615988236deb750e233417af881127d11772

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36105fb8193b2ba3cdb79e5d73eaf8ef9bbc439d

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1473450 - Remove angle values from image-orientation.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8989907 [details]
Bug 1473450 - Remove angle values from image-orientation.

https://reviewboard.mozilla.org/r/254910/#review262092

I haven't taken a super-close look at the removed tests, let me know if you want me to do that. But the tests changed look good.

::: layout/base/nsLayoutUtils.cpp
(Diff revision 1)
>    nsCOMPtr<imgIContainer> img(aContainer);
>  
> -  if (aOrientation.IsFromImage()) {
> +  if (aOrientation == StyleImageOrientation::FromImage) {
>      img = ImageOps::Orient(img, img->GetOrientation());
> -  } else if (!aOrientation.IsDefault()) {
> -    Angle angle = aOrientation.Angle();

Chances are that we could clean up `Orientation` and `ImageOps::Orient` in a followup. Maybe worth filing a followup ImageLib bug?

Also, given the three callers of this function are in the same file and that this function is simple we could just make it static in nsImageFrame.cpp, but that's not a big deal.

::: layout/style/nsComputedDOMStyle.cpp:3758
(Diff revision 1)
>    val->SetNumber(StyleUIReset()->mForceBrokenImageIcon);
>    return val.forget();
>  }
>  
>  already_AddRefed<CSSValue>
>  nsComputedDOMStyle::DoGetImageOrientation()

You can just remove this method given this is a keyword, it'll automatically get serialized using Servo.

::: servo/components/style/properties/gecko.mako.rs
(Diff revision 1)
>  
>  </%self:impl_trait>
>  
> -<%self:impl_trait style_struct_name="InheritedBox"
> +<%self:impl_trait style_struct_name="InheritedBox">
> -                  skip_longhands="image-orientation">
> -    // FIXME: Gecko uses a tricky way to store computed value of image-orientation

Neat

::: servo/components/style/properties/longhands/inherited_box.mako.rs:62
(Diff revision 1)
>                           extra_servo_values="pixelated crisp-edges",
>                           custom_consts=image_rendering_custom_consts,
>                           animation_value_type="discrete",
>                           spec="https://drafts.csswg.org/css-images/#propdef-image-rendering")}
>  
> -${helpers.predefined_type("image-orientation",
> +${helpers.single_keyword("image-orientation",

Could you use block indentation here while touching this? I'm touching this file as well and I was going to reformat the rest :)

::: testing/web-platform/tests/css/css-images/parsing/image-orientation-invalid.html:23
(Diff revision 1)
>  test_invalid_value("image-orientation", "0deg from-image");
>  test_invalid_value("image-orientation", "flip 0deg");
>  test_invalid_value("image-orientation", "flip from-image");
>  test_invalid_value("image-orientation", "from-image 0deg");
>  test_invalid_value("image-orientation", "from-image flip");
> +test_invalid_value("image-orientation", "30deg");

Maybe worth pointing in a comment that the value was deprecated / removed?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Also, please send an intent to unship for this. Thanks!

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8989907 [details]
Bug 1473450 - Remove angle values from image-orientation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254910/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/641b02da5961
Remove angle values from image-orientation. r=emilio

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing xpcshell at devtools/shared/tests/unit/test_css-properties-db.js and mochitest at  layout/style/test/test_compute_data_with_start_struct.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=641b02da59616e7f83213eb65a12514cf58b059e

Failure logs:
xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=187309132&repo=autoland&lineNumber=10268
mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=187310548&repo=autoland&lineNumber=1991

Backout: https://hg.mozilla.org/integration/autoland/rev/a73e38ceb4252f1f1c3e19c7385af3ee8dbf1181

Comment 10

[Andreea Pavel [:apavel]]

There are also devtools failures that appeared on a later push: https://treeherder.mozilla.org/logviewer.html#?job_id=187314364&repo=autoland&lineNumber=1784

06:45:51     INFO -  266 INFO TEST-START | devtools/client/inspector/rules/test/browser_rules_cycle-angle.js
06:45:52     INFO -  TEST-INFO | started process screenshot
06:45:52     INFO -  TEST-INFO | screenshot: exit 0
06:45:52     INFO -  Buffered messages logged at 06:45:51
06:45:52     INFO -  267 INFO Entering test bound
06:45:52     INFO -  268 INFO Adding a new tab with URL: data:text/html;charset=utf-8,%0A%20%20%3Cstyle%20type%3D%22text%2Fcss%22%3E%0A%20%20%20%20body%20%7B%0A%20%20%20%20%20%20image-orientation%3A%201turn%3B%0A%20%20%20%20%7D%0A%20%20%20%20div%20%7B%0A%20%20%20%20%20%20image-orientation%3A%20180deg%3B%0A%20%20%20%20%7D%0A%20%20%3C%2Fstyle%3E%0A%20%20%3Cbody%3E%3Cdiv%3ETest%3C%2Fdiv%3Ecycling%20angle%20units%20in%20the%20rule%20view!%3C%2Fbody%3E%0A
06:45:52     INFO -  269 INFO Tab added and finished loading
06:45:52     INFO -  270 INFO Loading the helper frame script chrome://mochitests/content/browser/devtools/client/inspector/rules/test/doc_frame_script.js
06:45:52     INFO -  271 INFO Opening the inspector
06:45:52     INFO -  272 INFO Opening the toolbox
06:45:52     INFO -  Buffered messages logged at 06:45:52
06:45:52     INFO -  273 INFO Toolbox opened and focused
06:45:52     INFO -  Buffered messages finished
06:45:52    ERROR -  274 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-angle.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_cycle-angle.js:35 - TypeError: valueNode is null
06:45:52     INFO -  Stack trace:
06:45:52     INFO -  checkAngleCycling@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_cycle-angle.js:35:3
06:45:52     INFO -  async*@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_cycle-angle.js:26:9
06:45:52     INFO -  Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1098:34
06:45:52     INFO -  async*Tester_execTest@chrome://mochikit/content/browser-test.js:1089:16
06:45:52     INFO -  nextTest/<@chrome://mochikit/content/browser-test.js:991:9
06:45:52     INFO -  SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
06:45:52     INFO -  275 INFO Leaving test bound
06:45:52     INFO -  276 INFO Removing tab.
06:45:52     INFO -  277 INFO Waiting for event: 'TabClose' on [object XULElement].
06:45:52     INFO -  278 INFO Got event: 'TabClose' on [object XULElement].
06:45:52     INFO -  279 INFO Tab removed and finished closing
06:45:52     INFO -  GECKO(4608) | MEMORY STAT | vsize 782MB | vsizeMaxContiguous 732MB | residentFast 288MB | heapAllocated 120MB
06:45:52     INFO -  280 INFO TEST-OK | devtools/client/inspector/rules/test/browser_rules_cycle-angle.js | took 989ms
06:45:52     INFO -  281 INFO checking window state

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11883 for changes under testing/web-platform/tests

Comment 12

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc7b50ea3e8e0e6403de4b6fdbf0a45b9f0517c

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 15

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=230943399fd3d6c8457ef4ce14cb014125ed6b6b

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8989907 [details]
Bug 1473450 - Remove angle values from image-orientation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254910/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a6bef7a9177
Remove angle values from image-orientation. r=emilio

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6a6bef7a9177

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 20

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/image-orientation-property-no-longer-accepts-angle-values/

Comment 21

[Estelle Weyl]

[X] - update https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation
[X] - update code example - remove obsolete values
[X] - update compat table - https://github.com/mdn/browser-compat-data/pull/2655
[X] - update css syntax - https://github.com/mdn/data/pull/269