Closed Bug 1101128 Opened 6 years ago Closed 6 years ago
Image background not painted, with "object-fit:none" and larger <img> element
643 bytes, text/html
2.74 KB, patch
|Details | Diff | Splinter Review|
13.92 KB, patch
|Details | Diff | Splinter Review|
While writing some tests for bug 1098417, I discovered that we don't always paint <img> backgrounds right now, presumably because we (no-longer-correctly) assume that the entire content-rect will be covered up by image data. (I hadn't caught this before, because all my tests for this have had borders, and borders turn off the background-disabling optimization, because we might need to draw the background between border-dots/dashes.) Testcase attached. Patch coming up soon, hopefully.
Attachment #8524787 - Attachment description: test.html → testcase 1 (make sure "layout.css.object-fit-and-position.enabled" is on)
Specifically, we need to teach nsDisplayImage::GetOpaqueRegion that the full image content-box is not actually guaranteed to be opaque (for an opaque image) anymore.
This fixes it locally for me. Just need to add a reftest & a code comment or two, and then this is ready for review.
(Part 1 also rewraps some lines that are longer than 80 characters.)
FWIW, the patch includes two reftests: - one with "object-fit" preventing the image from scaling up to cover the container element. - one with the image the same size as its container, but offset with "object-position" (so that some background should shine through) Both tests fail without this patch, but pass with the patch.
Summary: Image background not always painted, with "object-fit:none" and large <img> element → Image background not painted, with "object-fit:none" and larger <img> element
Comment on attachment 8525032 [details] [diff] [review] part 1: move nsDisplayImage::GetOpaqueRegion to .cpp file Review of attachment 8525032 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #8525032 - Flags: review?(seth) → review+
Comment on attachment 8525033 [details] [diff] [review] part 2: Make nsDisplayImage::GetOpaqueRegion() take object-fit & object-position into account Review of attachment 8525033 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: layout/generic/nsImageFrame.cpp @@ +1462,5 @@ > + // what GetBounds() returns). So, we grab those rects and intersect them. > + const nsRect frameContentBox = GetBounds(aSnap); > + > + // Note: To get the "dest rect", we have to provide the "constraint rect" > + // (which is the content-box, with the effects of fragmentation undone). Should we have a test for the fragmentation case? ::: layout/reftests/image/image-object-fit-with-background-2.html @@ +4,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ > +--> > +<!-- > + This testcase ensures that we paint the background around an opaque image, > + when the image is offset from the container via 'object-position'. This is Should this test be called |image-object-position-with-background|, then?
Attachment #8525033 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #9) > Should we have a test for the fragmentation case? Yup, probably. I'll spin one up, or possibly punt to a followup (per IRC) if it ends up being non-trivial or turning up unexpected dragons. :) > Should this test be called |image-object-position-with-background|, then? Ah, good point! Yes. I'll fix that before landing. Thanks for the review!
(Sorry, disregard comment 11; I wasn't seeing fragmentation there because we only fragment *block-level* img elements. Makes sense; we don't fragment other inline-level stuff like inline-blocks.)
Updated version of part 2. Addressed both things mentioned in comment 9 - renamed the object-position reftest, and added reftests with fragmentation. (I also adjusted the reftests to use a handy already-existing 32x32 blue PNG file, instead of a 1x1 png data URI. I hadn't noticed that particular PNG when initially writing these reftests.) Try run w/ these changes: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1e296bac8630
Try run's looking good; landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/67e4a03c17f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb767f5730d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.