Bug 18413 - hixie test fails for CSS: sanity tests for relative keyword values of font-size
Summary: hixie test fails for CSS: sanity tests for relative keyword values of font-size
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC Windows XP
: P4 Minor
Assignee: Nobody
URL: http://www.hixie.ch/tests/adhoc/css/f...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-10 12:41 PDT by jasneet
Modified: 2012-01-30 16:05 PST (History)
11 users (show)

See Also:


Attachments
screenshot (147.48 KB, image/jpeg)
2008-04-10 12:42 PDT, jasneet
no flags Details
patch (432.73 KB, patch)
2008-04-20 19:16 PDT, Anatoli Papirovski
mitz: review-
Details | Formatted Diff | Diff
patch 2 (71.26 KB, patch)
2008-05-06 20:45 PDT, Anatoli Papirovski
mitz: review-
Details | Formatted Diff | Diff
patch3 (396.39 KB, patch)
2009-01-01 22:24 PST, Anatoli Papirovski
mitz: review-
Details | Formatted Diff | Diff
patch4 part1 (containing the changed code and part of changed layout tests) (1.66 MB, patch)
2010-04-13 03:24 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch4 part2 (1.63 MB, patch)
2010-04-13 03:26 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated patch (not containing pixel test updates to reduce the size) (413.90 KB, patch)
2010-07-23 00:09 PDT, Xianzhu Wang
eric: review-
Details | Formatted Diff | Diff
Updated patch containing only changed code and added test case (10.98 KB, patch)
2011-05-25 03:18 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated patch containing only changed code and added test case (fixed style issues) (10.93 KB, patch)
2011-05-25 03:33 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.40 MB, application/zip)
2011-05-25 06:15 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-04-10 12:41:22 PDT
I Steps:
Go to 
http://www.hixie.ch/tests/adhoc/css/fonts/size/003.xml

II Issue:
the red & green "x" has opposite pattern as compared to Firefox.

III Conclusion:
issue with relative keyword values of font-size

IV Other browsers:
IE7: not okay; shows xml code on opening the test link
FF3: ok
Opera9.24: ok

