Bug 207740

Summary: Inline boxes for images that represent nothing should collapse to empty.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, darin, dbates, eric.carlson, esprehn+autocc, ews-watchlist, glenn, hta, jer.noble, kangil.han, kondapallykalyan, pdr, philipj, sergio, simon.fraser, tommyw, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Comment 1 zalan 2020-02-13 19:21:15 PST
<rdar://problem/57339992>
Comment 2 zalan 2020-02-13 19:38:29 PST
Created attachment 390724 [details]
Patch
Comment 3 zalan 2020-02-14 08:15:21 PST
Created attachment 390771 [details]
Patch
Comment 4 zalan 2020-02-14 10:10:41 PST
Created attachment 390780 [details]
Patch
Comment 5 zalan 2020-02-14 11:04:12 PST
Created attachment 390786 [details]
Patch
Comment 6 Simon Fraser (smfr) 2020-02-14 11:48:42 PST
Comment on attachment 390786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390786&action=review

> Source/WebCore/ChangeLog:11
> +        Let's collapse inline boxes when we would display only a broken image and

Say why, not just "let's". Preferably link to the spec.
Comment 7 zalan 2020-02-14 11:56:54 PST
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 390786 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390786&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Let's collapse inline boxes when we would display only a broken image and
> 
> Say why, not just "let's". Preferably link to the spec.
Sure, in the final patch. It's a WIP for EWS.
Comment 8 zalan 2020-02-14 12:44:37 PST
Created attachment 390798 [details]
Patch
Comment 9 zalan 2020-02-16 09:57:13 PST
Created attachment 390888 [details]
Patch
Comment 10 Darin Adler 2020-02-16 12:24:19 PST
Comment on attachment 390888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390888&action=review

Probably not quite expert enough to review, but spotted two small things

> Source/WebCore/rendering/RenderImage.cpp:265
> +        return imageResource().errorOccurred() && (m_altText.isEmpty() || m_altText.isNull());

Null is also empty, so no need for "|| .isNull" clause here.

> Source/WebCore/rendering/RenderImage.cpp:276
> +    if (element() && shouldCollapseToEmpty())

