| Summary: | Implement object-fit CSS property | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | 50167214, abarth, adele, alancutter, alexa.gerancho, allan.jensen, ap, buildbot, commit-queue, dglazkov, dino, dminor, ed, eflews.bot, eoconnor, ericbidelman, eric.carlson, eric, esprehn+autocc, feature-media-reviews, fsamuel, glenn, gyuyoung.kim, hyatt, japhet, jer.noble, joepeck, koivisto, kondapallykalyan, lstorset, macpherson, mathias, menard, mitz, mstensho, ojan.autocc, pdr, peter, rniwa, robink, sam, scherkus, simon.fraser, syoichi, webkit-bug-importer, webkit.bugzilla, webkit-ews, webkit.review.bot, webmaster, zcorpan | ||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||||||||||
| URL: | http://dev.opera.com/articles/view/css3-object-fit-object-position/ | ||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 46590 | ||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Simon Fraser (smfr)
2011-01-06 18:09:04 PST
Created attachment 78304 [details]
Patch
Note that the incorrect image result for the video test is covered by bug 52103. I'll fix that in a separate patch. 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.
@Eric: I think the style-queue error messages are WAY too long ^^^^^^^ (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. Filed bug 52161 about the style-queue message length. Comment on attachment 78304 [details]
Patch
r- discussed in IRC.
Note to self: need to deal with auto height/width if max-width or height are set. Hint should be a layout hint. 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.
None of the new layout tests seem to have come through. Lets try this again. 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.
Created attachment 89112 [details]
Patch
Attachment 89108 [details] did not build on qt: Build output: http://queues.webkit.org/results/8390087 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 on attachment 89112 [details]
Patch
Does this patch implement the max-width/max-height stuff that hyatt said we had to do?
(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. Comment on attachment 89112 [details]
Patch
Right, but the spec refers to the 'used size', which takes max-width/height into account.
(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? (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(). Evidently, it needs layout tests! Created attachment 192214 [details]
Patch
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. 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.
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 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 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 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? (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? (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'. Created attachment 192314 [details]
video element: Test switching object-fit between 'contain' and 'cover'
Created attachment 192580 [details]
Patch
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. 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 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 (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 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 Created attachment 192742 [details]
Patch
Change since previous patch: Use CANVAS in the test expectation too. Some platforms apparently won't do it pixel-by-pixel identically otherwise. 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 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 (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. (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/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!) Created attachment 196119 [details]
Patch
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 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 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
Created attachment 196241 [details]
Patch
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. 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>. 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? Created attachment 205374 [details]
Patch
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 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 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
(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 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... :) Created attachment 205680 [details]
Patch
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 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
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 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
Created attachment 205765 [details]
Patch
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.
Would it be possible for someone to review this patch soon? Simon? (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. :) (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. Created attachment 206664 [details]
Patch
Comment on attachment 206664 [details] Patch Attachment 206664 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1062937 Comment on attachment 206664 [details] Patch Attachment 206664 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1065881 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 Created attachment 206665 [details]
Patch
Grr.. this libpeerconnection.log file snuck in, causing all bots to fail. Created attachment 206666 [details]
Patch
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 Created attachment 209684 [details]
Patch
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 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 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
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 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
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 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? 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! 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. https://trac.webkit.org/r154858 Followups for video will happen via bug 52103. *** 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. (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" |