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

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (make sure "layout.css.object-fit-and-position.enabled" is on)

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: wip

This fixes it locally for me. Just need to add a reftest & a code comment or two, and then this is ready for review.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1: move nsDisplayImage::GetOpaqueRegion to .cpp file

Comment 4

[Daniel Holbert [:dholbert]]

(Part 1 also rewraps some lines that are longer than 80 characters.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make nsDisplayImage::GetOpaqueRegion() take object-fit & object-position into account

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e2c16144944

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8525032 [details] [diff] [review]
part 1: move  nsDisplayImage::GetOpaqueRegion to .cpp file

Review of attachment 8525032 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 9

[Seth Fowler [:seth] [:s2h]]

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?

Comment 10

[Daniel Holbert [:dholbert]]

(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 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (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. :)

Actually, I'm not sure we *really* support fragmenting images these days. I couldn't get us to fragment a tall image -- I tried mulitcol & print-preview. Here's a simple testcase for trying print-preview:
data:text/html,<img style="width: 100px; height: 16in" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAICAIAAAB%2FFOjAAAAAJElEQVQY02NkYPjPgB0wYhVlYiAR0F4Dy%2F%2BGRuw%2BqGcYKn4AAG3kA5L%2BTFsxAAAAAElFTkSuQmCC">

So, it seems like the fragmented-image codepath may be impossible to test right now. :-/

Comment 12

[Daniel Holbert [:dholbert]]

(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.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: Make nsDisplayImage::GetOpaqueRegion() take object-fit & object-position into account

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

Comment 14

[Daniel Holbert [:dholbert]]

Try run's looking good; landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/67e4a03c17f0
 https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb767f5730d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67e4a03c17f0
https://hg.mozilla.org/mozilla-central/rev/5cb767f5730d