Last Comment Bug 624647 - [css3-images] Implement object-fit and object-position CSS properties
: [css3-images] Implement object-fit and object-position CSS properties
Status: RESOLVED FIXED
[parity-chrome]
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 enhancement with 24 votes (vote)
: mozilla36
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css3-images/#...
Depends on: 1092436 1055285 1065766 1084500 1097488 1101128 1124043 1125572
Blocks: css3test 1094609 1211717 1008935 1014571 1092378 1095153 1098417 1099450 1103184 1110950 1200611
  Show dependency treegraph
 
Reported: 2011-01-10 23:49 PST by Fabian (Crash) Grutschus
Modified: 2015-10-13 12:53 PDT (History)
39 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reftest patch, v1 (208.51 KB, patch)
2014-10-17 15:34 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
minimized reftest patch, v1 (for review purposes) (27.58 KB, patch)
2014-10-17 15:39 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
reftests, part 1, v2: test <embed>, <img>, <object>, <video poster> (173.30 KB, patch)
2014-10-22 15:52 PDT, Daniel Holbert [:dholbert]
seth.bugzilla: review+
Details | Diff | Splinter Review
reftests, part 1, v2, *minimized* (just source material & one test/reference pair -- for review purposes) (22.71 KB, patch)
2014-10-22 15:56 PDT, Daniel Holbert [:dholbert]
seth.bugzilla: review+
Details | Diff | Splinter Review
shell script to generate WebM <video src> reftests from <video poster> reftests (2.84 KB, text/plain)
2014-10-22 16:28 PDT, Daniel Holbert [:dholbert]
no flags Details
reftests, part 2: test <video src> with WebM video (using 'hg cp' liberally) (49.65 KB, patch)
2014-10-22 16:33 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
reftests, part 2 v2: test <video src> with WebM video (using 'hg cp' liberally) (52.88 KB, patch)
2014-10-22 17:03 PDT, Daniel Holbert [:dholbert]
seth.bugzilla: review+
Details | Diff | Splinter Review
reftests, part 1, v3: test <embed>, <img>, <object>, <video poster> with PNG image (175.57 KB, patch)
2014-11-05 15:29 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
reftests, part 2 v3: test <video src> with WebM video (65.03 KB, patch)
2014-11-05 15:41 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
reftests, part 3, v1: test <embed>, <img>, <object>, <video poster> with SVG images (503.54 KB, patch)
2014-11-05 21:23 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
reftests, part 3, v1: *minimized* for review (26.47 KB, patch)
2014-11-05 21:31 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
(wrong patch) (18.83 KB, patch)
2014-11-06 15:31 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix, part 1: Add ComputeObjectRenderRect helper-function (13.55 KB, patch)
2014-11-06 15:32 PST, Daniel Holbert [:dholbert]
seth.bugzilla: review+
Details | Diff | Splinter Review
fix, part 2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame. (18.82 KB, patch)
2014-11-06 15:43 PST, Daniel Holbert [:dholbert]
roc: review-
Details | Diff | Splinter Review
fix, part 3: Temporary hack: if about:config pref is disabled, force nsVideoFrame to render with "object-fit: contain" behavior (8.27 KB, patch)
2014-11-06 16:02 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix, part 0: Don't pass ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT if overflow might happen (9.29 KB, patch)
2014-11-11 14:18 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
fix, part 2 v2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame (16.08 KB, patch)
2014-11-11 15:19 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
fix, part 3 v2: hack to handle about:config pref being off at startup (13.21 KB, patch)
2014-11-11 15:34 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Fabian (Crash) Grutschus 2011-01-10 23:49:52 PST
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
Comment 1 Fabian (Crash) Grutschus 2011-01-11 00:08:30 PST
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.
Comment 2 Chris Mills (Opera) 2011-01-12 02:28:49 PST
(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.
Comment 3 Silvia Pfeiffer 2011-04-11 22:47:28 PDT
Note that Webkit are implementing it, too: https://bugs.webkit.org/show_bug.cgi?id=52040
Comment 4 Tim 'mithro' Ansell 2013-10-03 06:01:32 PDT
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.
Comment 5 Dan Mosedale (:dmose) 2013-10-03 10:57:06 PDT
In case it's helpful info in prioritizing implementation work, implementation object-fit would be very helpful to the Talkilla project.
Comment 6 Dan Mosedale (:dmose) 2013-10-07 14:22:50 PDT
More easily-readable (but undoubtedly less specific!) docs about these tags: <http://dev.opera.com/articles/view/css3-object-fit-object-position/>.
Comment 7 yiorsi 2013-12-09 23:06:31 PST
Want to support as soon as possible!
Comment 8 Dan Mosedale (:dmose) 2014-02-20 20:36:43 PST
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.
Comment 9 Jeremy Morton 2014-03-29 09:03:58 PDT
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!
Comment 10 Florens Verschelde 2014-03-29 11:53:58 PDT
[“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).
Comment 11 info 2014-03-30 03:36:19 PDT
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.
Comment 12 jonkoops 2014-07-08 01:25:09 PDT
Note that object-fit is now in Chrome stable (32+), and available to use.
Comment 13 :shell escalante 2014-07-17 18:44:48 PDT
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.
Comment 14 Daniel Holbert [:dholbert] 2014-08-14 17:04:35 PDT
I'm going to take a look at this in the next week or so.
Comment 15 Daniel Holbert [:dholbert] 2014-10-17 15:34:44 PDT
Created attachment 8507221 [details] [diff] [review]
reftest patch, v1

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.
Comment 16 Daniel Holbert [:dholbert] 2014-10-17 15:39:23 PDT
Created attachment 8507227 [details] [diff] [review]
minimized reftest patch, v1 (for review purposes)

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.
Comment 17 Daniel Holbert [:dholbert] 2014-10-17 15:46:39 PDT
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)
Comment 18 Daniel Holbert [:dholbert] 2014-10-17 16:00:00 PDT
(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.
Comment 19 Daniel Holbert [:dholbert] 2014-10-22 15:52:28 PDT
Created attachment 8509881 [details] [diff] [review]
reftests, part 1, v2: test <embed>, <img>, <object>, <video poster>

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.)
Comment 20 Daniel Holbert [:dholbert] 2014-10-22 15:56:02 PDT
Created attachment 8509884 [details] [diff] [review]
reftests, part 1, v2, *minimized* (just source material & one test/reference pair -- for review purposes)

