Bug 18413 – hixie test fails for CSS: sanity tests for relative keyword values of font-size

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[jasneet]

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

[jasneet]

Created attachment 20462 [details]
screenshot

[Robert Blaut]

jasneet, you should have Ahem font (http://webkit.org/quality/Ahem.ttf) installed to properly test this case.

[Anatoli Papirovski]

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 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.

[Anatoli Papirovski]

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 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 on attachment 20996 [details]
patch 2

Marking r- because of the larger->smaller round-trip issue and other comments.

[Anatoli Papirovski]

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 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.

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 53237 [details]
patch4 part2

[Dimitri Glazkov (Google)]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Xianzhu Wang]

(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.

[WebKit Review Bot]

Attachment 53236 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2285315

[Kent Tamura]

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.

[Xianzhu Wang]

(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.

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 62384 [details]
Updated patch (not containing pixel test updates to reduce the size)

[Eric Seidel (no email)]

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.

[Xianzhu Wang]

(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)?

[Eric Seidel (no email)]

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).

[Xianzhu Wang]

Created attachment 94763 [details]
Updated patch containing only changed code and added test case

[WebKit Review Bot]

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.

[Xianzhu Wang]

Created attachment 94765 [details]
Updated patch containing only changed code and added test case (fixed style issues)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

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).

[Xianzhu Wang]

(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.

[Ahmad Saleem]

Created attachment 459835 [details]
Safari 15.4 matches Chrome 104 and Firefox 102

All browsers now match on test case. Is this needed anymore or can it be closed?

[Alexey Proskuryakov]

Thank you for checking! Marking as resolved, one of the CC'ed folks can reopen if needed.