Device to reproduce - iPhone - OSX Safari Responsive design -> iPhone Steps to reproduce 1. Open https://smartslider3.com/bugs/webkit/object-fit-srcset/srcset.html Expected result The background image with the cow should cover the area as it has object-fit:cover Actual result The background image rendered as if it would be stretched with width:100% and height:100% and object-fit omitted. Screenshot (iPhone 11, IOS 14.6): https://i.imgur.com/iYkQVDr.png Other It seems like that his bug related to the fact that srcset="... 400w" is defined for the image. Check this example, which do not have the 400w in the markup and it looks fine: https://smartslider3.com/bugs/webkit/object-fit-srcset/good.html
<rdar://problem/80215534>
Created attachment 432967 [details] Reduction
Doesn't reproduce in a smaller testcase. Need to simplify the content.
One question is whether this is interacting with grid layout.
This isn't specific to srcset. Here is a simpler reduction. Top one uses img with object-fit. Second is the same asset as background image: https://codepen.io/benfrain/pen/PoKPbPR
Ben, I think your bug is different. I filed bug 231754 about it.
Ok Simon, thanks, I’ll track that 👍
This bug is because shouldApplySizeContainment() is breaking object-fit: LayoutSize intrinsicSize() const final { if (shouldApplySizeContainment(*this)) return LayoutSize(); return m_intrinsicSize; } LayoutRect RenderReplaced::replacedContentRect(const LayoutSize& intrinsicSize) const { LayoutRect contentRect = contentBoxRect(); if (intrinsicSize.isEmpty()) return contentRect; Cathie, could you take a look?
(In reply to Simon Fraser (smfr) from comment #8) > This bug is because shouldApplySizeContainment() is breaking object-fit: > > LayoutSize intrinsicSize() const final > { > if (shouldApplySizeContainment(*this)) > return LayoutSize(); > return m_intrinsicSize; > } > > LayoutRect RenderReplaced::replacedContentRect(const LayoutSize& > intrinsicSize) const > { > LayoutRect contentRect = contentBoxRect(); > if (intrinsicSize.isEmpty()) > return contentRect; > > Cathie, could you take a look? I was wrong; shouldApplySizeContainment() is false. But m_intrinsicSize is {0,0} in this content.
Created attachment 441332 [details] test-case-replacing-nodes.html
Created attachment 441333 [details] test-case-having-img.html
Simplified the test case a little bit. It seems the bug is only reproducible by replacing the image nodes. Will keep digging in.
Created attachment 441559 [details] test-case-replacing-nodes.html
Created attachment 441709 [details] Patch
Created attachment 441713 [details] Patch
Created attachment 441715 [details] Patch
Created attachment 441825 [details] Patch
Comment on attachment 441825 [details] Patch Hi, I think this patch is ready for review now:)
Comment on attachment 441825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441825&action=review > Source/WebCore/html/HTMLImageElement.cpp:685 > + if (pictureElement()) > + pictureElement()->sourcesChanged(); Modern way to write this is to make sure to ref the thing before calling a function on it: if (RefPtr element = pictureElement()) element->sourcesChanged(); This seems kind of messy since it will almost always call sourcesChanged twice, once when the picture element moves and again when the image element moves. Might be nice if it was lazy enough not to do it all twice.
(In reply to Darin Adler from comment #19) > Comment on attachment 441825 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441825&action=review > > > Source/WebCore/html/HTMLImageElement.cpp:685 > > + if (pictureElement()) > > + pictureElement()->sourcesChanged(); > > Modern way to write this is to make sure to ref the thing before calling a > function on it: > > if (RefPtr element = pictureElement()) > element->sourcesChanged(); > Done, thanks! > This seems kind of messy since it will almost always call sourcesChanged > twice, once when the picture element moves and again when the image element > moves. Might be nice if it was lazy enough not to do it all twice. Yeah, I was not very sure if we can just remove HTMLPictureElement::didMoveToNewDocument. Double checked, it seems right. If there is img inside picture, then HTMLImageElment::didMoveToNewDocument will call sourcesChanged. If there is no img at all, sourcesChanged does nothing eventually. OK, I'll try to remove HTMLPictureElement::didMoveToNewDocument in the new patch.
Created attachment 441980 [details] Patch
Committed r284667 (243387@main): <https://commits.webkit.org/243387@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441980 [details].
This change should be present in iOS 15.1, and macOS 12.1 or newer.