(...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.)
Comment 21 Daniel Holbert [:dholbert] 2014-10-22 16:28:03 PDT
Created attachment 8509917 [details]
shell script to generate WebM <video src> reftests from <video poster> reftests

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.
Comment 22 Daniel Holbert [:dholbert] 2014-10-22 16:33:33 PDT
Created attachment 8509927 [details] [diff] [review]
reftests, part 2: test <video src> with WebM video (using 'hg cp' liberally)

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.)
Comment 23 David Baron :dbaron: ⌚️UTC-8 2014-10-22 16:42:44 PDT
(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.
Comment 24 Daniel Holbert [:dholbert] 2014-10-22 17:01:20 PDT
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.)
Comment 25 Daniel Holbert [:dholbert] 2014-10-22 17:03:27 PDT
Created attachment 8509951 [details] [diff] [review]
reftests, part 2 v2: test <video src> with WebM video (using 'hg cp' liberally)

--> Updated the "reftests, part 2" patch to include the shell script, per comment 23.
Comment 26 Seth Fowler [:seth] [:s2h] 2014-10-28 16:15:56 PDT
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? =)
Comment 27 Seth Fowler [:seth] [:s2h] 2014-10-28 16:19:15 PDT
(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 28 Seth Fowler [:seth] [:s2h] 2014-10-28 16:26:07 PDT
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!
Comment 29 Daniel Holbert [:dholbert] 2014-10-28 17:58:06 PDT
(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.
Comment 30 Daniel Holbert [:dholbert] 2014-11-05 15:29:40 PST
Created attachment 8517776 [details] [diff] [review]
reftests, part 1, v3: test <embed>, <img>, <object>, <video poster> with PNG image

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+.
Comment 31 Daniel Holbert [:dholbert] 2014-11-05 15:41:29 PST
Created attachment 8517785 [details] [diff] [review]
reftests, part 2 v3: test <video src> with WebM video

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"
Comment 32 Daniel Holbert [:dholbert] 2014-11-05 21:23:21 PST
Created attachment 8517916 [details] [diff] [review]
reftests, part 3, v1: test <embed>, <img>, <object>, <video poster> with SVG images

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.)
Comment 33 Daniel Holbert [:dholbert] 2014-11-05 21:31:08 PST
Created attachment 8517918 [details] [diff] [review]
reftests, part 3, v1: *minimized* for review

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.)
Comment 34 Daniel Holbert [:dholbert] 2014-11-06 15:31:13 PST
Created attachment 8518538 [details] [diff] [review]
(wrong patch)

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.
Comment 35 Daniel Holbert [:dholbert] 2014-11-06 15:32:56 PST
Created attachment 8518539 [details] [diff] [review]
fix, part 1: Add ComputeObjectRenderRect helper-function

