Bug 227682 - Object-fit:cover and img source with srcset cause rendering issue
Summary: Object-fit:cover and img source with srcset cause rendering issue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 14
Hardware: All Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-05 03:12 PDT by Roland Soos
Modified: 2021-10-21 22:05 PDT (History)
14 users (show)

See Also:


Attachments
Reduction (401 bytes, text/html)
2021-07-06 13:24 PDT, Simon Fraser (smfr)
no flags Details
test-case-replacing-nodes.html (1.99 KB, text/html)
2021-10-14 22:00 PDT, cathiechen
no flags Details
test-case-having-img.html (810 bytes, text/html)
2021-10-14 22:01 PDT, cathiechen
no flags Details
test-case-replacing-nodes.html (1.03 KB, text/html)
2021-10-17 21:10 PDT, cathiechen
no flags Details
Patch (5.25 KB, patch)
2021-10-19 04:02 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2021-10-19 05:55 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2021-10-19 05:57 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (7.22 KB, patch)
2021-10-19 17:31 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (8.56 KB, patch)
2021-10-20 21:10 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Soos 2021-07-05 03:12:46 PDT
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
Comment 1 Radar WebKit Bug Importer 2021-07-06 09:40:14 PDT
<rdar://problem/80215534>
Comment 2 Simon Fraser (smfr) 2021-07-06 13:24:50 PDT Comment hidden (obsolete)
Comment 3 Simon Fraser (smfr) 2021-07-06 13:30:36 PDT
Doesn't reproduce in a smaller testcase. Need to simplify the content.
Comment 4 Simon Fraser (smfr) 2021-07-06 13:48:55 PDT
One question is whether this is interacting with grid layout.
Comment 5 Ben Frain 2021-10-14 04:37:06 PDT
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
Comment 6 Simon Fraser (smfr) 2021-10-14 12:04:41 PDT
Ben, I think your bug is different. I filed bug 231754 about it.
Comment 7 Ben Frain 2021-10-14 12:07:32 PDT
Ok Simon, thanks, I’ll track that 👍
Comment 8 Simon Fraser (smfr) 2021-10-14 12:21:04 PDT
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?
Comment 9 Simon Fraser (smfr) 2021-10-14 12:24:32 PDT
(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.
Comment 10 cathiechen 2021-10-14 22:00:12 PDT
Created attachment 441332 [details]
test-case-replacing-nodes.html
Comment 11 cathiechen 2021-10-14 22:01:52 PDT
Created attachment 441333 [details]
test-case-having-img.html
Comment 12 cathiechen 2021-10-14 22:04:12 PDT
Simplified the test case a little bit.
It seems the bug is only reproducible by replacing the image nodes.
Will keep digging in.
Comment 13 cathiechen 2021-10-17 21:10:54 PDT
Created attachment 441559 [details]
test-case-replacing-nodes.html
Comment 14 cathiechen 2021-10-19 04:02:42 PDT
Created attachment 441709 [details]
Patch
Comment 15 cathiechen 2021-10-19 05:55:47 PDT
Created attachment 441713 [details]
Patch
Comment 16 cathiechen 2021-10-19 05:57:43 PDT
Created attachment 441715 [details]
Patch
Comment 17 cathiechen 2021-10-19 17:31:44 PDT
Created attachment 441825 [details]
Patch
Comment 18 cathiechen 2021-10-19 19:42:07 PDT
Comment on attachment 441825 [details]
Patch

Hi,
I think this patch is ready for review now:)
Comment 19 Darin Adler 2021-10-20 19:11:36 PDT
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.
Comment 20 cathiechen 2021-10-20 20:59:25 PDT
(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.
Comment 21 cathiechen 2021-10-20 21:10:16 PDT
Created attachment 441980 [details]
Patch
Comment 22 EWS 2021-10-21 22:05:50 PDT
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].