Bug 79526

Summary: Incorrect handling of sizes in "em" when first-line changes font size
Product: WebKit Reporter: Boris Zbarsky <bzbarsky>
Component: CSSAssignee: Grace Ku <gracek>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, mitz, tomz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Testcase without the typo
none
proposed fix for 79626
none
Patch
none
Patch none

Description Boris Zbarsky 2012-02-24 13:22:17 PST
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.
Comment 1 Boris Zbarsky 2012-02-24 13:23:32 PST
Created attachment 128792 [details]
Testcase without the typo
Comment 2 Grace Ku 2012-03-05 10:51:43 PST
Created attachment 130159 [details]
proposed fix for 79626
Comment 3 WebKit Review Bot 2012-03-06 09:19:45 PST
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.
Comment 4 WebKit Review Bot 2012-03-06 09:53:14 PST
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
Comment 5 Grace Ku 2012-03-06 14:06:54 PST
Created attachment 130441 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-03-07 15:15:05 PST
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.
Comment 7 Grace Ku 2012-03-07 16:31:36 PST
Created attachment 130721 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-03-09 13:56:32 PST
Comment on attachment 130721 [details]
Patch

r=me.
Comment 9 Tom Zakrajsek 2012-03-14 14:55:39 PDT
Comment on attachment 130721 [details]
Patch

cq+
Comment 10 WebKit Review Bot 2012-03-14 15:24:52 PDT
Comment on attachment 130721 [details]
Patch

Clearing flags on attachment: 130721

Committed r110769: <http://trac.webkit.org/changeset/110769>
Comment 11 WebKit Review Bot 2012-03-14 15:25:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Adam Barth 2012-03-14 16:29:26 PDT
This patch appears to require image baselines but was landed without any.
Comment 13 Grace Ku 2012-03-15 17:58:38 PDT
(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.