User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10 Build Identifier: Opera 10.60 has added support for object-fit. Testcase can be found here: http://testsuites.opera.com/object-fit/ and the link to the specification is here: http://dev.w3.org/csswg/css3-images/#object-fit Opera has support for <video>, <img>, <object>, <input type=image>, <svg>, <svg:image> and <svg:video>, but not for <embed> and <canvas>. Should there be another DOM property, beside width/height and naturalWidth/natrualHeight, like mozFitWidth/mozFitHeight to get the objects real width/height after object-fit is applied? Reproducible: Always

As far as I can see the property doesn't work in Opera 10.60, like an article at dev.opera.com said. But it works fine in Opera 11.

(In reply to comment #1) > As far as I can see the property doesn't work in Opera 10.60, like an article > at dev.opera.com said. But it works fine in Opera 11. Thank Fabian - you are right! I have ammended the article on dev.opera.com.

Note that Webkit are implementing it, too: https://bugs.webkit.org/show_bug.cgi?id=52040

Just a FYI about Chrome's progress on support for this; Chrome has added support for object-fit behind a runtime flag in https://src.chromium.org/viewvc/blink?revision=156535&view=revision ~6 weeks ago. Chrome has added support for object-position behind a runtime flag in https://src.chromium.org/viewvc/blink?revision=158155&view=revision ~12 days ago. It has also just started going through the "Intent to Ship" (http://www.chromium.org/blink#launch-process) process - https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/gnaAGjK_Ff4 ~5 hour ago.

In case it's helpful info in prioritizing implementation work, implementation object-fit would be very helpful to the Talkilla project.

More easily-readable (but undoubtedly less specific!) docs about these tags: <http://dev.opera.com/articles/view/css3-object-fit-object-position/>.

Want to support as soon as possible!

Current belief is that object-fit implementation for <video> will block the Loop MVP. If splitting that particular sub-chunk into its own bug will make it easier to get that sooner, that could be a fine strategy. It's possible that object-position for <video> will end up being helpful or even needed as for this too, but that's a bit hard for me to tell from here.

I'd really like to see these CSS properties (particularly object-fit) get added to Gecko as soon as possible. It would hlep me with my theme!

[“I have this use case” type of comment. Feel free to ignore.] The last website I designed used an object-fit–like behavior for images in a gallery and for a video trailer. I used CSS tricks for both, which requires somewhat complex code and hardcoding some values. Native support would be welcome (easier to use, more flexible and robust).

I guess the most common use-case is using object-fit for images. While there are use-cases for video, too, I’d say it’d be a good start if it’d be implemented for images with video to come later as Dan proposed. As author of the polyfill for object-fit (unfortunately with a Firefox bug atm (https://github.com/anselmh/object-fit/issues/5)) I can only say it’s an important CSS property that is very useful for web developers. If it helps, it’s implemented in Blink by now already.

Note that object-fit is now in Chrome stable (32+), and available to use.

Niko working with Song at TokBox on 1020445 - Loop layout in Google Chrome is broken. TokBox is taking the regressions, but also believe this bug could help the issue - so need to prioritize against stand-alone UI work.

I'm going to take a look at this in the next week or so.

Here's a reftest patch. The vast majority of this patch is auto-generated, using two scripts and two template files, which are included in the patch. So, you'll want to review the script & templates, and maybe look at just a few of the generated results. The relevant files to look at are in layout/reftests/image/images3/support. The template files are: template-object-fit-test.html template-object-fit-ref.html and the script that generates tests from them is: generate-object-fit-tests.sh I've got another script to separately generate <video src> tests, because those tests shouldn't be submitted to the w3c because they use WebM, which not everyone supports (or has to support). That script is: generate-object-fit-video-tests.sh These tests all live in layout/reftests/image/images3 right now, because they use "image-rendering:-moz-crisp-edges" for predictable (non-smearing) scaling behavior. Once we have "image-rendering: pixelated" implemented, we can move this directory to layout/reftests/w3c-css/submitted/images3. (except for the <video src> tests, as noted above.) (I'm using the folder-name "images3" because we've got that listed in reftests/w3c-css/submitted/reftest.list, commented-out, as a placeholder for when we add CSS3-images tests: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/reftest.list?rev=f79faac8a087&mark=28-29#28 The <video src> tests have 'fuzzy' annotations, which I've hand-added based on the amount of fuzziness on my local machine. VP8 is lossy, so those files don't perfectly match their PNG equivalents, but they're pretty close (as shown by the max difference of 3 in the fuzzy annotations, albeit with a lot of pixels; the color blocks are a very-slightly different color.) Also, the <video src> tests fail everywhere but on Linux, it seems, due to bug 1084564 and bug 1083516. This is unfortunate, but we've at least got test coverage on Linux.

For convenience, here's a hand-edited version of the reftest patch, with all of the generated reftest files removed except for a single test/reference pair. This may be easier to use for reviewing.

Try run with this reftest suite & nearly-ready patches for this feature: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=700021708472 (all tests passing, so far)

(In reply to Daniel Holbert [:dholbert] from comment #15) > These tests all live in layout/reftests/image/images3 right now, because > they use "image-rendering:-moz-crisp-edges" for predictable (non-smearing) > scaling behavior. Once we have "image-rendering: pixelated" implemented, we > can move this directory to layout/reftests/w3c-css/submitted/images3. > (except for the <video src> tests, as noted above.) Actually, dbaron says we can just directly put these tests in w3c-css/submitted/images3, and he's got a script that can automatically do the s/-moz-crisp-edges/pixelated/ substitution during his upload-to-w3c-repository process. (so that our prefixed style won't leak upstream) So, I'll just do that.

Here's the updated reftest patch, now putting the tests in the w3c-css directory. See comment 15 / comment 18 for background. (I've changed the colors in the helper-images, so that I can generate a lossless WebM video, without any rounding error from the RGB-->YUV conversion. WebM video script & tests are coming in a second reftest patch here.)

(...and for convenience, here's a version of the reftest patch with nearly all of the generated tests stripped out, except for a single test/reference pair.)

For reference, here's the shell script I used to generate the next patch -- the <video src> tests & resources in layout/reftests/webm-video/, from the main reftest patch's <video poster> tests.

Here's the second batch of reftests, for <video src> with WebM video. They use "hg cp" (as shown in the just-attached script), so Bugzilla's diff viewer may misleadingly show them as modifying other files in-place. I'm not bundling the test-generating script for these ones, since we're not submitting them upstream & there's no "support" directory equivalent where it would really belong. (I've attached it here for reference / usage, though.)

(In reply to Daniel Holbert [:dholbert] from comment #22) > I'm not bundling the test-generating script for these ones, since we're not > submitting them upstream & there's no "support" directory equivalent where > it would really belong. (I've attached it here for reference / usage, > though.) Still seems worth checking it in, though.

OK - I'm happy to add it to the patch & check it in; I just wasn't sure it was worth it, since it's really only intended to be used one time (to generate these probably-not-upstreamable WebM tests). (I definitely agree the other script is worth checking in, since it's in a folder that we're submitting upstream, & it's useful for other testers working with that upstream suite to see where the tests came from -- and those testers won't necessarily have the knowledge or resources to trace it back to this bug & to scripts attached here. That's not as true for these WebM tests, so the usefulness-vs.-clutter tradeoff is less obviously-positive for this second script, which is why I didn't initially include it; but I'll trust your judgement & add it in.)

--> Updated the "reftests, part 2" patch to include the shell script, per comment 23.

Comment on attachment 8509884 [details] [diff] [review] reftests, part 1, v2, *minimized* (just source material & one test/reference pair -- for review purposes) Review of attachment 8509884 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: layout/reftests/w3c-css/submitted/images3/support/generate-object-fit-tests.sh @@ +72,5 @@ > + cat $TEMPLATE_REFERENCE_FILENAME \ > + | sed "s,REPLACEME_IMAGE_FILENAME,$imageFile," \ > + | sed "s/REPLACEME_CONTAINER_WIDTH_VAL/$objWidth/" \ > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ Looks like you're doing this substitution more than once. If that's not merely an oversight, maybe using the 'g' substitution option would be a cleaner way to handler this. Micronit: The pattern separators aren't all the same. You use ',' for REPLACEME_IMAGE_FILENAME and '/' for the others. The same happens in the second block of sed commands below. @@ +74,5 @@ > + | sed "s/REPLACEME_CONTAINER_WIDTH_VAL/$objWidth/" \ > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > + | sed "s/REPLACEME_BACKGROUND_SIZE_VAL/$backgroundSizeEquiv/" \ > + | sed "s/\([ ]\+\)REPLACEME_SCALE_DOWN_EXTRA_RULE/$scaledownSmallRule/" \ I think you can avoid those backslashes if you use single quotes instead of double quotes. You probably don't want to tweak this file too much, since after all you're just using it to generate the tests, but FYI: another way of doing this that might end up cleaner would be to make |scaledownSmallRule| hold the entire sed command - either '/REPLACEME_SCALE_DOWN_EXTRA_RULE/d' to delete that line totally, if it's not needed, or 's/REPLACEME_SCALE_DOWN_EXTRA_RULE/.objectOuter > .small { background-size: contain; }' to insert the code that you want. An advantage of that approach is that you don't need to worry about the whitespace capture stuff at all. @@ +95,5 @@ > + | sed "s/REPLACEME_CONTAINER_WIDTH_VAL/$objWidth/" \ > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > + | sed "s/REPLACEME_CONTAINER_TAG/$tagName/" \ > + | sed "s,REPLACEME_CONTAINER_CLOSETAG,$tagCloseToken," \ > + | sed "s/src/$tagSrc/" \ No REPLACEME? =)

(In reply to Seth Fowler [:seth] from comment #26) > Looks like you're doing this substitution more than once. If that's not > merely an oversight, maybe using the 'g' substitution option would be a > cleaner way to handler this. Actually, do you use REPLACEME_CONTAINER_WIDTH_VAL and REPLACEME_CONTAINER_HEIGHT_VAL at all in the templates? I don't see them...

Comment on attachment 8509951 [details] [diff] [review] reftests, part 2 v2: test <video src> with WebM video (using 'hg cp' liberally) Review of attachment 8509951 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(In reply to Seth Fowler [:seth] from comment #26) > > + | sed "s/REPLACEME_CONTAINER_WIDTH_VAL/$objWidth/" \ > > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > > + | sed "s/REPLACEME_CONTAINER_HEIGHT_VAL/$objHeight/" \ > > Looks like you're doing this substitution more than once. If that's not > merely an oversight, maybe using the 'g' substitution option would be a > cleaner way to handler this. Oops, thanks! That was an oversight. And as you said in your subsequent comment, these WIDTH_VAL/HEIGHT_VAL aren't actually used in the templates at all. (I originally was going to use them to have "fat" & "skinny" reftest variants, but now those variants are just included in each test file.) Anyway -- fixed by dropping these WIDTH_VAL/HEIGHT_VAL lines. > Micronit: The pattern separators aren't all the same. You use ',' for > REPLACEME_IMAGE_FILENAME and '/' for the others. The same happens in the > second block of sed commands below. Yup -- there's a reason for this. I'm using "/" in most cases, because that's the standard for regexp substitutions. In the cases where I use a comma, it's because the substitution itself contains a "/" character (inside of a filename-variable), which I'd need to escape if I were using "/" as the regexp delimiter. So, I just picked a different character (",") as the delimiter in those cases, so that I don't need to bother with escaping. (I guess I could just *always* use a comma, but meh... s,foo,bar, looks weird, so I prefer to only resort to it when I have to. :)) > @@ +74,5 @@ > You probably don't want to tweak this file too much, since after all you're > just using it to generate the tests, but FYI: another way of doing this that > might end up cleaner would be to make |scaledownSmallRule| hold the entire > sed command - either '/REPLACEME_SCALE_DOWN_EXTRA_RULE/d' to delete that > line totally, if it's not needed, or Ah, that's handy -- I'll give that a try. I agree it'd be nicer to just delete that line. > 's/REPLACEME_SCALE_DOWN_EXTRA_RULE/.objectOuter > .small { background-size: > > + | sed "s,REPLACEME_CONTAINER_CLOSETAG,$tagCloseToken," \ > > + | sed "s/src/$tagSrc/" \ > > No REPLACEME? =) Ha, good point. I think I had this one as "src" in the templates since "src" is *usually* exactly what we want for this. But yeah, I should probably make that consistent & use a REPLACEME variable. I'll make that tweak.

Posting an updated version of "reftests, part 1". Changes from previous version: - The tests now have "png" in the filename (to distinguish them from forthcoming tests that use SVG images) - Addressed the things mentioned in comment 9. - Made a few minor tweaks/simplifications to the script. Carrying forward r+.

Here's an updated version of part 2, rebased on top of the updated version of part 1, & with a few minor script tweaks. For naming consistency, these WebM reftests now include "webm" in the filename -- e.g. "object-fit-contain-webm-001.html"

Here's a patch that's similar to "reftests, part 1", except with SVG images instead of PNG images. In particular, this patch has tests for three different types of SVG image: (a) an SVG image that essentially emulates the PNG image (fixed height & width, and preserveAspectRatio="none" to allow it to be stretched to fit arbitrary viewports) (b) an SVG image with *no* intrinsic size, but with an aspect ratio. (In practice, that just means that "object-fit:scale-down" and "object-fit:none" both behave like "object-fit:contain".) (c) an SVG image with the default preserveAspectRatio value ("xMidYMid meet"). (In practice, this means that "object-fit: fill" ends up rendering like "object-fit: contain", because we give the SVG the full content-box as its viewport, but the SVG content just scales its viewBox proportionally to meet that viewport's edges instead of filling it.) As with part 1, we use "background-size" / "background-position" in the reference cases, with the same (SVG) image as the testcase. I ran across one bug with our "background-size" impl, which breaks a few of the tests' reference cases -- I filed bug 1092436 on that, and annotated the failures with that bug number. (I believe the testcases render correctly in these cases, and the reference cases will too once bug 1092436 is fixed.)

Here's a minimized version of reftests part 3 (with only one of the generated testcases & reference cases, and the image flies and the script). Note that the script is generated via "hg cp" to copy the script from part 1, so it only shows the differences from the PNG script. (Also worth noting: the script adds a "sed" command to drop usages of "image-rendering" when generating SVG reftests from the template files, because we don't need "image-rendering" in the SVG reftests -- we get crisp upscaling/downscaling for free with SVG.)

Here's the first part of the fix. This adds a utility function, nsLayoutUtils::ComputeObjectRenderRect, which figures out the rect that should be rendered to, given the "object-fit" & "object-position" properties, along with the container size & the image/video/etc. intrinsic size & aspect-ratio. The next patch will add callers of this function. One known limitation of this patch: our helper-function "nsImageRenderer::ComputeObjectAnchorPoint()" (which handles "object-position" for us) gives us an anchor point, which we're supposed to make sure is pixel-aligned. Right now, we don't bother with this. I think this is only a problem if we're positioned with respect to the bottom-right corner, and it'll just mean we might be off by a fraction of a pixel, I think. I intend to handle this in a followup; I expect it might complicate things a bit, but hopefully not too much.

(Sorry, that was the wrong patch. Correct patch attached now.)

Here are the tweaks to replaced elements' frame classes to make them use ComputeObjectRenderRect. A few notable things: - I added an XXXdholbert comment in nsContainerFrame for something that I think might be broken, but I haven't caused it to trigger breakage yet. I'll file a followup bug on that (and/or poke around a bit more) before landing. - In current mozilla-central, nsImageFrame & nsVideoFrame use the flag DisplayListClipState::ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT in their display-list creation code -- i.e. they're passing up an opportunity to clip their content -- because they can be sure that their content will never try to be rendered outside of the container's content-box. This patch removes that assurance, because "object-fit" & "object-position" can scale and/or move the content such that it protrudes from the container's content-box. This protruding content needs to be clipped -- so, I've removed the DisplayListClipState::ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT flag from our display-list code in these classes, which means we'll now automatically clip the rendered content.

This third patch is something that we only need as long as we support the about:config pref for these properties -- layout.css.object-fit-and-position.enabled". (When we remove support for the pref, we can back this out.) I'll describe the problem that this patch addresses: So, <video> has to render with "object-fit:contain" behavior, by default. In current mozilla-central, that behavior is hardcoded into nsVideoFrame's layout code. The previous patch rips that out, and instead makes us rely on an "object-fit:contain" CSS rule that we added to html.css for <video> in bug 1065766. BUT, if the pref is disabled (as it is by default right now), we'll fail to parse that rule in html.css, so we won't know that we should use the "contain" behavior. (and instead, we'll render with the initial value, "object-fit:fill", which is incorrect default behavior for <video>.) This patch addresses this by having nsVideoFrame check if the pref is disabled; and if it is, we assume that we must've failed to parse html.css at startup time, and we *force* "object-fit: contain", using an additional flag that I've added to ComputeObjectRenderRect. This makes us *almost* handle dynamic pref tweaks correctly. There's only one case that won't work right: if the pref starts out disabled, and then is enabled at runtime (e.g. by the user), then we'll mis-render unstyled <video> elements for the rest of that session, until the user restarts their browser. (Specifically, we'll render unstyled <video> as if it had object-fit:fill.) I think this is an acceptable quirk, and I don't think we can really avoid it until bug 1068477 is fixed.

[CC'ing cpearce, as I might tag him to review (or at least give feedback on) the nsVideoFrame parts of 'fix, part 2' & 'fix, part 3'.]

Comment on attachment 8518550 [details] [diff] [review] fix, part 2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame. [setting review=cpearce for nsVideoFrame parts of this patch.]

Comment on attachment 8518550 [details] [diff] [review] fix, part 2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame. Review of attachment 8518550 [details] [diff] [review]: ----------------------------------------------------------------- I think this is more roc's cup of tea than mine...

Comment on attachment 8518550 [details] [diff] [review] fix, part 2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame. Review of attachment 8518550 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsContainerFrame.cpp @@ +717,5 @@ > } > > + // XXXdholbert Does this break <embed> with 'object-fit' / 'object-position'? > + // (Its view should be sized to a rect determined by those properties, not > + // to the frame's overflow area...) No it doesn't. nsPluginFrame manages the view for its plugin. ::: layout/generic/nsImageFrame.cpp @@ -1527,5 @@ > > DisplayBorderBackgroundOutline(aBuilder, aLists); > > DisplayListClipState::AutoClipContainingBlockDescendantsToContentBox > - clip(aBuilder, this, DisplayListClipState::ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT); Removing ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT is going to add overhead. Is it really true that object-fit and object-position can cause the imgae to overflow the content rect? If so, we should at least add some kind of check of object-fit/object-position around the setting of this flag. ::: layout/generic/nsVideoFrame.cpp @@ +291,5 @@ > + // anonymous poster-image element *inherit* these properties from the > + // <video> element. Then, we could just give the poster-image the same > + // size as the video frame, and trust it to size & position the image > + // data according to those inherited properties. (To do that, we'd need > + // a pseudoclass to let us style the poster image.) Why don't we just do the more elegant solution now? We should be able to style the poster image; we just added the (UA-sheet-only) "-moz-native-anonymous" pseudoclass to style anonymous content, so something like video > img:-moz-native-anonymous { ... } should work.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41) > Removing ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT is going to add overhead. > Is it really true that object-fit and object-position can cause the imgae to > overflow the content rect? Yes, depending on the value. For example: - "object-fit:cover" can easily give us an image-render-rect that's larger than the container element's content-box. (e.g. with a wide image and a square <img>) "object-fit:none" will also cause overflow (and require clipping), if the image's intrinsic size is larger than the <img> element. - object-position can shift around the render rect arbitrarily, pushing it partially or even entirely outside of the container element's content box. (e.g. "object-position: 500px 500px") For the default values, though, we can't overflow, so we should keep this optimization. I'll add a check, as you suggest. > ::: layout/generic/nsVideoFrame.cpp > Why don't we just do the more elegant solution now? We should be able to > style the poster image; we just added the (UA-sheet-only) > "-moz-native-anonymous" pseudoclass to style anonymous content, so something > like > video > img:-moz-native-anonymous { ... } > should work. Nice, thanks! With that pseudoclass, it is indeed that simple. (One caveat, though: that means we'll need a bit more hackery than what we've got in "fix part 3", to correctly render in sessions where this pref is enabled dynamically (e.g. during reftests), because those sessions won't have parsed this ua.css "inherit" rule. Bug 1068477 will remove this complication; depending on how quickly that lands, we might be able to just rely on that here, but for now I'm going to try to add hacks so that we don't need it, so that we can get this in.

Here's a separate patch that *only* makes us drop the ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT optimization if we're using an 'object-fit' and/or 'object-position' that can conceivably cause overflow. Notably, the default values of these properties *cannot* cause overflow, so this patch lets us keep the optimization (and skip clipping) by default.

Here's an updated version of part 2. Changed to address comment 41 -- in particular: - I dropped the XXXdholbert comment mentioned at the beginning of comment 41. - I dropped the ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT tweak (that's in "part 0" now, and is more intelligent) - I'm now styling poster images via a stylesheet, using the suggested selector from comment 41. I put this alongside the existing CSS rule for the default <video> object-fit value, in html.css. - I've implified nsVideoFrame's poster-image reflow code, given the above stylesheet tweak. Now, poster images will get the video's full content-box (or an empty box, if ShouldDisplayPoster() is false). Rather than having 2 reviewers, I'm just requesting review from roc on this updated version (instead of seth+roc), to reduce review-burden on seth -- hoping roc doesn't mind reviewing this full patch. (Feedback from seth/others is welcome, of course.)

Here's an updated version of "fix, part 3". I've updated this to handle the problem described at the end of comment 42, by forcing poster-images to use their parent nsStyleContext for determining "object-fit" and "object-position". (We can't be sure that we successfully parsed the "inherit" decl for these properties at startup time, but since we know it's got to be "inherit", we can just use the parent's style-struct.) We need this in order for reftests to work, because our reftest process starts with this pref turned off (for now) and enables it dynamically to run the tests. (Also, one correction to comment 42: bug 1068477 won't actually help us with this inheritance, because we need the inheritance to work even when the pref is disabled. We'll need this hack until we're ready to deprecate the "layout.css.object-fit-and-position.enabled" pref entirely.) (roc is probably a better reviewer on this patch, too, since this is video-related; hence, transferring review request to him. Thanks in advance! :))

Comment on attachment 8520994 [details] [diff] [review] fix, part 2 v2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame Review of attachment 8520994 [details] [diff] [review]: ----------------------------------------------------------------- Is it possible for object-fit and object-position to make painting happen outside the border-box? If so, then we need Reflow() methods (and probably UpdateOverflow()) to extend the overflow areas of the frame to include the actual painted area. And it's arguable whether this should affect the scrollable overflow area or just the visual overflow area. And we'll need reftests for this.

Comment on attachment 8521010 [details] [diff] [review] fix, part 3 v2: hack to handle about:config pref being off at startup Review of attachment 8521010 [details] [diff] [review]: ----------------------------------------------------------------- This seems a bit over the top! How about instead of dynamically setting the pref for nsVideoFrame tests, we just make the tests conditional on the pref being set. Seems like that would be good enough and avoid all this code.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46) > Is it possible for object-fit and object-position to make painting happen > outside the border-box? No. It's possible they'll inadvertently make us *try* to paint outside the border-box, but we need to prevent that painting with clipping (that's what this patch is for). See examples at beginning of comment 42. So I think we don't need to worry about updating the overflow areas. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47) > This seems a bit over the top! > > How about instead of dynamically setting the pref for nsVideoFrame tests, we > just make the tests conditional on the pref being set. Seems like that would > be good enough and avoid all this code. We'd still need most of this patch, in order for <video> / <video poster> to render correctly (with "contain" behavior) with the pref turned off. (since "part 2" is dropping the nsVideoFrame matrix-math, and makes us rely instead on the "object-fit:contain" rule in html.css, which we'll only see if the pref is enabled.) We could simplify this patch slightly, by dropping the "fake inheritance" and just relying on the eForceObjectFitContain hack for both <video> and poster-images. But the fake inheritance doesn't add significantly more complexity to this patch (on top of the eForceObjectFitContain hack), and it lets us actually test this stuff before we turn it on by default, which is useful. (I discussed this a bit with dbaron -- he said (and I agree) that we *really* should have a way to use pref-disabled properties in UA stylesheets, in a targeted way, for cases like this where a property is being added that lets us express old hardcoded behavior in terms of the new property. We don't have that yet, though, so for now we need some version of this hack.)

(In reply to Daniel Holbert [:dholbert] from comment #48) > we *really* should have a way to use pref-disabled properties in UA > stylesheets, in a targeted way (I filed bug 1097355 on this, BTW. In the meantime, I'm hoping comment 48 clarifies things a bit.)

Comment on attachment 8520994 [details] [diff] [review] fix, part 2 v2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame Review of attachment 8520994 [details] [diff] [review]: ----------------------------------------------------------------- OK!

Comment on attachment 8521010 [details] [diff] [review] fix, part 3 v2: hack to handle about:config pref being off at startup Review of attachment 8521010 [details] [diff] [review]: ----------------------------------------------------------------- OK

Thanks!

Comment on attachment 8521010 [details] [diff] [review] fix, part 3 v2: hack to handle about:config pref being off at startup Sorry -- per bug 1097355, I should be able to add CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS on these properties, which makes the hacks in "fix, part 3" unnecessary. (Haven't tested that yet, but it looks like exactly what we need here.) Apologies for the unnecessary review there (and hooray for less hackery!)

(I spun off bug 1097488 to add that flag, to keep this bug's patch-count from growing, & since that flag-addition can land ahead of everything else here.)

(In reply to Daniel Holbert [:dholbert] from comment #34) > One known limitation of this patch: our helper-function > "nsImageRenderer::ComputeObjectAnchorPoint()" (which handles > "object-position" for us) gives us an anchor point, which we're supposed to > make sure is pixel-aligned. Right now, we don't bother with this. [...] I intend to > handle this in a followup; I expect it might complicate things a > bit, but hopefully not too much. Filed followup bug 1098417 for this, BTW.

Comment on attachment 8517918 [details] [diff] [review] reftests, part 3, v1: *minimized* for review (Canceling review request on 'reftests part 3', to reduce review-load, since it's substantially similar to 'reftests part 1' -- its test-generating script is even generated from hg cp'ing the script in 'reftests part 1' [and making minor tweaks]. So, I don't think it really needs its own review pass.)

Comment on attachment 8518539 [details] [diff] [review] fix, part 1: Add ComputeObjectRenderRect helper-function Review of attachment 8518539 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! And nice use of Maybe. =) ::: layout/base/nsLayoutUtils.cpp @@ +3460,5 @@ > +static nscoord > +ComputeMissingDimension(const nsSize& aDefaultObjectSize, > + const nsSize& aIntrinsicRatio, > + Maybe<nscoord>& aSpecifiedWidth, > + Maybe<nscoord>& aSpecifiedHeight, These could be const, right? Doesn't look like you write to them. @@ +3474,5 @@ > + // Fill in the missing dimension using the intrinsic aspect ratio. > + nscoord knownDimensionSize; > + float ratio; > + if (aDimensionToCompute == eWidth) { > + knownDimensionSize = aSpecifiedHeight.value(); FYI: same as |*aSpecifiedHeight|. (Well, technically |*aSpecifiedHeight| returns a *reference*, but the result is the same in this case.) @@ +3512,5 @@ > + * > + * Per its final bulleted section: since there's no specified size, > + * we run the default sizing algorithm using the object's intrinsic size in > + * place of the specified size. But if the object has neither an intrinsic > + * height nor an inrinsic width, then we instead return without populating our inrinsic -> intrinsic @@ +3520,5 @@ > +static void > +MaybeComputeObjectFitNoneSize(const nsSize& aDefaultObjectSize, > + const IntrinsicSize& aIntrinsicSize, > + const nsSize& aIntrinsicRatio, > + Maybe<nsSize>& aNoneSizeResult) This function could just return a Maybe<nsSize>, right? @@ +3536,5 @@ > + if (aIntrinsicSize.height.GetUnit() == eStyleUnit_Coord) { > + specifiedHeight.emplace(aIntrinsicSize.height.GetCoordValue()); > + } > + > + if (specifiedWidth.isSome() || specifiedHeight.isSome()) { FYI, |if (specifiedWidth || specifiedHeight)| also works. @@ +3543,5 @@ > + // no valid ratio) using the default object size. > + aNoneSizeResult.emplace(); > + > + aNoneSizeResult.ref().width = specifiedWidth.isSome() ? > + specifiedWidth.value() : So I think this one deserves an upgrade from FYI to nit =) |aNoneSizeResult.ref().width| is the same as |aNoneSizeResult->width|, which IMO reads so much better that the former should be considered an antipattern. Back in FYI mode, you can write this whole thing as: |aNoneSizeResult->width = specifiedWidth ? *specifiedWidth : ComputeMissingDimension(...);| @@ +3549,5 @@ > + specifiedWidth, specifiedHeight, > + eWidth); > + > + aNoneSizeResult.ref().height = specifiedHeight.isSome() ? > + specifiedHeight.value() : (See the previous comment.) @@ +3573,5 @@ > + // (Also: if there's no valid intrinsic ratio, then we have the "fill" > + // behavior & just use the constraint size.) > + if (MOZ_LIKELY(aObjectFit == NS_STYLE_OBJECT_FIT_FILL) || > + aIntrinsicRatio.width == 0 || > + aIntrinsicRatio.height == 0) { You probably want MOZ_LIKELY around the entire condition, right? @@ +3585,5 @@ > + if (aObjectFit == NS_STYLE_OBJECT_FIT_NONE || > + aObjectFit == NS_STYLE_OBJECT_FIT_SCALE_DOWN) { > + MaybeComputeObjectFitNoneSize(aConstraintSize, aIntrinsicSize, > + aIntrinsicRatio, noneSize); > + if (noneSize.isNothing() || aObjectFit == NS_STYLE_OBJECT_FIT_SCALE_DOWN) { FYI, could be |!noneSize|. I'll stop telling you about boolean conversion opportunities now. =) @@ +3592,5 @@ > + fitType.emplace(nsImageRenderer::CONTAIN); > + } > + } > + > + if (aObjectFit == NS_STYLE_OBJECT_FIT_COVER) { Nit: make this |else if|. @@ +3619,5 @@ > + if (noneSize.isSome()) { > + return noneSize.value(); > + } > + MOZ_ASSERT(constrainedSize.isSome()); > + return constrainedSize.value(); FYI, you could write this whole block as: MOZ_ASSERT(noneSize || constrainedSize); return noneSize.valueOr(*constrainedSize); @@ +3629,5 @@ > + std::min(constrainedSize.ref().width, noneSize.ref().width); > + constrainedSize.ref().height = > + std::min(constrainedSize.ref().height, noneSize.ref().height); > + } > + return constrainedSize.value(); This could be a lot cleaner using the tips above (|.ref().| is the same as |->|, boolean conversion, |return *constrainedSize;|) @@ +3657,5 @@ > + &imageTopLeftPt, &imageAnchorPt); > + > + // XXXdholbert Probably need to be using or returning imageAnchorPt here (to > + // make sure we're fully snapped to the bottom and/or right edge, where > + // appropriate from specified style.) ISTR you're handling this in a followup, right? ::: layout/base/nsLayoutUtils.h @@ +1096,5 @@ > nsIFrame* aFrame, > uint32_t aFlags = 0); > > /** > + * Computes the rect that a given replaced element should render into, based It sounds like that's the same as the 'destination rect' we usually talk about for image drawing. If that's true, and 'render rect' is not an existing term, then maybe |ComputeObjectDestRect| would be a good alternative name. I'm not sure, though, because the rect you're returning here is in coordinates relative to the content-box, and the destination rect is usually specified in user space coordinates for some gfxContext.

(In reply to Seth Fowler [:seth] from comment #57) > Looks good! And nice use of Maybe. =) Thanks! I incorporated all of your FYI's & nits, except as noted below. > @@ +3573,5 @@ > > + // (Also: if there's no valid intrinsic ratio, then we have the "fill" > > + // behavior & just use the constraint size.) > > + if (MOZ_LIKELY(aObjectFit == NS_STYLE_OBJECT_FIT_FILL) || > > + aIntrinsicRatio.width == 0 || > > + aIntrinsicRatio.height == 0) { > > You probably want MOZ_LIKELY around the entire condition, right? I left this as-is, since (as discussed on IRC) the more-tightly-scoped MOZ_LIKELY seems to generate better assembly, and it's also more precise/human-readable. (since really only that first part is the "likely" part) > @@ +3619,5 @@ > > + if (noneSize.isSome()) { > > + return noneSize.value(); > > + } > > + MOZ_ASSERT(constrainedSize.isSome()); > > + return constrainedSize.value(); > > FYI, you could write this whole block as: > > MOZ_ASSERT(noneSize || constrainedSize); > return noneSize.valueOr(*constrainedSize); Per IRC, I can't quite do this -- we'd fail a fatal assertion in the case where noneSize is populated & constrainedSize is not-populated, since your sample-code requires that we unconditionally dereference "*constrainedSize" so that we can pass it as a arg to valueOr. So, I'm leaving this part as-is (except with boolean conversions and *'s, as you suggest elsewhere). > ISTR you're handling this in a followup, right? Yes -- bug 1098417. I've now updated this code-comment to include a mention of that bug. > ::: layout/base/nsLayoutUtils.h > > > > /** > > + * Computes the rect that a given replaced element should render into, based > > It sounds like that's the same as the 'destination rect' we usually talk > about for image drawing. If that's true, and 'render rect' is not an > existing term, then maybe |ComputeObjectDestRect| would be a good > alternative name. I'm not sure, though, because the rect you're returning > here is in coordinates relative to the content-box, and the destination rect > is usually specified in user space coordinates for some gfxContext. I agree that it'd be better to avoid implicitly introducing an additional named rect concept here. I believe the rect that we return *is* the "destination rect", though it's just missing an offset (which right now, the clients add after calling ComputeObjectRenderRect). I'm going to change it so that clients *pass in* this offset, and then we can add it under the hood and truly return the "destination rect".

(To be clear, the "destination rect" whose meaning I'm wanting to match is the "nsRect& aDest" parameter in the nsLayoutUtils Draw*Image methods. There's also a completely different "DestRect" usage, which is a gfxRect in device pixels -- I filed bug 1099314 to rename that one, to clear up ambiguity.)

(In reply to Daniel Holbert [:dholbert] from comment #58) > I believe the rect that we return *is* the "destination rect", though it's > just missing an offset (which right now, the clients add after calling > ComputeObjectRenderRect). I'm going to change it so that clients *pass in* > this offset, and then we can add it under the hood and truly return the > "destination rect". FWIW: I addressed this by having the clients pass in a constraint-nsRect instead of a constraint-nsSize. (So in the default case with "object-fit:fill", we'll just return the same rect as the passed-in dest-rect.) Try run with that change: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e81ebdf77b33

Try run isn't quite complete, but based on its completed parts & earlier try coverage, I'm confident that this passes tests. So, landed: fix part 0: https://hg.mozilla.org/integration/mozilla-inbound/rev/a083cd9a7c8c fix part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/39a32a0978f5 fix part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/466d3ff030e6 reftests part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/91b713785458 reftests part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/270fc475e29c reftests part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd01637b2ef (Note that these features are turned off by default, for now -- I filed bug 1099450 on enabling the pref to turn them on.)

https://hg.mozilla.org/mozilla-central/rev/a083cd9a7c8c https://hg.mozilla.org/mozilla-central/rev/39a32a0978f5 https://hg.mozilla.org/mozilla-central/rev/466d3ff030e6 https://hg.mozilla.org/mozilla-central/rev/91b713785458 https://hg.mozilla.org/mozilla-central/rev/270fc475e29c https://hg.mozilla.org/mozilla-central/rev/3cd01637b2ef

Regarding docs, we will triage these next week. Meanwhile if somebody wants to help, the two pages to create are https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit and https://developer.mozilla.org/en-US/docs/Web/CSS/object-position and the document helping you to write them: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Document_a_CSS_property

In addition to that, you might get some documentation from the WPD which have covered the `object-fit` property already: https://docs.webplatform.org/wiki/css/properties/object-fit

(In reply to Daniel Holbert [:dholbert] from comment #60) > FWIW: I addressed this by having the clients pass in a constraint-nsRect > instead of a constraint-nsSize. Note: I realized that this late-breaking change was a bit clumsy in nsVideoFrame::BuildLayer -- it ended up resurrecting a local-var that my patch was removing (but under a new name, and in a roundabout way). I pushed a followup to restore the original local-var ("nsRect area") and remove the roundabout recreation of it: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d18de1eb6b3

I added documentation for both properties. I accidentally additionally added https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/object-position. Can somebody delete that page? I don't seem to have the rights to do that. (I turned it into a redirection page for now.) Sebastian

Keeping it as a redirect is perfectly fine. Thanks for the nice work. https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS is also up-to-date.

Thank you, Sebastian!

You're welcome! Sebastian

This bug needs to be re-opened. The fix doesn't seem to apply to xul:image, which *is* replaced content. *ALL* replaced content should be affected by this bugfix.

The CSS spec says that object fit should apply to all "replaced elements": http://dev.w3.org/csswg/css-images-3/#the-object-fit A xul:image whose source is a raster image is one such element.

Unless this is backed out (and it is unlikely), the bug won't be reopened. You should file a new bug with this specific problem and mark it as blocking this bug.

If you think it should apply to xul:image, that's a separate bug. This bug covered everything that's a Web standard or standards-track feature. It's not even necessarily clear to me that xul:image is a replaced element.

Why wouldn't it be a replaced element? You have a raster image that replaces the xul:image. I'm failing to see the confusion here.

If you want to discuss it further, file a separate bug.