V Nightly tested: 31446
Comment 1 jasneet 2008-04-10 12:42:34 PDT
Created attachment 20462 [details]
screenshot
Comment 2 Robert Blaut 2008-04-16 11:00:36 PDT
jasneet, you should have Ahem font (http://webkit.org/quality/Ahem.ttf) installed to properly test this case.
Comment 3 Anatoli Papirovski 2008-04-20 19:16:33 PDT
Created attachment 20711 [details]
patch

This is not the full patch, I removed the appended pngs... but they're still in the changelogs, so if this is approved I will send over the full one or just attach it to the bug.
Comment 4 mitz 2008-04-21 10:47:10 PDT
Comment on attachment 20711 [details]
patch

This patch is on the right track, but has several issues:

+        Implement larger and smaller keywords for font-size, bug 18413

It is convenient to have the bugs.webkit.org URL in the change log. You should also describe the changes.

-    if (childFont.keywordSize()) {
+     if (childFont.keywordSize()) {

Oops, extra space there.

+    float fallbackSize = size * 1.2f;
+    int modifier = 1;
+    
+    if (!increaseFontSize) {
+        fallbackSize = size / 1.2f;
+        modifier = -1;
+    }

Initializing the variables to one value only to reassign a different value if increaseFontSize is false is somewhat inelegant. Even though it might be one or two more lines of code, I think this would be better:

    float fallbackSize; = size * 1.2f;
    int modifier; = 1;

    if (increaseFontSize) {
        fallbackSize = size * 1.2f;
        modifier = 1;
    }else {
        fallbackSize = size / 1.2f;
        modifier = -1;
    }

Or maybe just
   float fallbackSize = increaseFontSize ? size * 1.2f : size / 1.2f;

And do the same for the modifier variable right before you use it (right now you're defining and initializing it even if it's not used).

+    if (mediumSize >= fontSizeTableMin && mediumSize <= fontSizeTableMax) {

You could check for the opposite condition and return early, and then the rest of the could wouldn't need to be indented.

The definition

+    const int (*currentFontSizeTable)[fontSizeTableMax - fontSizeTableMin + 1] = quirksMode ? quirksFontSizeTable : strictFontSizeTable;

could be moved after checking that the medium size falls inside the table sizes range.

+        //if outside the scope of the table, return approximate value

Please add a space after the //, capitalize the If and end with a period.

+        if (size > currentFontSizeTable[row][totalKeywords-1]) return fallbackSize;

Missing spaces around the minus sign. The return statement  should be on a separate line.

But more importantly, why is the check one-sided? What about sizes smaller than the smallest size in the table?

+    long double deviation = (float) INT_MAX;

I don't think you need a long double for that.

+                if (deviation == 0) break;

The break statement should go on a separate line.

+        if (currentMatch == totalKeywords - 1) 
+            return fallbackSize;

Do we really want to use the fallback here even if we are going to decrease the font size? The given size is between the last two entries on the table, closer to the last one (it cannot be past the last one because of the "if outside the scope" check), so why not snap to the closest value?

+        fontDescription.setKeywordSize(currentMatch+modifier+1);

Missing spaces around the plus signs.

On a higher level, I think it is somewhat confusing  that this function takes a FontDescription and modifies its keyword size, but the numeric size is returned from the function and fed back into the FontDescription by the caller. It would be really great if you could rework it so that either everything happens inside the function or it just takes a reference to a keyword size and sets it.
Comment 5 Anatoli Papirovski 2008-05-06 20:45:20 PDT
Created attachment 20996 [details]
patch 2

Ok, this took a while, but I pretty much completely changed what it does... now the behavior matches Firefox. If the original value is within the table or within 0.5 of the table, then just find the next/previous value from the table, otherwise extrapolate a value based on the position between the two closest values. Also, now it doesn't just use ratios if medium size is outside the range, it creates its own table and then uses that to do everything else.

I'm not a fan of how the code looks now, but I'm having hard time coming up with anything better, so any ideas are welcome... or if it's fine the way it is, that's fine by me.

A ton of test results need to be regenerated btw, because they use larger or smaller keywords or use big/small html tags, but I didn't put those in the patch.
Comment 6 mitz 2008-05-08 22:41:55 PDT
Comment on attachment 20996 [details]
patch 2

+    Settings* settings = m_document->settings();
+    if (!settings)
+        return fallbackSize;

I think you should not define fallbackSize and assign to it before checking settings, because you only use the 1.2 factor in this early return case.

+    const int (*currentFontSizeTable);

Why the parentheses?

+    // If medium size is not within existing tables, then create new table using fontSizeFactors,
+    // otherwise just point to the correct row of an existing table.
+    if (!(mediumSize >= fontSizeTableMin && mediumSize <= fontSizeTableMax)) {

Please reverse the order so that you check for the non-negated (and perhaps common) condition first.

+        for (i = 0; i < totalKeywords; i++) {
+            customFontSizeTable[i] = mediumSize * fontSizeFactors[i];
+        }

The WebKit coding style is that single-line blocks do not have braces around them.

+        currentFontSizeTable = (const int *) customFontSizeTable;

I think you don't need this cast.

Setting this to point to a variable that goes out of scope by the time you dereference the variable seems fragile (I am not a C++ expert so I do not know whether it is in fact safe). I would define customFontSizeTable outside the if statement. But do you really want to define this table and populate it when you may end up not even comparing with all the entries in it? I guess it allows you handle the "table found" and "no table found" cases uniformly.

+    float deviation = (float) INT_MAX;

Please use static_cast.

+    // If outside the scope of the table, return approximate value.
+    if ((size > currentFontSizeTable[totalKeywords - 1] && increaseFontSize) || (size < currentFontSizeTable[0] && !increaseFontSize))
+        return fallbackSize;

Why not >= and <=, respectively?

+    for (i = 0; i < totalKeywords; i++) {
+        if (fabsf(size - currentFontSizeTable[i]) <= fabsf(deviation)) {
+            currentMatch = i;
+            deviation = size - currentFontSizeTable[i];
+            if (roundf(deviation) == 0)
+                break;
+        }
+    }

Seems to me like you are not really interested in which table value you are closest to, but rather which two table values you lie between.
   
+    int modifier = increaseFontSize ? 1 : -1;

You can move this definition further down.

+    // This is set-up to match the Mozilla behavior. They determine what the relative position
+    // of the font-size is between fontSizeA and fontSizeB and then put it in the same relative
+    // position only between fontSizeB and fontSizeC.

Kind of weird that it's using arithmetic rather than geometric (weighted) means given that this is essentially multiplicative.

+    float distance, position, newDeviation;
+    int fontSizeA, fontSizeB, fontSizeC;

Please declare each variable in a separate statement.

+    if ((deviation > 0 && increaseFontSize) || (deviation < 0 && !increaseFontSize)) {
+        fontSizeA = currentFontSizeTable[currentMatch];
+        fontSizeB = currentFontSizeTable[currentMatch + modifier];
+        if (currentMatch == totalKeywords - 2 && increaseFontSize)
+            fontSizeC = fontSizeB * 1.5f;
+        else if (currentMatch == 1 && !increaseFontSize)
+            fontSizeC = fontSizeB / 1.1f;
+        else
+            fontSizeC = currentFontSizeTable[currentMatch + modifier * 2];
+    } else {
+        fontSizeB = currentFontSizeTable[currentMatch];
+        fontSizeC = currentFontSizeTable[currentMatch + modifier];
+        if (currentMatch == totalKeywords - 1)
+            fontSizeA = fontSizeB * 1.5f;
+        else if (currentMatch == 0) 
+            fontSizeA = fontSizeB / 1.1f;
+        else
+            fontSizeA = currentFontSizeTable[currentMatch - modifier];
+    }

I think this can be simplified along with changing the loop that finds "currentMatch".

We talked about behavior with sizes that lie outside the table and you said you'd fix the bug I pointed out to you. I think it might be better to stick to 1.5 above and 1.1 below if that's what Firefox does.
Comment 7 mitz 2008-05-09 18:01:40 PDT
Comment on attachment 20996 [details]
patch 2

Marking r- because of the larger->smaller round-trip issue and other comments.
Comment 8 Anatoli Papirovski 2009-01-01 22:24:01 PST
Created attachment 26360 [details]
patch3

Whew. That took a while... real life took over, but here goes the update. Works EXACTLY like Gecko/Firefox. Fixes all of the issues above, I believe. With that said, having not worked on WebKit for 6 months I might've screwed up something when creating the patch... so, that's that.

Hopefully we can get this in soon, it's kind of frustrating that WebKit still does not support proper relative font-size keywords.
Comment 9 mitz 2009-01-06 11:48:12 PST
Comment on attachment 26360 [details]
patch3

Thanks for coming back to this.

> +    int i;

In WebKit C++ code, loop iterators are usually defined inside the for statement. If the iterator is used outside the loop's scope, then it should be defined just before the loop.

> +    const int *currentFontSizeTable;

The * should go next to the type.

> +        currentFontSizeTable = const_cast<int *> (customFontSizeTable);

There should not be a space before the *. There also shouldn't be a space before the left parenthesis.

> +    // If outside the scope of the table, return approximate value.
> +    if (size >= currentFontSizeTable[totalKeywords - 1])
> +        return increaseFontSize ? size * (static_cast<float> (currentFontSizeTable[totalKeywords - 1]) / static_cast<float> (currentFontSizeTable[totalKeywords - 2]))
> +                                : size / (static_cast<float> (currentFontSizeTable[totalKeywords - 1]) / static_cast<float> (currentFontSizeTable[totalKeywords - 2]));
> +    
> +    if (size < currentFontSizeTable[0] || (size == currentFontSizeTable[0] && !increaseFontSize))
> +        return size + static_cast<float> (modifier);

Why is adding or subtracting 1 the correct behavior for sizes smaller than the smallest size in the table? Wouldn't a multiplicative scale factor make more sense? The asymmetry with the other side is odd.

> +    int currentMatch = 0;
> +    float deviation = static_cast<float> (INT_MAX);

There should not be a space before the left parenthesis.

> +    // find which two values from the table the size falls between
> +    for (i = 0; i < totalKeywords - 1; i++) {
> +        if (size - currentFontSizeTable[i] < currentFontSizeTable[i + 1] - currentFontSizeTable[i] && size >= currentFontSizeTable[i]) {

No need to subtract currentFontSizeTable[i] from both sides of the inequality.

> +    float newDeviation;
> +    float position = deviation / static_cast<float> (currentFontSizeTable[currentMatch + 1] - currentFontSizeTable[currentMatch]);

No need to cast because deviation is a float.

> +    if (increaseFontSize) {
> +        if (currentMatch == totalKeywords - 2)
> +            newDeviation = position * ((static_cast<float> (currentFontSizeTable[currentMatch+1]) * static_cast<float> (currentFontSizeTable[currentMatch+1]) / static_cast<float> (currentFontSizeTable[currentMatch]))
> +                                            - static_cast<float> (currentFontSizeTable[currentMatch + 1]));

Not all of the casts are necessary. There should be spaces around the + operator and no spaces between > and (.

> +        else
> +            newDeviation = position * static_cast<float> (currentFontSizeTable[currentMatch + 2] - currentFontSizeTable[currentMatch + 1]);
> +    } else
> +        newDeviation = position * static_cast<float> (currentFontSizeTable[currentMatch] - currentFontSizeTable[currentMatch - 1]);

How do you know that currentMatch is greater than 0?

> +    if (roundf(newDeviation) == 0) {
> +        fontDescription.setKeywordSize(currentMatch + modifier + 1);
> +        return currentFontSizeTable[currentMatch + modifier];
> +    }
> +    
> +    return currentFontSizeTable[currentMatch + modifier] + newDeviation;

Same question.
Comment 10 Xianzhu Wang 2010-04-13 03:24:51 PDT
Created attachment 53236 [details]
patch4 part1 (containing the changed code and part of changed layout tests)

Anatoli allowed me to continue his work on this issue.
Here is the updated patch based on the last patch.

> Why is adding or subtracting 1 the correct behavior for sizes smaller than the
> smallest size in the table? Wouldn't a multiplicative scale factor make more
> sense? The asymmetry with the other side is odd.

Yes, the asymmetry looks odd, but Mozilla does exactly the same.
I think we'd better keep consistent with Mozilla.
Comment 11 Xianzhu Wang 2010-04-13 03:26:56 PDT
Created attachment 53237 [details]
patch4 part2
Comment 12 Dimitri Glazkov (Google) 2010-05-04 10:00:30 PDT
You should mark the patches for review if you want to get them reviewed (see http://webkit.org/coding/contributing.html). Also, make sure to put all changes into one patch. Landing these patches separately seems like a a lot of work.
Comment 13 WebKit Review Bot 2010-05-11 02:36:14 PDT
Attachment 53236 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:6031:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/css/CSSStyleSelector.cpp:6052:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Review Bot 2010-05-11 02:37:50 PDT
Attachment 53237 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Xianzhu Wang 2010-05-11 02:40:20 PDT
(In reply to comment #12)
> You should mark the patches for review if you want to get them reviewed (see http://webkit.org/coding/contributing.html). Also, make sure to put all changes into one patch. Landing these patches separately seems like a a lot of work.

Thanks for your reminding. The reason of splitting the patch is that Bugzilla doesn't allow attachments bigger than 2M bytes. The patch is big because it contains effected images of layout tests.
Comment 16 WebKit Review Bot 2010-05-20 00:33:19 PDT
Attachment 53236 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2285315
Comment 17 Kent Tamura 2010-06-19 09:58:14 PDT
Comment on attachment 53236 [details]
patch4 part1 (containing the changed code and part of changed layout tests)

Update for security/block-test.html looks not related to the code change.
Comment 18 Xianzhu Wang 2010-06-20 19:36:26 PDT
(In reply to comment #17)
> (From update of attachment 53236 [details])
> Update for security/block-test.html looks not related to the code change.

You're right. Thanks!

Hopefully I'll get a new Mac book with Snow Leopard so that I can create new layout test expectations.
Comment 19 Xianzhu Wang 2010-06-20 19:37:25 PDT
Comment on attachment 53236 [details]
patch4 part1 (containing the changed code and part of changed layout tests)

I'll create a new patch after I get a Snow Leopard mac book.
Comment 20 Xianzhu Wang 2010-07-23 00:09:42 PDT
Created attachment 62384 [details]
Updated patch (not containing pixel test updates to reduce the size)
Comment 21 Eric Seidel (no email) 2011-05-23 10:58:28 PDT
Comment on attachment 62384 [details]
Updated patch (not containing pixel test updates to reduce the size)

This gigantic patch has not been reviewed in 6 months.  It's not gonna get reviewed as-is.  Please find a way to do this in smaller pieces so it can be reviewed.
Comment 22 Xianzhu Wang 2011-05-23 23:47:13 PDT
(In reply to comment #21)
> (From update of attachment 62384 [details])
> This gigantic patch has not been reviewed in 6 months.  It's not gonna get reviewed as-is.  Please find a way to do this in smaller pieces so it can be reviewed.

Thanks Eric for bringing this old issue up.

I'm wondering what the common practice is if a small code change needs to rebaseline many layout tests. May I split the changes to the layout tests into another patch (so the patch of code change could be small)?
Comment 23 Eric Seidel (no email) 2011-05-24 07:02:33 PDT
The patch looks reasonable to me.  As you note, it's actually quite a small code change in the end.  I suspect the best way to get this reviewed is to pester dhyatt in #webkit.

Looks like he wasn't even CC'd.  I don't feel well enough versed in CSS font sizing rules to understand if this approach is OK.

Feel free to mark this r? again.

right now the only way to deal with mass rebaselines is to just land it and pull the new results off hte bots.(or to ask folks from various platforms to try your patch and upload new results for you).
Comment 24 Xianzhu Wang 2011-05-25 03:18:47 PDT
Created attachment 94763 [details]
Updated patch containing only changed code and added test case
Comment 25 WebKit Review Bot 2011-05-25 03:20:50 PDT
Attachment 94763 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.h:145:  The parameter name "fontDescription" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSStyleSelector.h:148:  The parameter name "fontDescription" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSStyleSelector.h:151:  The parameter name "fontDescription" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Xianzhu Wang 2011-05-25 03:33:07 PDT
Created attachment 94765 [details]
Updated patch containing only changed code and added test case (fixed style issues)
Comment 27 WebKit Review Bot 2011-05-25 06:14:59 PDT
Comment on attachment 94765 [details]
Updated patch containing only changed code and added test case (fixed style issues)

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

New failing tests:
css1/font_properties/font_size.html
fast/inline/outline-continuations.html
editing/selection/3690703-2.html
fast/dom/HTMLProgressElement/progress-element.html
fast/text/basic/generic-family-changes.html
tables/mozilla_expected_failures/marvin/backgr_layers-show.html
css2.1/t1507-c526-font-sz-00-b.html
fast/css-generated-content/before-with-first-letter.html
editing/selection/3690703.html
editing/selection/3690719.html
fast/invalid/residual-style.html
Comment 28 WebKit Review Bot 2011-05-25 06:15:05 PDT
Created attachment 94773 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 29 Ryosuke Niwa 2012-01-30 15:33:57 PST
Comment on attachment 94765 [details]
Updated patch containing only changed code and added test case (fixed style issues)

Do we really need to add this massive function?  And there's no way we can share code with findNearestLegacyFontSize? It seems like what you want to do is fontSizeForKeyword(legacyFontSize(...) + increaseFontSize ? 1 : -1).
Comment 30 Xianzhu Wang 2012-01-30 16:05:21 PST
(In reply to comment #29)
> (From update of attachment 94765 [details])
> Do we really need to add this massive function?  And there's no way we can share code with findNearestLegacyFontSize? It seems like what you want to do is fontSizeForKeyword(legacyFontSize(...) + increaseFontSize ? 1 : -1).

My first patch is older than findNearestLegacyFontSize(), and didn't notice it when making the later patches :)

By reusing findNearestLegacyFontSize(), the size of the function can only be reduced by a few lines. Most of the lines in relativeFontSize() handle the cases that the original font size is not in the table.

Canceled the review request because the patch is out-dated. Also lowered the priority and severity. Unlikely to get chance to work on it in the near future.