Closed Bug 1170052 Opened 9 years ago Closed 9 years ago

Support object-fit/object-position on <xul:image src="...">

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Spun up some patches over the weekend for getting object-fit/object-position working on XUL <image> element.

(Filing this separately from bug 1094609 (treating that bug as a meta-bug) since I haven't fixed SVG <image> to support these properties yet, but I don't think we should gate XUL <image> on that.)
Component: ImageLib → Layout: Images
No functional changes in this patch; just making control flow easier to follow by handling a failure case in an early-return, instead of handling the normal case in an early-return.
Attachment #8613336 - Flags: review?(seth)
(Here's a "diff -w" version of part 1, if it helps with review, since this part is largely indentation changes.)
Haven't tested this thoroughly yet -- just tried "object-fit:contain" & "cover" on a XUL <image> locally, and things seem to be working correctly.

The code added here is similar to the lead-up to calling DrawSingleImage in nsImageFrame::PaintImage. (And the BuildDisplayList clipping tweak is the same one we use in nsImageFrame & nsVideoFrame.)
Attachment #8613339 - Flags: feedback?(seth)
Patches parts 1 and 2 applied to latest SeaMonkey x64, and it seems to work (renders object-fit:none correctly) - see MonkeyFix icon in before and after screenshots.
Attached image Before
Attached image After
Attachment #8613336 - Flags: review?(seth) → review+
Comment on attachment 8613339 [details] [diff] [review]
part 2: compute & use dest rect / anchor point when painting XUL images (and clip if object-fit/position might make us overflow)

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

Looks good!

::: layout/xul/nsImageBoxFrame.cpp
@@ +311,5 @@
> +  uint32_t clipFlags =
> +    nsStyleUtil::ObjectPropsMightCauseOverflow(StylePosition()) ?
> +    0 : DisplayListClipState::ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT;
> +
> +  DisplayListClipState::AutoClipContainingBlockDescendantsToContentBox

Wow, sometimes it's amazing to me that we've been able to make 80 columns work for this long. =)
Attachment #8613339 - Flags: feedback?(seth) → feedback+
So, one bit of nuance here, RE whether XUL <image> counts as a replaced element:

The definition of replaced element is:
 # An element whose content is outside the scope of
 # the CSS formatting model, such as an image, embedded
 # document, or applet. For example, the content of the
 # HTML IMG element is often replaced by the image that
 # its "src" attribute designates.
http://www.w3.org/TR/CSS21/conform.html#replaced-element

So, firstly: <image src="whatever.png"> clearly fits this qualification.

But, I'm not as sure about <image style="list-style-image:url('whatever.png')">. There, the content is arguably *determined by CSS*, in the same way that a div's background-image is determined by CSS.  And a div with a background-image doesn't count as a replaced element, so perhaps an <image> with list-style-image shouldn't either.

