Bug 52040 - Implement object-fit CSS property
: Implement object-fit CSS property
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://dev.opera.com/articles/view/cs...
: InRadar, WebExposed
:
: 46590
  Show dependency treegraph
 
Reported: 2011-01-06 18:09 PST by
Modified: 2014-02-02 17:10 PST (History)


Attachments
Patch (472.83 KB, patch)
2011-01-07 22:05 PST, Simon Fraser (smfr)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.66 KB, patch)
2011-04-11 15:53 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (475.44 KB, patch)
2011-04-11 16:02 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (84.22 KB, patch)
2013-03-08 06:39 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
video element: Test switching object-fit between 'contain' and 'cover' (673 bytes, text/html)
2013-03-08 17:12 PST, Morten Stenshorne
no flags Details
Patch (90.43 KB, patch)
2013-03-11 15:35 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.24 KB, patch)
2013-03-12 08:34 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (103.89 KB, patch)
2013-04-02 04:08 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (855.06 KB, application/zip)
2013-04-02 13:59 PST, Build Bot
no flags Details
Patch (104.76 KB, patch)
2013-04-02 15:35 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (106.31 KB, patch)
2013-06-25 02:48 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-1 for win-future (797.11 KB, application/zip)
2013-06-25 06:15 PST, Build Bot
no flags Details
Patch (120.26 KB, patch)
2013-06-28 02:03 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (645.01 KB, application/zip)
2013-06-28 04:59 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (656.29 KB, application/zip)
2013-06-28 07:15 PST, Build Bot
no flags Details
Patch (120.91 KB, patch)
2013-06-29 04:54 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (120.16 KB, patch)
2013-07-15 08:03 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (120.33 KB, patch)
2013-07-15 08:17 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (120.18 KB, patch)
2013-07-15 08:31 PST, Morten Stenshorne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (125.36 KB, patch)
2013-08-26 15:39 PST, Simon Fraser (smfr)
sam: review+
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (616.62 KB, application/zip)
2013-08-26 17:27 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (525.20 KB, application/zip)
2013-08-26 17:47 PST, Build Bot
no flags Details


Note

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


