[webkit-reviews] review denied: [Bug 53239] Wrapping text overlaps floated element(s) when there are 2 or more adjacent floated elements : [Attachment 99853] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Jacky Jiang <zkjiang008 at gmail.com>'s
request for review:
Bug 53239: Wrapping text overlaps floated element(s) when there are 2 or more
adjacent floated elements
https://bugs.webkit.org/show_bug.cgi?id=53239

Attachment 99853: Patch
https://bugs.webkit.org/attachment.cgi?id=99853&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
I think it's wrong to patch logicalLeftOffsetForLine and
logicalRightOffsetForLine. Those methods are behaving correctly in the sense
that they just look at a particular y-coordinate and act accordingly.

Really the issue is that we only sample at the top of the line in the line
layout code. The reason we do this is that we don't actually know how tall the
line is going to be. Your patch is trying to just use the line-height of the
block, but of course that isn't necessarily how tall the line of text will end
up being. In quirks mode it can be shorter than that, and if there are taller
items on the line, it can be larger.

This has been a known issue for years... I just haven't come up with a great
way to address it without having to "be wrong" and do another pass (which can
itself be cyclic if you keep changing your mind).

I think using the line-height of the block is somewhat reasonable, but it's
tricky. The two issues I can think of with using the line-height of the block
as a guide are that in quirks mode the descent may go away, and then you have
to consider -webkit-line-box-contain also, since if the block isn't being
allowed to contribute to line-height, it definitely seems wrong to use it.

This is a really challenging bug to fix.