Bug 52040 – Implement object-fit CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

There's a proposed CSS property called object-fit that allows you to maintain aspect ratio when rendering replaced elements.

[Simon Fraser (smfr)]

Created attachment 78304 [details]
Patch

[Simon Fraser (smfr)]

Note that the incorrect image result for the video test is covered by bug 52103. I'll fix that in a separate patch.

[WebKit Review Bot]

Attachment 78304 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/object-fit-elements.html', u'LayoutTests/fast/css/object-fit-elements2.html', u'LayoutTests/fast/css/object-fit-grow.html', u'LayoutTests/fast/css/object-fit-shrink.html', u'LayoutTests/fast/css/parsing-object-fit-expected.txt', u'LayoutTests/fast/css/parsing-object-fit.html', u'LayoutTests/fast/css/resources/circles-landscape-small.png', u'LayoutTests/fast/css/resources/circles-landscape.png', u'LayoutTests/fast/css/resources/circles-portrait-small.png', u'LayoutTests/fast/css/resources/circles-portrait.png', u'LayoutTests/fast/css/script-tests/parsing-object-fit.js', u'LayoutTests/media/video-object-fit.html', u'LayoutTests/platform/mac/fast/css/object-fit-elements-expected.checksum', u'LayoutTests/platform/mac/fast/css/object-fit-elements-expected.png', u'LayoutTests/platform/mac/fast/css/object-fit-elements-expected.txt', u'LayoutTests/platform/mac/fast/css/object-fit-elements2-expected.checksum', u'LayoutTests/platform/mac/fast/css/object-fit-elements2-expected.png', u'LayoutTests/platform/mac/fast/css/object-fit-elements2-expected.txt', u'LayoutTests/platform/mac/fast/css/object-fit-grow-expected.checksum', u'LayoutTests/platform/mac/fast/css/object-fit-grow-expected.png', u'LayoutTests/platform/mac/fast/css/object-fit-grow-expected.txt', u'LayoutTests/platform/mac/fast/css/object-fit-shrink-expected.checksum', u'LayoutTests/platform/mac/fast/css/object-fit-shrink-expected.png', u'LayoutTests/platform/mac/fast/css/object-fit-shrink-expected.txt', u'LayoutTests/platform/mac/media/video-object-fit-expected.checksum', u'LayoutTests/platform/mac/media/video-object-fit-expected.png', u'LayoutTests/platform/mac/media/video-object-fit-expected.txt', u'WebCore/ChangeLog', u'WebCore/css/CSSComputedStyleDeclaration.cpp', u'WebCore/css/CSSParser.cpp', u'WebCore/css/CSSPrimitiveValueMappings.h', u'WebCore/css/CSSPropertyNames.in', u'WebCore/css/CSSStyleSelector.cpp', u'WebCore/css/CSSValueKeywords.in', u'WebCore/platform/graphics/FloatRect.cpp', u'WebCore/platform/graphics/FloatRect.h', u'WebCore/rendering/RenderHTMLCanvas.cpp', u'WebCore/rendering/RenderImage.cpp', u'WebCore/rendering/RenderImage.h', u'WebCore/rendering/RenderReplaced.cpp', u'WebCore/rendering/RenderReplaced.h', u'WebCore/rendering/RenderVideo.cpp', u'WebCore/rendering/style/RenderStyle.cpp', u'WebCore/rendering/style/RenderStyle.h', u'WebCore/rendering/style/RenderStyleConstants.h', u'WebCore/rendering/style/StyleRareInheritedData.cpp', u'WebCore/rendering/style/StyleRareInheritedData.h']" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:561:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2252:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2270:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 3 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

@Eric: I think the style-queue error messages are WAY too long ^^^^^^^

[Eric Seidel (no email)]

