Closed Bug 1101128 Opened 6 years ago Closed 6 years ago

Image background not painted, with "object-fit:none" and larger <img> element

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch wip (obsolete) — Splinter Review
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: nobody → dholbert
Attachment #8524903 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8525032 - Flags: review?(seth)
(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
Attachment #8525033 - Attachment is obsolete: true
Attachment #8525690 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/67e4a03c17f0
https://hg.mozilla.org/mozilla-central/rev/5cb767f5730d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.