Bug 227682 – Object-fit:cover and img source with srcset cause rendering issue

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Roland Soos]

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

[Radar WebKit Bug Importer]

<rdar://problem/80215534>

[Simon Fraser (smfr)]

Created attachment 432967 [details]
Reduction

[Simon Fraser (smfr)]

Doesn't reproduce in a smaller testcase. Need to simplify the content.

[Simon Fraser (smfr)]

One question is whether this is interacting with grid layout.

[Ben Frain]

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

[Simon Fraser (smfr)]

Ben, I think your bug is different. I filed bug 231754 about it.

[Ben Frain]

Ok Simon, thanks, I’ll track that 👍

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

(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.

[cathiechen]

Created attachment 441332 [details]
test-case-replacing-nodes.html

[cathiechen]

Created attachment 441333 [details]
test-case-having-img.html

[cathiechen]

Simplified the test case a little bit.
It seems the bug is only reproducible by replacing the image nodes.
Will keep digging in.

[cathiechen]

Created attachment 441559 [details]
test-case-replacing-nodes.html

[cathiechen]

Created attachment 441709 [details]
Patch

[cathiechen]

Created attachment 441713 [details]
Patch

[cathiechen]

Created attachment 441715 [details]
Patch

[cathiechen]

Created attachment 441825 [details]
Patch

[cathiechen]

Comment on attachment 441825 [details]
Patch

Hi,
I think this patch is ready for review now:)

[Darin Adler]

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.

[cathiechen]

(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.

[cathiechen]

Created attachment 441980 [details]
Patch

[EWS]

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].

[Brent Fulgham]

This change should be present in iOS 15.1, and macOS 12.1 or newer.