Summary: | Object-fit:cover and img source with srcset cause rendering issue | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roland Soos <roland> | ||||||||||||||||||||
Component: | Images | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, cathiechen, cdumez, changseok, contact, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, rbuis, sabouhallawa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | Safari 14 | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=231754 | ||||||||||||||||||||||
Attachments: |
|
Description
Roland Soos
2021-07-05 03:12:46 PDT
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. |