Closed Bug 1473450 Opened 5 years ago Closed 5 years ago

remove <angle> values from image-orientation

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

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

Attachments

(1 file)

Bug 1473450 - Remove angle values from image-orientation.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
The <angle> values have been deprecated in the spec, and we are the only browser to implement them anyway.
Blocks: 1358327
Assignee: nobody → cam
Status: NEW → ASSIGNED
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?
Attachment #8989907 - Flags: review?(emilio) → review+
Also, please send an intent to unship for this. Thanks!
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/641b02da5961
Remove angle values from image-orientation. r=emilio
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11883 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/6a6bef7a9177
Remove angle values from image-orientation. r=emilio
https://hg.mozilla.org/mozilla-central/rev/6a6bef7a9177
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(cam)
[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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.