[webkit-reviews] review denied: [Bug 74723] REGRESSION (r94492): Text is shifted to the right in some buttons in the Mac App Store : [Attachment 120558] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Robert Hogan <robert at webkit.org>'s request for
review:
Bug 74723: REGRESSION (r94492): Text is shifted to the right in some buttons in
the Mac App Store
https://bugs.webkit.org/show_bug.cgi?id=74723

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=120558&action=review


r- because of the regression in the case of lines with only positioned objects
and a <br> in quirks mode. I think you can fix it by adding to LineInfo a
counter of “positioned object runs from leading whitespace”, incrementing it as
needed in skipLeadingWhitespace, then subtracting it from bidiRuns.runCount()
when computing isOnlyRun in constructLine().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1881
> +		   appendRunsForObject(bidiRuns, resolver.position().offset(),
object->length(), object, resolver);

appendRunsForObject() contains a lot of logic that’s irrelevant here; also,
since you know object is a box, you don’t need to call length() (not that the
length matters here). I would just do it like this:
    resolver.runs().addRun(createRun(0, 1, object, resolver);

> LayoutTests/ChangeLog:21
> +	   *
platform/chromium-linux/fast/inline/styledEmptyInlinesWithBRs-expected.png:
> +	   *
platform/chromium-linux/fast/replaced/absolute-position-percentage-height-expec
ted.png:
> +	   *
platform/chromium-win/fast/inline/styledEmptyInlinesWithBRs-expected.txt:
> +	   *
platform/chromium-win/fast/replaced/absolute-position-percentage-height-expecte
d.txt:
> +	   * platform/chromium-win/media/audio-controls-rendering-expected.txt:

> +	       With this patch, WebKit now treats the inline positioned <img>
as an element in the inline run, so the 
> +	       first subsequent <br> does not contribute any height. Before,
the inline positioned element was not in the run 
> +	       so the first <br> was treated as free-standing, contributing
20px of height - hence the smaller height in 
> +	       the updated results. Neither Firefox nor Opera allow the <br>'s
to contribute any height to the rendering of the <img> element.

These changes show that the patch is incorrect. The presence of a positioned
object on the line should not affect how the <br> element behaves. The problem
is that now that it contributes a run, it causes isOnlyRun to be false in
constructLine(), and consequently causes the line to have no text descendants
(for the purposes of the quirks mode behavior) and thus collapse.