Can the null check for element go inside the shouldCollapseToEmpty function? Before I noticed every call doing this check I was going to ask about the assumption in that function that element can’t be null.
Comment 11 zalan 2020-02-16 12:27:56 PST
Created attachment 390891 [details]
Patch
Comment 12 zalan 2020-02-16 12:38:50 PST
(In reply to Darin Adler from comment #10)
> Comment on attachment 390888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390888&action=review
> 
> Probably not quite expert enough to review, but spotted two small things
> 
> > Source/WebCore/rendering/RenderImage.cpp:265
> > +        return imageResource().errorOccurred() && (m_altText.isEmpty() || m_altText.isNull());
> 
> Null is also empty, so no need for "|| .isNull" clause here.
Thanks! Clearly I don't use String class very often.

> 
> > Source/WebCore/rendering/RenderImage.cpp:276
> > +    if (element() && shouldCollapseToEmpty())
> 
> Can the null check for element go inside the shouldCollapseToEmpty function?
> Before I noticed every call doing this check I was going to ask about the
> assumption in that function that element can’t be null.
Yeah, I moved it there (I had a previous version of this patch where the check was in shouldCollapseToEmpty(), but then I thought maybe shouldCollapseToEmpty() should only have the relevant logic. I think it's safer this way though.)
Comment 13 Darin Adler 2020-02-16 14:38:29 PST
(In reply to zalan from comment #12)
> (In reply to Darin Adler from comment #10)
> > Can the null check for element go inside the shouldCollapseToEmpty function?
> > Before I noticed every call doing this check I was going to ask about the
> > assumption in that function that element can’t be null.
> Yeah, I moved it there (I had a previous version of this patch where the
> check was in shouldCollapseToEmpty(), but then I thought maybe
> shouldCollapseToEmpty() should only have the relevant logic. I think it's
> safer this way though.)

I agree that while this is safer, it’s not as conceptually clear and pure given the function name. Maybe best mitigated by writing a brief comment inside the function explaining why returning false for the case where there is no element is always a good idea.
Comment 14 zalan 2020-02-16 16:59:36 PST
(In reply to Darin Adler from comment #13)
> (In reply to zalan from comment #12)
> > (In reply to Darin Adler from comment #10)
> > > Can the null check for element go inside the shouldCollapseToEmpty function?
> > > Before I noticed every call doing this check I was going to ask about the
> > > assumption in that function that element can’t be null.
> > Yeah, I moved it there (I had a previous version of this patch where the
> > check was in shouldCollapseToEmpty(), but then I thought maybe
> > shouldCollapseToEmpty() should only have the relevant logic. I think it's
> > safer this way though.)
> 
> I agree that while this is safer, it’s not as conceptually clear and pure
> given the function name. Maybe best mitigated by writing a brief comment
> inside the function explaining why returning false for the case where there
> is no element is always a good idea.
Agreed!
Comment 15 zalan 2020-02-16 17:13:58 PST
Created attachment 390893 [details]
Patch
Comment 16 zalan 2020-02-16 17:14:47 PST
(In reply to Darin Adler from comment #13)
> (In reply to zalan from comment #12)
> > (In reply to Darin Adler from comment #10)
> > > Can the null check for element go inside the shouldCollapseToEmpty function?
> > > Before I noticed every call doing this check I was going to ask about the
> > > assumption in that function that element can’t be null.
> > Yeah, I moved it there (I had a previous version of this patch where the
> > check was in shouldCollapseToEmpty(), but then I thought maybe
> > shouldCollapseToEmpty() should only have the relevant logic. I think it's
> > safer this way though.)
> 
> I agree that while this is safer, it’s not as conceptually clear and pure
> given the function name. Maybe best mitigated by writing a brief comment
> inside the function explaining why returning false for the case where there
> is no element is always a good idea.
Thanks for the review!
Comment 17 Simon Fraser (smfr) 2020-02-17 10:59:02 PST
Comment on attachment 390893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390893&action=review

> Source/WebCore/ChangeLog:16
> +        https://www.w3.org/TR/2016/WD-html51-20160412/rendering.html#replaced-elements-images

It's weird to reference an out-of-date W3C HTML spec. Is this in the WHATWG spec? Or in CSS?

> Source/WebCore/rendering/RenderImage.cpp:269
> +        // category of possible unwanted content.

possibly unwanted?

Does this mean that it's pseudoelement content? Is there a more explicit check for this?

> Source/WebCore/rendering/RenderImage.cpp:304
> +    if (shouldCollapseToEmpty()) {
> +        // Image might need resizing when we are at the final state.
> +        setNeedsLayout();
> +    }

What about the other direction? Good image swapping to a bad image? Do we get here?
Comment 18 zalan 2020-02-17 12:53:04 PST
Created attachment 390961 [details]
Patch
Comment 19 zalan 2020-02-17 12:55:29 PST
(In reply to Simon Fraser (smfr) from comment #17)
> Comment on attachment 390893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390893&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        https://www.w3.org/TR/2016/WD-html51-20160412/rendering.html#replaced-elements-images
> 
> It's weird to reference an out-of-date W3C HTML spec. Is this in the WHATWG
> spec? Or in CSS?
I didn't even notice it was an out-of-date spec. Updated to WHATWG.

> 
> > Source/WebCore/rendering/RenderImage.cpp:269
> > +        // category of possible unwanted content.
> 
> possibly unwanted?
> 
> Does this mean that it's pseudoelement content? Is there a more explicit
> check for this?
> 
> > Source/WebCore/rendering/RenderImage.cpp:304
> > +    if (shouldCollapseToEmpty()) {
> > +        // Image might need resizing when we are at the final state.
> > +        setNeedsLayout();
> > +    }
> 
> What about the other direction? Good image swapping to a bad image? Do we
> get here?
Good->bad case is actually what is covered here. The bad->good case is covered implicitly through geometry changes since the "bad" state has (0, 0) dimension.
Comment 20 WebKit Commit Bot 2020-02-17 14:23:08 PST
Comment on attachment 390961 [details]
Patch

Clearing flags on attachment: 390961

Committed r256773: <https://trac.webkit.org/changeset/256773>
Comment 21 WebKit Commit Bot 2020-02-17 14:23:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 youenn fablet 2020-02-17 14:48:20 PST
Reopening to attach new patch.
Comment 23 youenn fablet 2020-02-17 14:48:21 PST
Created attachment 390986 [details]
Patch
Comment 24 youenn fablet 2020-02-17 14:50:53 PST
Comment on attachment 390986 [details]
Patch

Wrong bug