Closed
Bug 1101128
Opened 10 years ago
Closed 10 years ago
Image background not painted, with "object-fit:none" and larger <img> element
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(3 files, 2 obsolete files)
643 bytes,
text/html
|
Details | |
2.74 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
13.92 KB,
patch
|
dholbert
:
review+
|
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8524787 -
Attachment description: test.html → testcase 1 (make sure "layout.css.object-fit-and-position.enabled" is on)
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
This fixes it locally for me. Just need to add a reftest & a code comment or two, and then this is ready for review.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee: nobody → dholbert
Attachment #8524903 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8525032 -
Flags: review?(seth)
Assignee | ||
Comment 4•10 years ago
|
||
(Part 1 also rewraps some lines that are longer than 80 characters.)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8525033 -
Flags: review?(seth)
Assignee | ||
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e2c16144944
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
(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!
Comment hidden (obsolete) |
Assignee | ||
Comment 12•10 years ago
|
||
(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.)
Assignee | ||
Comment 13•10 years ago
|
||
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
Attachment #8525033 -
Attachment is obsolete: true
Attachment #8525690 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
Try run's looking good; landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/67e4a03c17f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb767f5730d
Flags: in-testsuite+
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/67e4a03c17f0 https://hg.mozilla.org/mozilla-central/rev/5cb767f5730d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•