Bug 38891 - REGRESSION (r57657): Incorrect font-size of :first-letter when using cufon.js
Summary: REGRESSION (r57657): Incorrect font-size of :first-letter when using cufon.js
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P1 Major
Assignee: Dave Hyatt
URL: http://l-c-n.com/
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-05-11 01:28 PDT by Philippe Wittenbergh
Modified: 2010-05-12 12:04 PDT (History)
4 users (show)

See Also:


Attachments
screenshot (45.20 KB, image/png)
2010-05-11 01:28 PDT, Philippe Wittenbergh
no flags Details
Patch (3.77 KB, patch)
2010-05-12 11:54 PDT, Dave Hyatt
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Wittenbergh 2010-05-11 01:28:46 PDT
Created attachment 55675 [details]
screenshot

It is enough to have the page link to the script, it does not need to be actually used.
Minimal testcase:
http://dev.l-c-n.com/webkit/test.html

The font-size in the test case is twice as large as it should be.
Safari 4.05, Gecko 1.9.2 (fx 3.6.3) and Opera 10.5 are correct.

This regressed between WebKit-SVN-r57509 and WebKit-SVN-r57720

cufon.js
http://cufon.shoqolate.com/generate/

(only tested on 10.6.3)

In the attached screenshot, on the left WebKit nightly build, on the right Safari 4.05
Comment 1 Alexey Proskuryakov 2010-05-11 23:04:32 PDT
Works in r57654, fails in r57659, and r57657 is the only suspicious change in the range.

https://bugs.webkit.org/show_bug.cgi?id=37567, :first-letter inside a :visited link is wrong color.  Make sure that the pseudo style caching allows visited link styles to hang off other pseudo styles.
Comment 2 Philippe Wittenbergh 2010-05-11 23:39:27 PDT
If you want some more fun :~]

1. Load http://l-c-n.com/
   (pay attention to the size of the first letter in the headlines)
2. make window narrower (less than ~650px)
3. the layout changes (correct - media query kicks in)
   but the size of the :first-letter changes ! (becomes correct)
Comment 3 Alexey Proskuryakov 2010-05-12 10:23:22 PDT
<rdar://problem/7974548>
Comment 4 Dave Hyatt 2010-05-12 11:54:21 PDT
Created attachment 55881 [details]
Patch
Comment 5 WebKit Review Bot 2010-05-12 11:55:56 PDT
Attachment 55881 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderBlock.cpp:4654:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Darin Adler 2010-05-12 11:56:21 PDT
Comment on attachment 55881 [details]
Patch

> -    while (currChild && currChild->needsLayout() && ((!currChild->isReplaced() && !currChild->isRenderButton() && !currChild->isMenuList()) || currChild->isFloatingOrPositioned()) && !currChild->isText()) {
> +    while (currChild && ((!currChild->isReplaced() && !currChild->isRenderButton() && !currChild->isMenuList()) || currChild->isFloatingOrPositioned()) && !currChild->isText()) {

Is there a way to make this more readable with a helper function for the boolean test here?

> -            if (currChild->style()->styleType() == FIRST_LETTER)
> +            if (currChild->style()->styleType() == FIRST_LETTER) {
> +                currChild = currChild->firstChild();
>                  break;
> +            } else
> +                currChild = currChild->nextSibling();

No need for else after break.

r=me
Comment 7 Dave Hyatt 2010-05-12 12:04:13 PDT
Fixed in r59247.