(In reply to comment #4)
> @Eric: I think the style-queue error messages are WAY too long ^^^^^^^

And whose fault is that? ;p

I agree. Will look at fixing on Monday.

[Eric Seidel (no email)]

Filed bug 52161 about the style-queue message length.

[Dave Hyatt]

Comment on attachment 78304 [details]
Patch

r- discussed in IRC.

[Simon Fraser (smfr)]

Note to self: need to deal with auto height/width if max-width or height are set. Hint should be a layout hint.

[Jer Noble]

Created attachment 89108 [details]
Patch

Rebased Simon's patch against ToT, fixed up the style errors, and made a couple of build fixes caused by intervening changes in the tree.

[Jer Noble]

None of the new layout tests seem to have come through.  Lets try this again.

[WebKit Review Bot]

Attachment 89108 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'Source/WebCore/C..." exit_code: 1

Source/WebCore/css/CSSPrimitiveValueMappings.h:2291:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/css/CSSPrimitiveValueMappings.h:2309:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/rendering/style/RenderStyle.cpp:563:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 89112 [details]
Patch

[Early Warning System Bot]

Attachment 89108 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8390087

[Jer Noble]

Note this last patch does not have any logical changes aside from those necessary to fix the build (e.g., FloatRect::right() has been renamed FloatRect::maxX()).

[Simon Fraser (smfr)]

Comment on attachment 89112 [details]
Patch

Does this patch implement the max-width/max-height stuff that hyatt said we had to do?

[Jer Noble]

(In reply to comment #15)
> (From update of attachment 89112 [details])
> Does this patch implement the max-width/max-height stuff that hyatt said we had to do?

This patch is no different than your first patch, except that it's been rebased against a newer version of ToT.  The original was from before the /WebCore -> /Source/WebCore transition.

That said, the max-width/max-height stuff is no longer in the dev version of the HTML5 spec.  Tab Atkins may be able to shed light on what that means for implementors.

[Simon Fraser (smfr)]

Comment on attachment 89112 [details]
Patch

Right, but the spec refers to the 'used size', which takes max-width/height into account.

[Jer Noble]

(In reply to comment #17)
> (From update of attachment 89112 [details])
> Right, but the spec refers to the 'used size', which takes max-width/height into account.

How is the "used size" different than the contentBoxRect() size?

[Jer Noble]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 89112 [details] [details])
> > Right, but the spec refers to the 'used size', which takes max-width/height into account.
> 
> How is the "used size" different than the contentBoxRect() size?

From a cursory inspection, all the max-width and max-height calculations are done in RenderBox::computeLogicalWidth() and are therefore already present in contentBoxRect().

[Simon Fraser (smfr)]

Evidently, it needs layout tests!

[Morten Stenshorne]

Created attachment 192214 [details]
Patch

[Morten Stenshorne]

As discussed with Simon via e-mail, I'm taking over this bug. Based on his original patch, my patch is an implementation of object-fit as defined in http://www.w3.org/TR/2012/CR-css3-images-20120417/#object-fit

Compared to his patch, here are the most important changes:

Remove -webkit- prefix, since the spec is in CR now.

Chop off even more code from RenderVideo::videoBox(), and make sure that the poster's size is used (when it should be used).

All types of VIDEO elements need to be clipped, not only posters.

Add video { object-fit: contain; } to match the old behavior of VIDEO elements.

All tests that were previously DumpRenderTree tests are now reftests. There are no longer any platform specific expectations. Also test the new object fit values ('auto' is gone, 'none' and 'scale-down' have been added).

RenderImage::backgroundIsObscured() may have to return false because of object-fit.

[WebKit Review Bot]

Attachment 192214 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/object-fit-elements-expected.html', u'LayoutTests/fast/css/object-fit-elements.html', u'LayoutTests/fast/css/object-fit-grow-landscape-expected.html', u'LayoutTests/fast/css/object-fit-grow-landscape.html', u'LayoutTests/fast/css/object-fit-grow-portrait-expected.html', u'LayoutTests/fast/css/object-fit-grow-portrait.html', u'LayoutTests/fast/css/object-fit-shrink-expected.html', u'LayoutTests/fast/css/object-fit-shrink.html', u'LayoutTests/fast/css/parsing-object-fit-expected.txt', u'LayoutTests/fast/css/parsing-object-fit.html', u'LayoutTests/fast/css/resources/circles-landscape-small.png', u'LayoutTests/fast/css/resources/circles-landscape.png', u'LayoutTests/fast/css/resources/circles-portrait-small.png', u'LayoutTests/fast/css/resources/circles-portrait.png', u'LayoutTests/fast/css/script-tests/parsing-object-fit.js', u'LayoutTests/media/video-object-fit-expected.html', u'LayoutTests/media/video-object-fit.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/html.css', u'Source/WebCore/rendering/RenderHTMLCanvas.cpp', u'Source/WebCore/rendering/RenderImage.cpp', u'Source/WebCore/rendering/RenderReplaced.cpp', u'Source/WebCore/rendering/RenderReplaced.h', u'Source/WebCore/rendering/RenderVideo.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
LayoutTests/fast/css/resources/circles-portrait.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Morten Stenshorne]

I'm using git locally, but nevertheless I did what check-webkit-style told me to do to my svn configuration file, and the warnings went away on my machine. Do the review bot machines need to get their svn settings changed as well, or have I actually done something wrong here that needs to be fixed?

[WebKit Review Bot]

Comment on attachment 192214 [details]
Patch

Attachment 192214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17027244

New failing tests:
media/video-object-fit.html
fast/css/object-fit-elements.html

[WebKit Review Bot]

Comment on attachment 192214 [details]
Patch

Attachment 192214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17014195

New failing tests:
media/video-object-fit.html
fast/css/object-fit-elements.html

[Simon Fraser (smfr)]

Comment on attachment 192214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192214&action=review

I think this is generally good, but please address the comments.

> Source/WebCore/rendering/RenderReplaced.cpp:331
> +        bool widthStretchedMore = contentRect.height().toFloat() / intrinsicSize.height().toFloat() < contentRect.width().toFloat() / intrinsicSize.width().toFloat();
> +        if (widthStretchedMore != (objectFit == ObjectFitCover))
> +            finalRect.setWidth(contentRect.height() * intrinsicSize.width() / intrinsicSize.height());
> +        else
> +            finalRect.setHeight(contentRect.width() * intrinsicSize.height() / intrinsicSize.width());

I suspect we have logic similar to this in several places. Perhaps we can move it into a function in FloatRect.cpp?

> Source/WebCore/rendering/style/RenderStyle.cpp:687
> +        || rareNonInheritedData->m_objectFit != other->rareNonInheritedData->m_objectFit

Since videos are rendered via compositing, I wonder if issuing a repaint when object-fit changes is enough to get videos to resize when just object-fit changes. Can you make a testcase where you change object-fit on video on hover, and see if it works?

[Morten Stenshorne]

(In reply to comment #27)

> I suspect we have logic similar to this in several places. Perhaps we can move it into a function in FloatRect.cpp?

Sure, but FloatRect? Although the current solution converts some of the values to floats to reliably figure out which dimension to change, wouldn't it be better and cheaper to work on LayoutRect and LayoutSize, so we won't have to convert back and forth?

[Morten Stenshorne]

(In reply to comment #27)

> Since videos are rendered via compositing, I wonder if issuing a repaint when object-fit changes is enough to get videos to resize when just object-fit changes. Can you make a testcase where you change object-fit on video on hover, and see if it works?

Yes, that actually works fine, but it could be a platform dependent thing, I suppose? I use the Gtk port. Even specified --enable-accelerated-compositing=true on the command line. Still no problem. I tested switching between 'cover', 'contain' and 'fill'.

[Morten Stenshorne]

Created attachment 192314 [details]
video element: Test switching object-fit between 'contain' and 'cover'

[Morten Stenshorne]

Created attachment 192580 [details]
Patch

[Morten Stenshorne]

Changes since the previous patch:

Add tests for changing object-fit values on videos after initially rendering them.

Fix test for object-fit:fill in the video test. It failed.

add fitRectToAspectRatio() utility function, using LayoutRect and LayoutSize.

[WebKit Review Bot]

Attachment 192580 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/object-fit-elements-expected.html', u'LayoutTests/fast/css/object-fit-elements.html', u'LayoutTests/fast/css/object-fit-grow-landscape-expected.html', u'LayoutTests/fast/css/object-fit-grow-landscape.html', u'LayoutTests/fast/css/object-fit-grow-portrait-expected.html', u'LayoutTests/fast/css/object-fit-grow-portrait.html', u'LayoutTests/fast/css/object-fit-shrink-expected.html', u'LayoutTests/fast/css/object-fit-shrink.html', u'LayoutTests/fast/css/parsing-object-fit-expected.txt', u'LayoutTests/fast/css/parsing-object-fit.html', u'LayoutTests/fast/css/resources/circles-landscape-small.png', u'LayoutTests/fast/css/resources/circles-landscape.png', u'LayoutTests/fast/css/resources/circles-portrait-small.png', u'LayoutTests/fast/css/resources/circles-portrait.png', u'LayoutTests/fast/css/script-tests/parsing-object-fit.js', u'LayoutTests/media/video-object-fit-change-expected.html', u'LayoutTests/media/video-object-fit-change.html', u'LayoutTests/media/video-object-fit-expected.html', u'LayoutTests/media/video-object-fit.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/html.css', u'Source/WebCore/platform/graphics/LayoutRect.cpp', u'Source/WebCore/platform/graphics/LayoutRect.h', u'Source/WebCore/rendering/RenderHTMLCanvas.cpp', u'Source/WebCore/rendering/RenderImage.cpp', u'Source/WebCore/rendering/RenderReplaced.cpp', u'Source/WebCore/rendering/RenderReplaced.h', u'Source/WebCore/rendering/RenderVideo.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
LayoutTests/fast/css/resources/circles-portrait.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 192580 [details]
Patch

Attachment 192580 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17170357

New failing tests:
fast/css/object-fit-elements.html

[Alan Cutter]

(In reply to comment #33)
> Attachment 192580 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/object-fit-elements-expected.html', u'LayoutTests/fast/css/object-fit-elements.html', u'LayoutTests/fast/css/object-fit-grow-landscape-expected.html', u'LayoutTests/fast/css/object-fit-grow-landscape.html', u'LayoutTests/fast/css/object-fit-grow-portrait-expected.html', u'LayoutTests/fast/css/object-fit-grow-portrait.html', u'LayoutTests/fast/css/object-fit-shrink-expected.html', u'LayoutTests/fast/css/object-fit-shrink.html', u'LayoutTests/fast/css/parsing-object-fit-expected.txt', u'LayoutTests/fast/css/parsing-object-fit.html', u'LayoutTests/fast/css/resources/circles-landscape-small.png', u'LayoutTests/fast/css/resources/circles-landscape.png', u'LayoutTests/fast/css/resources/circles-portrait-small.png', u'LayoutTests/fast/css/resources/circles-portrait.png', u'LayoutTests/fast/css/script-tests/parsing-object-fit.js', u'LayoutTests/media/video-object-fit-change-expected.html', u'LayoutTests/media/video-object-fit-change.html', u'LayoutTests/media/video-object-fit-expected.html', u'LayoutTests/media/video-object-fit.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/html.css', u'Source/WebCore/platform/graphics/LayoutRect.cpp', u'Source/WebCore/platform/graphics/LayoutRect.h', u'Source/WebCore/rendering/RenderHTMLCanvas.cpp', u'Source/WebCore/rendering/RenderImage.cpp', u'Source/WebCore/rendering/RenderReplaced.cpp', u'Source/WebCore/rendering/RenderReplaced.h', u'Source/WebCore/rendering/RenderVideo.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
> LayoutTests/fast/css/resources/circles-portrait.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
> Total errors found: 1 in 38 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This style bot problem should now be resolved: https://bugs.webkit.org/show_bug.cgi?id=107724

[Build Bot]

Comment on attachment 192580 [details]
Patch

Attachment 192580 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17008706

New failing tests:
media/video-object-fit.html
fast/css/object-fit-elements.html
media/video-object-fit-change.html

[Morten Stenshorne]

Created attachment 192742 [details]
Patch

[Morten Stenshorne]

Change since previous patch:

Use CANVAS in the test expectation too.
Some platforms apparently won't do it pixel-by-pixel identically otherwise.

[Build Bot]

Comment on attachment 192742 [details]
Patch

Attachment 192742 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17131199

New failing tests:
media/video-object-fit.html
fast/css/object-fit-elements.html
media/video-object-fit-change.html

[Build Bot]

Comment on attachment 192742 [details]
Patch

Attachment 192742 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17115462

New failing tests:
media/video-object-fit.html
media/video-object-fit-change.html

[Morten Stenshorne]

(In reply to comment #39)
> (From update of attachment 192742 [details])
> Attachment 192742 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17131199
> 
> New failing tests:
> media/video-object-fit.html
> fast/css/object-fit-elements.html
> media/video-object-fit-change.html

I think I need some help here, as I don't have access to a Mac. Is there a way I can get a bot to provide me with screenshots or something, so that I can investigate these failures?

The failing tests are all reftests.

[Andrew Scherkus]

(In reply to comment #41)
> (In reply to comment #39)
> > (From update of attachment 192742 [details] [details])
> > Attachment 192742 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17131199
> > 
> > New failing tests:
> > media/video-object-fit.html
> > fast/css/object-fit-elements.html
> > media/video-object-fit-change.html
> 
> I think I need some help here, as I don't have access to a Mac. Is there a way I can get a bot to provide me with screenshots or something, so that I can investigate these failures?
> 
> The failing tests are all reftests.

Currently there's no way for EWS to return failures.

I'm not sure if it's applicable in this case, but there's been a recent discussion on webkit-dev@ about the best way to land new tests / rebaseline tests:
http://www.mail-archive.com/[e-mail]/msg22640.html

Assuming your test failing on mac due to some subtle issues, it might be reasonable to set mac-specific expectations for them to fail, land the patch, then use garden-o-matic to inspect the mac results once they have been generated + ping the current mac sheriff:
http://trac.webkit.org/wiki/Rebaseline

(smfr: correct me if I'm in the wrong here!)

[Morten Stenshorne]

Created attachment 196119 [details]
Patch

[Morten Stenshorne]

Changes since last patch:
Split object-fit-elements test into one test per element, and use
video poster in the video poster ref, rather than regular images.

[Build Bot]

Comment on attachment 196119 [details]
Patch

Attachment 196119 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17338652

New failing tests:
media/video-object-fit.html
media/video-object-fit-change.html

[Build Bot]

Created attachment 196220 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Morten Stenshorne]

Created attachment 196241 [details]
Patch

[Morten Stenshorne]

Added expected failures for Mac. Also reported bug 113830. The screenshots from EWS were useful, but since I don't have access to a Mac, I cannot really investigate any further. The problems seem to be:

1. The Mac video player doesn't clip to the content box. Is something missing on the WebCore side, perhaps?
2. The Mac video player seems to lock the aspect ratio to the intrinsic one.

[Radar WebKit Bug Importer]

<rdar://problem/13678192>

[Erik Dahlström]

Should be fixed for svg too, https://svgwg.org/svg2-draft/struct.html#ImageElement. Presto implementation had some special handling of this for viewport-establishing elements, such as <svg>.

[Morten Stenshorne]

So for SVG it seems that we cannot handle object-fit on the HTML side (RenderImage), since the SVG attribute preserveAspectRatio may take precedence. The combination of 'object-fit:fill' (the initial value) and preserveAspectRatio=meet (default) is especially interesting, since that means retaining the aspect ratio. Am I right?

[Morten Stenshorne]

Created attachment 205374 [details]
Patch

[Morten Stenshorne]

New patch. In addition to a master rebase, make sure that SVG images always get object-fit:fill as far as RenderImage is concerned (otherwise things would look messed up). The SVG code needs to figure out on its own how to display it, and that probably belongs in a separate patch, done by someone who knows SVG.

[Build Bot]

Comment on attachment 205374 [details]
Patch

Attachment 205374 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/916499

New failing tests:
fast/media/mq-transform-03.html
fast/forms/search-event-delay.html
fast/media/mq-transform-02.html
platform/win/accessibility/multiple-select-element-role.html

[Build Bot]

Created attachment 205393 [details]
Archive of layout-test-results from APPLE-EWS-1 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-1  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Leif Arne Storset]

(In reply to comment #51)
> So for SVG it seems that we cannot handle object-fit on the HTML side (RenderImage), since the SVG attribute preserveAspectRatio may take precedence. The combination of 'object-fit:fill' (the initial value) and preserveAspectRatio=meet (default) is especially interesting, since that means retaining the aspect ratio. Am I right?

preserveAspectRatio should not "take precedence", exactly. It decides whether and how the graphics are stretched to fit within the SVG viewport, and that viewport is sized in part by 'object-fit'. Yes, 'object-fit: fill' will by defeault retain the aspect ratio of the embedded SVG graphic, but the SVG canvas will fill the content box. With 'object-fit: contain' (and an intrinsic aspect ratio or dimensions), the SVG graphic will be the same size, but SVG only controls the rectangle immediately surrounding it.

Not knowing WebKit (I'm just the implementer of 'object-*' for Presto) I don't know whether what I wrote above makes any difference or contradicts what you wrote, but it sounds like it does. If there is any meaningful "HTML side" that can handle 'object-fit', it should.

I suppose you've already read the note in the spec (beginning "Per the CSS⇋Object Negotiation algorithm…"). More discussion in the short thread starting at http://lists.w3.org/Archives/Public/www-style/2011Mar/0141.html

[Morten Stenshorne]

This does indeed contradict what I was saying, and what you write makes a lot of sense. I suppose I have to write a patch that deals with object-fit on the "HTML side" also for SVG then. That requires more work than one might think, since there's something fishy going on with instrinsic size handling of SVG images that are also extrinsicly sized. So much for trying to walk around the entire problem... :)

[Morten Stenshorne]

Created attachment 205680 [details]
Patch

[Build Bot]

Comment on attachment 205680 [details]
Patch

Attachment 205680 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/997315

New failing tests:
fast/images/link-body-content-imageDimensionChanged-crash.html

[Build Bot]

Created attachment 205699 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205680 [details]
Patch

Attachment 205680 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/981714

New failing tests:
fast/images/link-body-content-imageDimensionChanged-crash.html

[Build Bot]

Created attachment 205712 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Morten Stenshorne]

Created attachment 205765 [details]
Patch

[Morten Stenshorne]

Attempt to fix crash in fast/images/link-body-content-imageDimensionChanged-crash.html
    
Make sure that we have been laid out before updating the inner content rectangle. Otherwise the containing block may be NULL. We need the containing block to resolve percentages.

[Morten Stenshorne]

Would it be possible for someone to review this patch soon? Simon?

[Leif Arne Storset]

(I don't have the EditBugs permission, so commenting here instead of the review.)

View in context: https://bugs.webkit.org/attachment.cgi?id=205680&action=review

> Source/WebCore/platform/graphics/LayoutRect.cpp:134
> +void fitRectToAspectRatio(LayoutRect& rect, const LayoutSize& size, AspectRatioFit fit)
> +{
> +    // Change width or height to honor the aspect ratio. If fit is AspectRatioFitGrow,
> +    // increase in the dimension that was stretched less (proportionally to the intrinsic
> +    // length). Otherwise, decrease in the dimension that was stretched more
> +    // (proportionally to the intrinsic length).

I struggled to understand this until I read replacedContentRect. The description of this function doesn't make sense outside that context: reading "the dimension that was stretched less" you have to ask "who stretched it?".  Some things that would help would be prepending to the first sentence in the comment "After stretching an object to completely fill a rect", and renaming the arguments to stretched_rect and intrinsic_size.

I wonder if it's worth having an independent function for this, since it's only used once. But I've already commented too much on a codebase I barely know. :)

[Morten Stenshorne]

(In reply to comment #66)
> (I don't have the EditBugs permission, so commenting here instead of the review.)
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=205680&action=review
> 
> > Source/WebCore/platform/graphics/LayoutRect.cpp:134
> > +void fitRectToAspectRatio(LayoutRect& rect, const LayoutSize& size, AspectRatioFit fit)
> > +{
> > +    // Change width or height to honor the aspect ratio. If fit is AspectRatioFitGrow,
> > +    // increase in the dimension that was stretched less (proportionally to the intrinsic
> > +    // length). Otherwise, decrease in the dimension that was stretched more
> > +    // (proportionally to the intrinsic length).
> 
> I struggled to understand this until I read replacedContentRect. The description of this function doesn't make sense outside that context: reading "the dimension that was stretched less" you have to ask "who stretched it?".  Some things that would help would be prepending to the first sentence in the comment "After stretching an object to completely fill a rect", and renaming the arguments to stretched_rect and intrinsic_size.

Yeah, that wasn't very clear without the secret context. I think I've come up with something better now. I also came to the realization that using a rectangle here isn't very useful, since we're only interested in the size. So I've moved the code to LayoutSize and tried to document it better.

> I wonder if it's worth having an independent function for this, since it's only used once. But I've already commented too much on a codebase I barely know. :)

I'm not sure. Yes, so far, it's only used once, but I was asked to add it in comment 27, so I'm keeping it for now.

I will upload a new patch now.

[Morten Stenshorne]

Created attachment 206664 [details]
Patch

[Early Warning System Bot]

Comment on attachment 206664 [details]
Patch

Attachment 206664 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1062937

[EFL EWS Bot]

Comment on attachment 206664 [details]
Patch

Attachment 206664 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1065881

[Early Warning System Bot]

Comment on attachment 206664 [details]
Patch

Attachment 206664 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1074130

[Morten Stenshorne]

Created attachment 206665 [details]
Patch

[Morten Stenshorne]

Grr.. this libpeerconnection.log file snuck in, causing all bots to fail.

[Morten Stenshorne]

Created attachment 206666 [details]
Patch

[Morten Stenshorne]

Landed in Blink. Unassigning myself here. Backporting from Blink should be possible, but not completely trivial, since files and classes have been moved and renamed a little.

http://src.chromium.org/viewvc/blink?view=revision&revision=156535

[Simon Fraser (smfr)]

Created attachment 209684 [details]
Patch

[Simon Fraser (smfr)]

There are a couple of issues with <video> in this patch. Compositing layers need to be clipped in some cases, and dragging a <video> testcase onto a tab causes:

ASSERTION FAILED: frame().view() == this
/Volumes/SSData/Development/OSX/webkit/OpenSource/Source/WebCore/page/FrameView.cpp(2377) : void WebCore::FrameView::scheduleRelayout()
1   0x102b09ea0 WTFCrash
2   0x103f2b5b2 WebCore::FrameView::scheduleRelayout()
3   0x104c24153 WebCore::RenderObject::scheduleRelayout()
4   0x104c23cd6 WebCore::RenderObject::markContainingBlocksForLayout(bool, WebCore::RenderObject*)
5   0x1039a2099 WebCore::RenderObject::setNeedsLayout(bool, WebCore::MarkingBehavior)
6   0x104d318be WebCore::RenderVideo::updateIntrinsicSize()
7   0x104d31e69 WebCore::RenderVideo::updatePlayer()
8   0x104d31f78 WebCore::RenderVideo::updateFromElement()
9   0x1040cdcad WebCore::HTMLMediaElement::stop()
10  0x1040cdd2c non-virtual thunk to WebCore::HTMLMediaElement::stop()
11  0x104dc5946 WebCore::ScriptExecutionContext::stopActiveDOMObjects()
12  0x103c82b24 WebCore::Document::detach()
13  0x1039525cc WebCore::CachedFrame::destroy()
14  0x10395c4eb WebCore::CachedPage::destroy()
15  0x10395c42c WebCore::CachedPage::~CachedPage()
16  0x10395c3f5 WebCore::CachedPage::~CachedPage()
17  0x103f0c3c9 WTF::RefCounted<WebCore::CachedPage>::deref()
18  0x103f0c375 void WTF::derefIfNotNull<WebCore::CachedPage>(WebCore::CachedPage*)
19  0x1049bece7 WTF::RefPtr<WebCore::CachedPage>::clear()
20  0x1049bd63c WebCore::PageCache::remove(WebCore::HistoryItem*)
21  0x1049bd141 WebCore::PageCache::prune()
22  0x1049bd0a7 WebCore::PageCache::setCapacity(int)
23  0x1014249de WebKit::WebProcess::releasePageCache()

[Build Bot]

Comment on attachment 209684 [details]
Patch

Attachment 209684 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1583060

New failing tests:
media/video-object-fit.html

[Build Bot]

Created attachment 209696 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209684 [details]
Patch

Attachment 209684 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1592068

New failing tests:
media/video-object-fit.html

[Build Bot]

Created attachment 209698 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Joseph Pecoraro]

Neat! When this lands it would be awesome to add it to the Inspector's CSS Autocompletion:

Source/WebInspectorUI/UserInterface/CSSKeywordCompletions.js

Just add the "property": ["values"] to the WebInspector.CSSKeywordCompletions._propertyKeywordMap:

    "object-fit": ["fill", "contain", "cover", "none", "down", "scale-down"]

[Antti Koivisto]

Comment on attachment 209684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209684&action=review

r=me

> Source/WebCore/platform/graphics/LayoutSize.h:126
> +        if ((widthScale > heightScale) != (fit == AspectRatioFitGrow))

This is compact but the logic is hard to follow. It would be helpful to separate the AspectRatioFitShrink/AspectRatioFitGrow cases so both get explicitly named.

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:74
> +    bool clip = !contentRect.contains(paintRect);
> +    if (clip) {
> +        // Not allowed to overflow the content box.
> +        paintInfo.context->save();
> +        paintInfo.context->clip(pixelSnappedIntRect(contentRect));
> +    }

If this is a common pattern it might be good to add a RAII helper.

> Source/WebCore/rendering/RenderImage.cpp:425
> +        LayoutRect contentRect = contentBoxRect();
> +        contentRect.moveBy(paintOffset);
> +        LayoutRect paintRect = replacedContentRect();
> +        paintRect.moveBy(paintOffset);
> +        bool clip = !contentRect.contains(paintRect);
> +        if (clip) {
> +            context->save();
> +            context->clip(contentRect);
> +        }

The pattern seems to be repeating.

> Source/WebCore/rendering/style/RenderStyleConstants.h:215
> +enum EObjectFit {
> +    ObjectFitFill, ObjectFitContain, ObjectFitCover, ObjectFitNone, ObjectFitScaleDown
> +};

Can't we stop using this ancient EFoo naming convention?

[Sam Weinig]

Comment on attachment 209684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209684&action=review

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3597
> +    switch (m_value.valueID) {

Please add an ASSERT(isValueID());

> Source/WebCore/loader/cache/CachedImage.h:73
> +    enum SizeType {
> +        NormalSize, // Report the size of the image associated with a certain renderer
> +        IntrinsicSize // Report the intrinsic size, i.e. ignore whatever has been set extrinsically.
> +    };

These comments are a bit confusing. Can you clarify what these do.

> Source/WebCore/platform/graphics/LayoutRect.h:163
> +    void scale(float xAxisScale, float yAxisScale);

The parameters aren't great.  How about xScale, yScale.

> Source/WebCore/platform/graphics/LayoutSize.h:126
> +        if ((widthScale > heightScale) != (fit == AspectRatioFitGrow))

This if-statement is not easy to understand.

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:85
> +    if (clip)
> +        context->restore();

Can you use a stack based protector here?

> Source/WebCore/rendering/RenderImage.cpp:277
> +        if (everHadLayout() && !selfNeedsLayout()) {
> +            // The inner content rectangle is calculated during layout, but may need an update now
> +            // (unless the box has already been scheduled for layout). In order to calculate it, we
> +            // may need values from the containing block, though, so make sure that we're not too
> +            // early. It may be that layout hasn't even taken place once yet.
> +
> +            // FIXME: we should not have to trigger another call to setContainerSizeForRenderer()
> +            // from here, since it's already being done during layout.
> +            updateInnerContentRect();
> +        }

This looks odd.  Is this just an optimization?

> Source/WebCore/rendering/RenderImage.cpp:440
> +        if (clip)
> +            context->restore();

Can we use stack based protection.

> Source/WebCore/rendering/style/RenderStyleConstants.h:213
> +enum EObjectFit {

No E!

[Antti Koivisto]

Comment on attachment 209684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209684&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:301
> +LayoutRect RenderReplaced::replacedContentRect(const LayoutSize* overriddenIntrinsicSize) const

Not a fan of using pointer for an optional argument like this.

[Simon Fraser (smfr)]

https://trac.webkit.org/r154858

Followups for video will happen via bug 52103.

[Alexa]

*** Bug 260998 has been marked as a duplicate of this bug. ***
Seen live from the domain http://volichat.com/adult-chat-rooms
Marked for reference. Resolved as fixed @bugzilla.

[yisibl]

(In reply to comment #0)
> There's a proposed CSS property called object-fit that allows you to
> maintain aspect ratio when rendering replaced elements.

Please fix this bug: https://bugs.webkit.org/show_bug.cgi?id=122811

"support for the object-position CSS property"