Summary: | Inline boxes for images that represent nothing should collapse to empty. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
zalan
2020-02-13 19:20:48 PST
Created attachment 390724 [details]
Patch
Created attachment 390771 [details]
Patch
Created attachment 390780 [details]
Patch
Created attachment 390786 [details]
Patch
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. (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. Created attachment 390798 [details]
Patch
Created attachment 390888 [details]
Patch
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. Created attachment 390891 [details]
Patch
(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.) (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. (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! Created attachment 390893 [details]
Patch
(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 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? Created attachment 390961 [details]
Patch
(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 on attachment 390961 [details] Patch Clearing flags on attachment: 390961 Committed r256773: <https://trac.webkit.org/changeset/256773> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 390986 [details]
Patch
Comment on attachment 390986 [details]
Patch
Wrong bug
|