Bug 79526 – Incorrect handling of sizes in "em" when first-line changes font size

• Component: CSS
• Status: RESOLVED FIXED

[Boris Zbarsky]

Created attachment 128791 [details]
Testcase

See attached testcase.  Presto and Gecko get this right; the left border of the <span> should be 10px wide.  Trident seems to get is half-wrong: it leaves 100px of space but paints a 10px border.  WebKit just gets this wrong.

[Boris Zbarsky]

Created attachment 128792 [details]
Testcase without the typo

[Grace Ku]

Created attachment 130159 [details]
proposed fix for 79626

[WebKit Review Bot]

Attachment 130159 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/InlineFlowBox.cpp..." exit_code: 1
Source/WebCore/rendering/RenderBoxModelObject.cpp:2556:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/rendering/RenderBoxModelObject.h:196:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 130159 [details]
proposed fix for 79626

Attachment 130159 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11833438

[Grace Ku]

Created attachment 130441 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 130441 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        When pseudo class first-line changes the font size, the "em" unit is handled incorrectly as the element's original
> +        size (the size of the rest of the paragraph) rather than the font-size of the first-line of the element. This was
> +        corrected by checking if the InlineFlowBox was the first line using the existing InlineFlowBox::isFirstLineStyle()
> +        function.

You should include commentary here on how this does or does not match other browsers behavior, and what the CSS spec says on the topic.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2586
> +    getBorderEdgeInfo(edges, this->style());

Why this->style() here?  I don't se a "style" local to conflict.

[Grace Ku]

Created attachment 130721 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 130721 [details]
Patch

r=me.

[Tom Zakrajsek]

Comment on attachment 130721 [details]
Patch

cq+

[WebKit Review Bot]

Comment on attachment 130721 [details]
Patch

Clearing flags on attachment: 130721

Committed r110769: <http://trac.webkit.org/changeset/110769>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This patch appears to require image baselines but was landed without any.

[Grace Ku]

(In reply to comment #12)
> This patch appears to require image baselines but was landed without any.

I didn't think it required a pixel test because, in the expected render dump, the actual location of RenderText that the border appears to the left of must be exactly (360,0) after the patch is applied.

>          RenderText {#text} at (360,0) size 50x10
>            text run at (360,0) width 50: "this,"

Before the patch, it would be (450,0).

...Though, I suppose this only verifies that the word "this" has been shifted over the appropriate amount of space for the border. Theoretically the border painting could be completely wrong and the location would still be correct.