(Sorry, that was the wrong patch. Correct patch attached now.)
Comment 36 Daniel Holbert [:dholbert] 2014-11-06 15:43:55 PST
Created attachment 8518550 [details] [diff] [review]
fix, part 2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame.

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.
Comment 37 Daniel Holbert [:dholbert] 2014-11-06 16:02:33 PST
Created attachment 8518560 [details] [diff] [review]
fix, part 3: Temporary hack: if about:config pref is disabled, force nsVideoFrame to render with "object-fit: contain" behavior

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.
Comment 38 Daniel Holbert [:dholbert] 2014-11-07 00:04:11 PST
[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 39 Daniel Holbert [:dholbert] 2014-11-07 11:14:21 PST
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 40 Chris Pearce (:cpearce) 2014-11-09 13:21:01 PST
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 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2014-11-09 14:20:31 PST
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.
Comment 42 Daniel Holbert [:dholbert] 2014-11-10 13:48:49 PST
(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.
Comment 43 Daniel Holbert [:dholbert] 2014-11-11 14:18:36 PST
Created attachment 8520950 [details] [diff] [review]
fix, part 0: Don't pass ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT if overflow might happen

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.
Comment 44 Daniel Holbert [:dholbert] 2014-11-11 15:19:36 PST
Created attachment 8520994 [details] [diff] [review]
fix, part 2 v2: Honor "object-fit" & "object-position" in nsImageFrame, nsSubDocumentFrame, & nsVideoFrame

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.)
Comment 45 Daniel Holbert [:dholbert] 2014-11-11 15:34:37 PST
Created attachment 8521010 [details] [diff] [review]
fix, part 3 v2: hack to handle about:config pref being off at startup

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 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2014-11-11 15:35:21 PST
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 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2014-11-11 15:38:20 PST
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.
Comment 48 Daniel Holbert [:dholbert] 2014-11-11 15:46:01 PST
(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.)
Comment 49 Daniel Holbert [:dholbert] 2014-11-11 16:13:16 PST
(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 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2014-11-11 16:17:56 PST
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 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2014-11-11 16:19:10 PST
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
Comment 52 Daniel Holbert [:dholbert] 2014-11-11 16:21:22 PST
Thanks!
Comment 53 Daniel Holbert [:dholbert] 2014-11-11 21:16:24 PST
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!)
Comment 54 Daniel Holbert [:dholbert] 2014-11-11 22:47:14 PST
(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.)
Comment 55 Daniel Holbert [:dholbert] 2014-11-13 09:00:59 PST
(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 56 Daniel Holbert [:dholbert] 2014-11-13 09:03:01 PST
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 57 Seth Fowler [:seth] [:s2h] 2014-11-13 19:41:54 PST
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.
Comment 58 Daniel Holbert [:dholbert] 2014-11-14 11:29:33 PST
(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".
Comment 59 Daniel Holbert [:dholbert] 2014-11-14 11:58:53 PST
(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.)
Comment 60 Daniel Holbert [:dholbert] 2014-11-14 14:56:42 PST
(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
Comment 61 Daniel Holbert [:dholbert] 2014-11-14 16:49:14 PST
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.)
Comment 63 Jean-Yves Perrier [:teoli] 2014-11-18 03:43:16 PST
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
Comment 64 info 2014-11-18 06:20:39 PST
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
Comment 65 Daniel Holbert [:dholbert] 2014-11-25 15:51:43 PST
(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
Comment 66 Ryan VanderMeulen [:RyanVM] 2014-11-26 17:54:10 PST
https://hg.mozilla.org/mozilla-central/rev/4d18de1eb6b3
Comment 67 Sebastian Zartner [:sebo] 2014-11-28 14:13:55 PST
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
Comment 68 Jean-Yves Perrier [:teoli] 2014-11-29 10:24:39 PST
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.
Comment 69 Daniel Holbert [:dholbert] 2014-11-30 14:21:34 PST
Thank you, Sebastian!
Comment 70 Sebastian Zartner [:sebo] 2014-11-30 15:26:18 PST
You're welcome!

Sebastian
Comment 71 Jeremy Morton 2015-01-24 16:07:47 PST
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.
Comment 72 Jeremy Morton 2015-01-24 16:11:18 PST
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.
Comment 73 Jean-Yves Perrier [:teoli] 2015-01-24 16:13:26 PST
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.
Comment 74 David Baron :dbaron: ⌚️UTC-8 2015-01-24 16:14:30 PST
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.
Comment 75 Jeremy Morton 2015-01-24 16:24:54 PST
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.
Comment 76 David Baron :dbaron: ⌚️UTC-8 2015-01-24 16:26:43 PST
If you want to discuss it further, file a separate bug.

Note You need to log in before you can comment on or make changes to this bug.