Description From 2011-01-06 18:09:04 PST
There's a proposed CSS property called object-fit that allows you to maintain aspect ratio when rendering replaced elements.
------- Comment #1 From 2011-01-07 22:05:49 PST -------
Created an attachment (id=78304) [details]
Patch
------- Comment #2 From 2011-01-07 22:07:45 PST -------
Note that the incorrect image result for the video test is covered by bug 52103. I'll fix that in a separate patch.
------- Comment #3 From 2011-01-07 22:09:41 PST -------
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.
------- Comment #4 From 2011-01-07 22:13:06 PST -------
@Eric: I think the style-queue error messages are WAY too long ^^^^^^^
------- Comment #5 From 2011-01-07 22:40:53 PST -------
(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.
------- Comment #6 From 2011-01-10 11:35:17 PST -------
Filed bug 52161 about the style-queue message length.
------- Comment #7 From 2011-01-10 14:19:49 PST -------
(From update of attachment 78304 [details])
r- discussed in IRC.
------- Comment #8 From 2011-01-10 14:31:45 PST -------
Note to self: need to deal with auto height/width if max-width or height are set. Hint should be a layout hint.
------- Comment #9 From 2011-04-11 15:53:02 PST -------
Created an attachment (id=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.
------- Comment #10 From 2011-04-11 15:54:04 PST -------
None of the new layout tests seem to have come through.  Lets try this again.
------- Comment #11 From 2011-04-11 15:55:19 PST -------
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.
------- Comment #12 From 2011-04-11 16:02:48 PST -------
Created an attachment (id=89112) [details]
Patch
------- Comment #13 From 2011-04-11 16:09:13 PST -------
Attachment 89108 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8390087
------- Comment #14 From 2011-04-11 16:12:27 PST -------
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()).
------- Comment #15 From 2011-04-26 17:02:23 PST -------
(From update of attachment 89112 [details])
Does this patch implement the max-width/max-height stuff that hyatt said we had to do?
------- Comment #16 From 2011-04-26 17:27:38 PST -------
(In reply to comment #15)
> (From update of attachment 89112 [details] [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.
------- Comment #17 From 2011-04-26 17:28:49 PST -------
(From update of attachment 89112 [details])
Right, but the spec refers to the 'used size', which takes max-width/height into account.
------- Comment #18 From 2011-04-26 17:50:56 PST -------
(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?
------- Comment #19 From 2011-04-26 18:18:01 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 89112 [details] [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().
------- Comment #20 From 2011-04-26 19:36:59 PST -------
Evidently, it needs layout tests!
------- Comment #21 From 2013-03-08 06:39:05 PST -------
Created an attachment (id=192214) [details]
Patch
------- Comment #22 From 2013-03-08 06:39:58 PST -------
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.
------- Comment #23 From 2013-03-08 06:50:49 PST -------
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.
------- Comment #24 From 2013-03-08 07:10:39 PST -------
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?
------- Comment #25 From 2013-03-08 08:05:39 PST -------
(From update of attachment 192214 [details])
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
------- Comment #26 From 2013-03-08 08:55:46 PST -------
(From update of attachment 192214 [details])
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
------- Comment #27 From 2013-03-08 10:59:14 PST -------
(From update of attachment 192214 [details])
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?
------- Comment #28 From 2013-03-08 14:30:00 PST -------
(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?
------- Comment #29 From 2013-03-08 17:10:58 PST -------
(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'.
------- Comment #30 From 2013-03-08 17:12:37 PST -------
Created an attachment (id=192314) [details]
video element: Test switching object-fit between 'contain' and 'cover'
------- Comment #31 From 2013-03-11 15:35:56 PST -------
Created an attachment (id=192580) [details]
Patch
------- Comment #32 From 2013-03-11 15:38:36 PST -------
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.
------- Comment #33 From 2013-03-11 15:41:22 PST -------
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.
------- Comment #34 From 2013-03-11 16:56:07 PST -------
(From update of attachment 192580 [details])
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
------- Comment #35 From 2013-03-11 23:45:11 PST -------
(In reply to comment #33)
> Attachment 192580 [details] [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
------- Comment #36 From 2013-03-12 05:08:52 PST -------
(From update of attachment 192580 [details])
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
------- Comment #37 From 2013-03-12 08:34:30 PST -------
Created an attachment (id=192742) [details]
Patch
------- Comment #38 From 2013-03-12 08:37:09 PST -------
Change since previous patch:

Use CANVAS in the test expectation too.
Some platforms apparently won't do it pixel-by-pixel identically otherwise.
------- Comment #39 From 2013-03-12 18:13:49 PST -------
(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
------- Comment #40 From 2013-03-12 18:34:12 PST -------
(From update of attachment 192742 [details])
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
------- Comment #41 From 2013-03-14 01:47:06 PST -------
(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.
------- Comment #42 From 2013-03-21 14:45:17 PST -------
(In reply to comment #41)
> (In reply to comment #39)
> > (From update of attachment 192742 [details] [details] [details])
> > Attachment 192742 [details] [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/webkit-dev@lists.webkit.org/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!)
------- Comment #43 From 2013-04-02 04:08:24 PST -------
Created an attachment (id=196119) [details]
Patch
------- Comment #44 From 2013-04-02 04:10:05 PST -------
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.
------- Comment #45 From 2013-04-02 13:59:14 PST -------
(From update of attachment 196119 [details])
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
------- Comment #46 From 2013-04-02 13:59:19 PST -------
Created an attachment (id=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
------- Comment #47 From 2013-04-02 15:35:48 PST -------
Created an attachment (id=196241) [details]
Patch
------- Comment #48 From 2013-04-02 15:41:06 PST -------
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.
------- Comment #49 From 2013-04-17 15:07:00 PST -------
<rdar://problem/13678192>
------- Comment #50 From 2013-04-18 08:23:52 PST -------
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>.
------- Comment #51 From 2013-06-24 00:05:24 PST -------
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?
------- Comment #52 From 2013-06-25 02:48:18 PST -------
Created an attachment (id=205374) [details]
Patch
------- Comment #53 From 2013-06-25 02:52:39 PST -------
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.
------- Comment #54 From 2013-06-25 06:15:20 PST -------
(From update of attachment 205374 [details])
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
------- Comment #55 From 2013-06-25 06:15:27 PST -------
Created an attachment (id=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
------- Comment #56 From 2013-06-25 07:56:59 PST -------
(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
------- Comment #57 From 2013-06-25 08:26:47 PST -------
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... :)
------- Comment #58 From 2013-06-28 02:03:27 PST -------
Created an attachment (id=205680) [details]
Patch
------- Comment #59 From 2013-06-28 04:59:38 PST -------
(From update of attachment 205680 [details])
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
------- Comment #60 From 2013-06-28 04:59:44 PST -------
Created an attachment (id=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
------- Comment #61 From 2013-06-28 07:15:24 PST -------
(From update of attachment 205680 [details])
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
------- Comment #62 From 2013-06-28 07:15:31 PST -------
Created an attachment (id=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
------- Comment #63 From 2013-06-29 04:54:43 PST -------
Created an attachment (id=205765) [details]
Patch
------- Comment #64 From 2013-06-29 04:55:22 PST -------
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.
------- Comment #65 From 2013-07-10 01:38:41 PST -------
Would it be possible for someone to review this patch soon? Simon?
------- Comment #66 From 2013-07-15 04:42:40 PST -------
(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. :)
------- Comment #67 From 2013-07-15 08:02:02 PST -------
(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.
------- Comment #68 From 2013-07-15 08:03:05 PST -------
Created an attachment (id=206664) [details]
Patch
------- Comment #69 From 2013-07-15 08:11:04 PST -------
(From update of attachment 206664 [details])
Attachment 206664 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1062937
------- Comment #70 From 2013-07-15 08:11:25 PST -------
(From update of attachment 206664 [details])
Attachment 206664 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1065881
------- Comment #71 From 2013-07-15 08:11:56 PST -------
(From update of attachment 206664 [details])
Attachment 206664 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1074130
------- Comment #72 From 2013-07-15 08:17:18 PST -------
Created an attachment (id=206665) [details]
Patch
------- Comment #73 From 2013-07-15 08:29:25 PST -------
Grr.. this libpeerconnection.log file snuck in, causing all bots to fail.
------- Comment #74 From 2013-07-15 08:31:20 PST -------
Created an attachment (id=206666) [details]
Patch
------- Comment #75 From 2013-08-22 00:00:01 PST -------
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
------- Comment #76 From 2013-08-26 15:39:26 PST -------
Created an attachment (id=209684) [details]
Patch
------- Comment #77 From 2013-08-26 16:27:43 PST -------
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()
------- Comment #78 From 2013-08-26 17:27:06 PST -------
(From update of attachment 209684 [details])
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
------- Comment #79 From 2013-08-26 17:27:13 PST -------
Created an attachment (id=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
------- Comment #80 From 2013-08-26 17:47:41 PST -------
(From update of attachment 209684 [details])
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
------- Comment #81 From 2013-08-26 17:47:50 PST -------
Created an attachment (id=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
------- Comment #82 From 2013-08-28 16:53:35 PST -------
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"]
------- Comment #83 From 2013-08-29 15:11:50 PST -------
(From update of attachment 209684 [details])
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?
------- Comment #84 From 2013-08-29 15:16:29 PST -------
(From update of attachment 209684 [details])
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!
------- Comment #85 From 2013-08-29 15:21:13 PST -------
(From update of attachment 209684 [details])
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.
------- Comment #86 From 2013-08-29 17:23:15 PST -------
https://trac.webkit.org/r154858

Followups for video will happen via bug 52103.
------- Comment #87 From 2014-02-02 17:10:35 PST -------
*** 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.