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)
Core
Layout: Images, Video, and HTML Frames
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)
1.70 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
Details | Diff | Splinter Review | |
27.29 KB,
image/png
|
Details | |
28.58 KB,
image/png
|
Details | |
5.01 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
7.38 KB,
patch
|
Details | Diff | Splinter Review | |
248.15 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•9 years ago
|
Component: ImageLib → Layout: Images
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
(Here's a "diff -w" version of part 1, if it helps with review, since this part is largely indentation changes.)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Linux try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98546c30b035
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.
Updated•9 years ago
|
Attachment #8613336 -
Flags: review?(seth) → review+
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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.)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8613339 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8615433 -
Attachment description: part 2 v2: → part 2 v2: support object-fit/object-position on XUL <image src=""> elements
Assignee | ||
Comment 11•9 years ago
|
||
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 )
Assignee | ||
Comment 12•9 years ago
|
||
...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.)
Assignee | ||
Comment 13•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a693d75e9337
Comment 14•9 years ago
|
||
(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.
Assignee | ||
Comment 15•9 years ago
|
||
(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).
Comment 16•9 years ago
|
||
(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 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a9981cf858 https://hg.mozilla.org/integration/mozilla-inbound/rev/b84154e35c77 https://hg.mozilla.org/integration/mozilla-inbound/rev/629a920112ee https://hg.mozilla.org/integration/mozilla-inbound/rev/198a12989a34
Assignee | ||
Comment 20•9 years ago
|
||
Filed bug 1174270 on the "list-style-image" parts.
Backed out for mulet reftest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=10734975&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/c117126d24e7
Flags: needinfo?(dholbert)
Assignee | ||
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 23•9 years ago
|
||
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.)
Assignee | ||
Comment 24•9 years ago
|
||
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.
Assignee | ||
Comment 25•9 years ago
|
||
(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
Assignee | ||
Comment 26•9 years ago
|
||
...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.
Assignee | ||
Comment 28•9 years ago
|
||
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)
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bfdbc2580cc https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8498c4f3d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/65b38002bf7c https://hg.mozilla.org/integration/mozilla-inbound/rev/f59c0223e6bf https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eaa0ee5ced
Assignee | ||
Comment 30•9 years ago
|
||
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.
Assignee | ||
Comment 31•9 years ago
|
||
(For the record, comment 30's log-snippet is from https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/k30Xy0KrRMm8j71saB-YyQ/0/public/logs/live_backing.log )
Assignee | ||
Comment 32•9 years ago
|
||
Landed part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f43b87bd4e
Comment 34•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2bfdbc2580cc https://hg.mozilla.org/mozilla-central/rev/4b8498c4f3d8 https://hg.mozilla.org/mozilla-central/rev/65b38002bf7c https://hg.mozilla.org/mozilla-central/rev/f59c0223e6bf https://hg.mozilla.org/mozilla-central/rev/b9eaa0ee5ced https://hg.mozilla.org/mozilla-central/rev/d4f43b87bd4e https://hg.mozilla.org/mozilla-central/rev/42552a7bce74
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Updated•9 years ago
|
Summary: Support object-fit/object-position on <xul:image> → Support object-fit/object-position on <xul:image src="...">
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
•