Closed Bug 1200611 Opened 9 years ago Closed 8 years ago

object-fit fails on layerized elements (e.g. during opacity transition)

Categories

(Core :: Layout, defect)

36 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: polarathene-signup, Assigned: mstange)

References

Details

(Keywords: css3)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

Used the 'object-fit: cover' css property, combined with css transition (animate.css).
Tested on Windows 8.1 x64 FF39/40 and FFDev42.


Actual results:

Tried out the 'object-fit: cover' css property on a small project. When testing on Firefox noticed the style was being disabled temporarily during transitions. You can find a CodePen showing this behaviour: http://codepen.io/polarathene/pen/MwNazK

I also came across a StackOverflow user who experienced the same issue and posted a js-fiddle here: http://stackoverflow.com/questions/28939689/firefox-css3-object-fit-cover-strange-behaviour-during-transition


Expected results:

The image should keep respecting 'object-fit: cover', not readjusting during a transition.
Keywords: css3
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=084441e904d1&tochange=b4fbeba78a7d

Daniel Holbert — Bug 1099450: Turn on pref "layout.css.object-fit-and-position.enabled" (enabling "object-fit" & "object-position" properties). r=dbaron
Blocks: 1099450
Component: Untriaged → Layout
Flags: needinfo?(dbaron)
Product: Firefox → Core
Version: 42 Branch → 36 Branch
Attached file reporter's testcase
This is from http://codepen.io/polarathene/pen/MwNazK, with the codepen HTML mess cleaned up, and as many libraries as I could removed, and the remaining one (animate.min.css) inlined.

It's still too complicated to analyze usefully.
Flags: needinfo?(dbaron)
(And please not that in general, I strongly prefer attached testcases to those at codepen and similar sites, both since those sites include a bunch of additional stuff, and since attached testcases are actually more sure of being preserved and not changed.)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #2)
> Created attachment 8655510 [details]
> reporter's testcase
> 
> This is from http://codepen.io/polarathene/pen/MwNazK, with the codepen HTML
> mess cleaned up, and as many libraries as I could removed, and the remaining
> one (animate.min.css) inlined.
> 
> It's still too complicated to analyze usefully.

I understand my CodePen being to complicated to isolate the problem. I am fairly certain it can be simplified down to using `object-fit: cover` on an image with width/height adjusted to see a visual difference from the source image, then apply a css transition. The js-fiddle is far more simpler at the provided StackOverflow link, direct link here: http://jsfiddle.net/sytwrd9L/1/

I created the report at 3am, will look into attaching an isolated test case with bare minimum for you.
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #3)
> (And please not that in general, I strongly prefer attached testcases to
> those at codepen and similar sites, both since those sites include a bunch
> of additional stuff, and since attached testcases are actually more sure of
> being preserved and not changed.)

Apologies, though I also provided the Github Repo without all the minified code, I can still provide that if useful. I've taken the linked JS-Fiddle, removed JQuery and added 2 more tests. It appears to be related to transitions on the opacity property, I've also verified that this is incorrect behaviour is happening on Chrome 44 as well. Why my CodePen is unaffected in Chrome despite similar transition logic to the JS-Fiddle is unknown. Chrome has another issue with transform3d with `object-fit: cover` this is the 3rd test provided and FireFox handles it fine.

JS-Fiddle: http://jsfiddle.net/sytwrd9L/6/

Attached(bug_object-fit_cover.html) is an html file containing the JS/CSS similar to the linked example, though with an additional delay before removing the 'active' class, otherwise the transition does not occur.
JS-Fiddle equivalent: http://jsfiddle.net/sytwrd9L/6/
Thanks - I think that testcase (and its dependence on opacity changing, in particular) means we aren't honoring 'object-fit' for layerized elements (one of our optimized graphics codepaths).

I can add this to my list of bugs to look into when I'm back from vacation, but someone else may get to it before me.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)
Summary: object-fit: cover fails during transition → object-fit: cover fails on layerized elements (e.g. during opacity transition)
Ah, makes sense.  I'd guess that we're creating an ImageLayer in that case.
(Also: despite comment 1, this isn't actually a regression from bug 1099450.  That's just where support for "object-fit" was turned on, so it's when the attached testcase went from not-honoring-object-fit-at-all to only honoring it some of the time.

Updating dependency to make this depend on bug 624647 (the main 'object-fit' bug), rather than blocking the pref-flip bug.)
No longer blocks: 1099450
Depends on: 624647
Flags: needinfo?(dholbert)
Chrome actually hits what looks like the same bug on the 2nd and 3rd attached testcases here. (from comment 6 and comment 10)
I suspect the code that needs fixing here is the "destRect" we're choosing in nsDisplayImage::ConfigureLayer:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?rev=ae625cd90875#1690

The second half of that function seems to assume that the image will be stretched to fill nsDisplayImage::GetDestRect, but that's not true if 'object-fit' is set.

We do somewhat-similar calculations in nsDisplayImage::GetLayerState -- that may need fixing as well.
I filed https://code.google.com/p/chromium/issues/detail?id=531739 on the Chrome version of this bug.

(I'm setting needinfo=me to circle back to this after I've finished processing vacation-email backlog and worked through high-priority items.)
Flags: needinfo?(dholbert)
The bread in this example will fall back to the specified dimensions: https://d157rqmxrxj6ey.cloudfront.net/natewiebe13/14723/
Flags: needinfo?(dholbert)
Summary: object-fit: cover fails on layerized elements (e.g. during opacity transition) → object-fit fails on layerized elements (e.g. during opacity transition)
Bug 1200611 - Size ImageLayers correctly for <img>s using object-fit. r?dholbert

This changes nsDisplayImage::GetDestRect and nsImageFrame::PredictedDestRect to return
the true dest rect of the image, without intersecting it with the image content box.
The only caller of these functions that actually requires a rectangle that's within
the image's content bounds is nsDisplayImage::GetOpaqueRegion, so I'm changing that
to intersect with the display item's bounds.
I'm pretty sure nsImageFrame::MaybeDecodeForPredictedSize() also prefers the unclipped
dest rect, because it cares about the scale at which the image is painted, and not
about which parts of the image are painted.
Attachment #8695287 - Flags: review?(dholbert)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment on attachment 8695287 [details]
MozReview Request: Bug 1200611 - Size ImageLayers correctly for <img>s using object-fit. r?dholbert

https://reviewboard.mozilla.org/r/27049/#review24449

Thank you for taking this! I was hoping to get to fixing this today or tomorrow, but now I don't need to. \o/  r=me

::: layout/reftests/bugs/1200611-1-ref.html:3
(Diff revision 1)
> +<title>Make sure the ImageLayer is sized correctly when using object-fit</title>

Nit: Consider putting "Reference: " in the reference case's <title>, so it's easier to tell which file is which when viewing them in two tabs side-by-side (because often, the URL doesn't fit in the URLbar, so you can't easily see the filename).
Attachment #8695287 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6c81a2c6c2ac3183f31afb437fc88534022f44
Bug 1200611 - Size ImageLayers correctly for <img>s using object-fit. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/ca6c81a2c6c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Markus, would you be up for having this backported to aurora? (which does happen to be nearly-beta)

Given that we've had 2 dupes of this filed, it looks like people are hitting this in the real world, and it'd be nice to fix it 6 weeks sooner.
Flags: needinfo?(mstange)
NI flag removed because FF45 is going to be release in a few days.
Flags: needinfo?(mstange)
Depends on: 1255641
No longer depends on: 1255641
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: