Bug 207740 – Inline boxes for images that represent nothing should collapse to empty.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

https://www.w3.org/TR/2016/WD-html51-20160412/rendering.html#replaced-elements-images

[zalan]

<rdar://problem/57339992>

[zalan]

Created attachment 390724 [details]
Patch

[zalan]

Created attachment 390771 [details]
Patch

[zalan]

Created attachment 390780 [details]
Patch

[zalan]

Created attachment 390786 [details]
Patch

[Simon Fraser (smfr)]

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.

[zalan]

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

[zalan]

Created attachment 390798 [details]
Patch

[zalan]

Created attachment 390888 [details]
Patch

[Darin Adler]

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.

[zalan]

Created attachment 390891 [details]
Patch

[zalan]

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

[Darin Adler]

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

[zalan]

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

[zalan]

Created attachment 390893 [details]
Patch

[zalan]

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

[Simon Fraser (smfr)]

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?

[zalan]

Created attachment 390961 [details]
Patch

[zalan]

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

[WebKit Commit Bot]

Comment on attachment 390961 [details]
Patch

Clearing flags on attachment: 390961

Committed r256773: <https://trac.webkit.org/changeset/256773>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 390986 [details]
Patch

[youenn fablet]

Comment on attachment 390986 [details]
Patch

Wrong bug