(I'll bet this is what dbaron was hinting at in bug 624647 comment 74, when he said "It's not even necessarily clear to me that xul:image is a replaced element.")

But on the other hand, it seems to me that XUL is kind of abusing CSS when it makes the "list-style-image" property set the contents of <image> -- the CSS spec doesn't *really* say how XUL <image> with a "list-style-image" renders.  XUL is just co-opting that property as a convenient storage place for image URLs, IIUC.

Anyway -- regardless, the CSS case would be a bit more complicated to support (and needs more comprehensive testing) because you can combine a list-style-image with "-moz-image-region(...)" to select out arbitrary sub-regions of larger images.  (And from some testing, it seems authors can provide e.g. negative-width rects, which the XUL <image> rendering code happily accepts, and which confuses our calculations for object-fit/object-position. So that may need some cleanup if we're going to allow subrects with these properties.)

So for now, I'm planning on fixing the simple <image src="..."> case first, and then perhaps fixing the more complicated image-determined-by-CSS(maybe-with-a-subrect) case later on.

(Not sure if this makes this less useful for the seamonkey use-case; I suspect it might.)
Per previous comment, this updated version of "part 2" only calls nsLayoutUtils::ComputeObjectDestRect (to apply object-[fit|position]) if we're getting our image URL from the src attribute (<image src="...">).

It also now handles cases where we have an intrinsic aspect ratio but no intrinsic size. (e.g. SVG images with a viewBox but no height/width attributes on root SVG node.)
Attachment #8615433 - Flags: review?(seth)
Attachment #8613339 - Attachment is obsolete: true
Attachment #8615433 - Attachment description: part 2 v2: → part 2 v2: support object-fit/object-position on XUL <image src=""> elements
Here are some shell scripts which generate reftests here, as copies of the existing <img> reftests in our w3c-submitted tests.

Not requesting review on these, but feedback is welcome.

(These scripts are modified/simplified copies of these similar scripts in the video reftests directory:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/webm-video/generate-object-fit-video-tests.sh
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/webm-video/generate-object-position-video-tests.sh
)
...and here are the generated tests themselves.

(Note that I've annotated 2 SVG tests as failing due to their references mis-rendering because of bug 1092436. We have the same problem for the corresponding <img> reftests.)
(In reply to Daniel Holbert [:dholbert] from comment #9)
> So, one bit of nuance here, RE whether XUL <image> counts as a replaced
> element:
> 
> The definition of replaced element is:
>  # An element whose content is outside the scope of
>  # the CSS formatting model, such as an image, embedded
>  # document, or applet. For example, the content of the
>  # HTML IMG element is often replaced by the image that
>  # its "src" attribute designates.
> http://www.w3.org/TR/CSS21/conform.html#replaced-element
> 
> So, firstly: <image src="whatever.png"> clearly fits this qualification.
> 
> But, I'm not as sure about <image
> style="list-style-image:url('whatever.png')">. There, the content is
> arguably *determined by CSS*, in the same way that a div's background-image
> is determined by CSS.  And a div with a background-image doesn't count as a
> replaced element, so perhaps an <image> with list-style-image shouldn't
> either.

First, not supporting list-style-image makes the patch virtually useless.  That style is used all over the place (especially in extensions) to specify XUL images.  It makes theming much easier (well, possible, in fact).

Second, the spec says "An element whose content is outside the scope of the CSS formatting model, such as an image".  An image.  xul:image is an image, no matter what determines its source URL.

> (I'll bet this is what dbaron was hinting at in bug 624647 comment 74, when
> he said "It's not even necessarily clear to me that xul:image is a replaced
> element.")

This is nonsense.  I don't see how an image can be anything but a replaced element.  What is causing its content to appear if not the image it is replaced with?

> But on the other hand, it seems to me that XUL is kind of abusing CSS when
> it makes the "list-style-image" property set the contents of <image> -- the
> CSS spec doesn't *really* say how XUL <image> with a "list-style-image"
> renders.  XUL is just co-opting that property as a convenient storage place
> for image URLs, IIUC.

Seems fine to me, and allows theming.

> Anyway -- regardless, the CSS case would be a bit more complicated to
> support (and needs more comprehensive testing) because you can combine a
> list-style-image with "-moz-image-region(...)" to select out arbitrary
> sub-regions of larger images.  (And from some testing, it seems authors can
> provide e.g. negative-width rects, which the XUL <image> rendering code
> happily accepts, and which confuses our calculations for
> object-fit/object-position. So that may need some cleanup if we're going to
> allow subrects with these properties.)
> 
> So for now, I'm planning on fixing the simple <image src="..."> case first,
> and then perhaps fixing the more complicated
> image-determined-by-CSS(maybe-with-a-subrect) case later on.

Noooooooooo, please don't do this.  xul:image's with list-style-image determining the image source MUST be supported.  It is very commonly used by themes and extensions, and not supporting it would be a major flaw.

> (Not sure if this makes this less useful for the seamonkey use-case; I
> suspect it might.)

Makes it virtually useless.  Please don't leave out this functionality.
(In reply to Jeremy Morton from comment #14)
> Second, the spec says "An element whose content is outside the scope of the
> CSS formatting model, such as an image".  An image.  xul:image is an image,
> no matter what determines its source URL.

So then, should we also apply object-fit & object-position to the CSS background of a XUL <image> as well, then?  (e.g. if I just have <image style="background: foo.png">)?  What about its border-image? Those are both ways of making the element paint an image, but clearly object-fit/object-position shouldn't apply to them.

There's some nuance here, with list-style-image being a CSS property.

> > But on the other hand, it seems to me that XUL is kind of abusing CSS when
> > it makes the "list-style-image" property set the contents of <image>
> 
> Seems fine to me, and allows theming.

(My point here was actually agreeing with you -- "list-style-image" isn't so much CSS controlling how the image renders, as XUL co-opting a CSS property to provide an alternate storage location for the "src" attribute. Which maybe means "list-style-image" should be counted as a source for the replaced image content.) 

> Noooooooooo, please don't do this.  xul:image's with list-style-image
> determining the image source MUST be supported.  It is very commonly used by
> themes and extensions, and not supporting it would be a major flaw.

To be clear, I'm not necessarily saying we won't support this.

I'm just deferring it for the moment, because of (a) minor concerns about "replaced" definition (which I want to sanity-check with dbaron), and (b) the need to write more robust automated tests to ensure this works (and to make sure that edge cases like e.g. negative-sized -moz-image-region won't make us blow up).
(In reply to Daniel Holbert [:dholbert] from comment #15)
> I'm just deferring it for the moment, because of (a) minor concerns about
> "replaced" definition (which I want to sanity-check with dbaron), and (b)
> the need to write more robust automated tests to ensure this works (and to
> make sure that edge cases like e.g. negative-sized -moz-image-region won't
> make us blow up).

-moz-image-region is implemented using ClippedImage IIRC. The ClippedImage code shouldn't blow up, even if it encounters a negative-sized rect, because we intersect the clipping region with the size of the underlying image, and BaseRect::Intersect always produces a non-negative width and height. You could probably take the same approach.
Comment on attachment 8615433 [details] [diff] [review]
part 2 v2: support object-fit/object-position on XUL <image src=""> elements

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

LGTM. I do concur that it seems like somewhat of a shame to lose list-style-image support here.

::: layout/xul/nsImageBoxFrame.cpp
@@ +394,5 @@
>             *aRenderingContext.ThebesContext(),
>             PresContext(), imgCon,
>             nsLayoutUtils::GetGraphicsFilterForFrame(this),
> +           dest, dirty, nullptr, aFlags,
> +           anchorPoint.isSome() ? anchorPoint.ptr() : nullptr,

You can just write |anchorPoint.ptrOr(nullptr)| here.
Attachment #8615433 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] - PTO until 6/12 from comment #16)
> -moz-image-region is implemented using ClippedImage IIRC. The ClippedImage
> code shouldn't blow up, even if it encounters a negative-sized rect,

When testing, I triggered a fatal (in debug builds) assertion somewhere in nsLayoutUtils, with a negative-sized -moz-image-region (I think as part of ComputeObjectDestRect), due to having a negative value in the intrinsic size or aspect ratio.

Good to know the ClippedImage should be OK, though. In any case, this scenario is something we should handle earlier than that (before we start doing math with a negative intrinsic size/aspect ratio), before/when we support list-style-image.
Blocks: 1174270
Filed bug 1174270 on the "list-style-image" parts.
The test-failures in comment 21 are:
{
12002 19:24:18 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/build/tests/reftest/tests/layout/reftests/svg/conditions-05.svg | image comparison (==), max difference: 102, number of differing pixels: 80
12104 19:24:20 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/build/tests/reftest/tests/layout/reftests/svg/dynamic-conditions-02.svg | image comparison (==), max difference: 102, number of differing pixels: 80
12123 19:24:20 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/build/tests/reftest/tests/layout/reftests/svg/dynamic-conditions-04.svg | image comparison (==), max difference: 102, number of differing pixels: 80
}
...and the failure is in the positioning of a scrollbar. (slightly higher in testcase as compared to reference case.

I don't immediately see how this bug could cause that issue; I suspect it might be intermittent & unrelated to my changes. Retriggering test to get more data.
Hmm, all the retriggers had the same issue. That scrollbar must be painted as a XUL image, I guess, and this bug's patches must be affecting how it's drawn.

(If it turns out that this <image> has "object-fit" or "object-position" set on it, then this change may be expected [and we may want to drop that styling]. If not, though, then it points to an actual bug in the patches here -- or a bug in ComputeObjectDestRect, which the patches here use.)
The reference case for these tests is "about:blank", BTW. (and the testcase has some content which should not be displayed)

We have several other reftests later on which use "about:blank" as their reference case, which still pass.

Also: I just notice that my push changed the test-chunking, shifting these reftests from R6 to R5. So one possible explanation is that there's just a test-chunking problem here, where these tests fail due to a bad interaction with some earlier test in R5. I'll do some Try runs to test that theory.
(In reply to Daniel Holbert [:dholbert] from comment #24)
> Also: I just notice that my push changed the test-chunking, shifting these
> reftests from R6 to R5. So one possible explanation is that there's just a
> test-chunking problem here, where these tests fail due to a bad interaction
> with some earlier test in R5. I'll do some Try runs to test that theory.

This seems to be exactly what's going on -- a Try push with the code changes but without the new tests has no issues on Mulet reftests:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=815f4e161682
...and a separate Try push that has the code changes *and* the tests triggers the R5 failure from comment 21: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b72c142bfa

So, definitely a test-chunking issue. :-/
Guess you could probably disable the new tests on mulet to keep the chunks the same.
I added a new patch to do that, & that does indeed seem to fix things:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf6be91353b

As a sanity-check: I verified locally that these new tests are still running on my non-Mulet machine, too (during ./mach reftest layout/reftests/xul/).

So, re-pushing with a "part 5" that disables these tests on Mulet.
Flags: needinfo?(dholbert)
These fail on B2G emulator as well (in a way that TreeHerder reports as a harness timeout), because we apparently don't support XUL in reftest runs on that platform:
{
20:52:27     INFO -  06-13 03:35:43.440 I/Gecko   (  714): REFTEST TEST-KNOWN-FAIL | http://10.0.2.2:8888/tests/layout/reftests/xul/text-small-caps-1.xul | (SKIP)
20:52:27     INFO -  06-13 03:35:43.440 I/Gecko   (  714): REFTEST TEST-START | http://10.0.2.2:8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul
20:52:27     INFO -  06-13 03:35:43.469 I/Gecko   (  714): REFTEST TEST-LOAD | http://10.0.2.2:8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul | 610 / 635 (96%)
20:52:27     INFO -  06-13 03:35:44.288 I/Gecko   (  823): ############ ErrorPage.js
20:52:27     INFO -  06-13 03:35:46.369 E/Test Container(  823): Content JS ERROR: net-error
20:52:27     INFO -  06-13 03:35:46.369 E/Test Container(  823):     at initPage (about:neterror?e=remoteXUL&u=http%3A//10.0.2.2%3A8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul&c=UTF-8&f=app&m=app%3A//test-container.gaiamobile.org/manifest.webapp&d=This%20page%20uses%20an%20unsupported%20technology%20that%20is%20no%20longer%20available%20by%20default%20in%20Firefox.:1320:7002)
20:52:27     INFO -  06-13 03:35:46.369 E/Test Container(  823):     at ee_emit (about:neterror?e=remoteXUL&u=http%3A//10.0.2.2%3A8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul&c=UTF-8&f=app&m=app%3A//test-container.gaiamobile.org/manifest.webapp&d=This%20page%20uses%20an%20unsupported%20technology%20that%20is%20no%20longer%20available%20by%20default%20in%20Firefox.:1013:89)
20:52:27     INFO -  06-13 03:35:46.369 E/Test Container(  823):     at setReady (about:neterror?e=remoteXUL&u=http%3A//10.0.2.2%3A8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul&c=UTF-8&f=app&m=app%3A//test-container.gaiamobile.org/manifest.webapp&d=This%20page%20uses%20an%20unsupported%20technology%20that%20is%20no%20longer%20available%20by%20default%20in%20Firefox.:1223:80)
20:52:27     INFO -  06-13 03:35:46.369 E/Test Container(  823):     at
20:52:27     INFO -  06-13 03:40:43.688 I/Gecko   (  714): REFTEST TEST-UNEXPECTED-FAIL | load failed with unknown reason
20:52:27     INFO -  06-13 03:40:43.688 I/Gecko   (  714): REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/xul/object-fit-contain-png-001.xul | load failed: null
20
}

I guess this explains why every other reftest in this directory is marked as being skipped on B2G. :)

I'll push a "part 6" to broaden the existing skip-if() to also skip B2G emulator.
Summary: Support object-fit/object-position on <xul:image> → Support object-fit/object-position on <xul:image src="...">
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: