Summary: | Incorrect handling of sizes in "em" when first-line changes font size | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Boris Zbarsky <bzbarsky> | ||||||||||||
Component: | CSS | Assignee: | 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: |
|
Created attachment 128792 [details]
Testcase without the typo
Created attachment 130159 [details]
proposed fix for 79626
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 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 Created attachment 130441 [details]
Patch
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. Created attachment 130721 [details]
Patch
Comment on attachment 130721 [details]
Patch
r=me.
Comment on attachment 130721 [details]
Patch
cq+
Comment on attachment 130721 [details] Patch Clearing flags on attachment: 130721 Committed r110769: <http://trac.webkit.org/changeset/110769> All reviewed patches have been landed. Closing bug. This patch appears to require image baselines but was landed without any. (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. |
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.