Bug 48578 – HTML5 Conformance Test failure: approved/canvas/canvas_text_font_001.htm

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Canvas

[David Kilzer (:ddkilzer)]

The approved/canvas/canvas_text_font_001.htm test fails with WebKit nightly build r70732 run in Safari 5.0.x.

[Mustafizur Rahaman (rahaman)]

Investigated the issue on r86221, please find the analysis below. Need some help to proceed further on this.

Test-case passes in FF but NOT working in Safari(5.0.3)/Winlauncher setup [r86221])

After going through the HTML5 Canvas spec (http://www.w3.org/TR/2dcontext/#dom-context-2d-font), my understanding is that if any of inherit/initial/default are used in the font property assignment statement (e.g., "20px inherit" OR "inherit 20px"), then it must ignore the font value assignment & continue with the previous font value assigned. This is in contrast to the CSS2.1 specification which allows the usage of "inherit" (The values initial/default are reserved for future use).
Is this understanding correct?

After debugging CSSParser & CSSGrammer.y I have found that:
=> for normal HTML/CSS the code flows through CSSParser::parseSheet() => cssyyparse() => CSSParser::parseValue()
=> for HTML5 Canvas, the code flows through CSSParser::parseDeclaration() => cssyyparse() => CSSParser::parseValue()
In the CSSParser::parseValue(), the handling is same for normal HTML/CSS as well as for HTML5 Canvas, i.e., it is handling the "inherit" in the same way.

However, if my understanding as mentioned above is correct, the handling within the parser should be different (reason: CSS2.1 spec & the HTML Canvas 2D Context spec expects "inherit" to be handled differently).

[Mustafizur Rahaman (rahaman)]

Created attachment 95409 [details]
Propose Patch 

I have uploaded a possible fix for the issue. I have also attached the test case to verify the fix. Can some one please review the fix.

This is my commit to the community, I have tried to follow all the steps mentioned in http://www.webkit.org/coding/contributing.html. Still if I have missed anything, please let me know.

[WebKit Review Bot]

Comment on attachment 95409 [details]
Propose Patch 

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

[Mustafizur Rahaman (rahaman)]

Created attachment 95416 [details]
Patch to further fix the issue with Chromim-Ews

I have fixed the issue with Chromium-EWS in this modified patch. However, I did not understand the error it is showing for Qt-Ews.

[Adam Barth]

Comment on attachment 95416 [details]
Patch to further fix the issue with Chromim-Ews

This patch doesn't seem right.  Why should the CSS parser know about HTML canvas?  There's got to be a better way to solve this problem.

[Mustafizur Rahaman (rahaman)]

Hi Adam,

The reason for this is that "inherit" property should be handled differently if you are parsing a normal HTML file (no canvas) & if you are parsing a HTML canvas. Because HTML canvas spec says the following in http://www.w3.org/TR/2dcontext/#dom-context-2d-font 

"The font IDL attribute, on setting, must be parsed the same way as the 'font' property of CSS (but without supporting property-independent style sheet syntax like 'inherit') "

That's why I thought the CSSParser has to behave differently.

[Mustafizur Rahaman (rahaman)]

Hi Adam,

The reason for this is that "inherit" property should be handled differently if you are parsing a normal HTML file (no canvas) & if you are parsing a HTML canvas. Because HTML canvas spec says the following in http://www.w3.org/TR/2dcontext/#dom-context-2d-font 

"The font IDL attribute, on setting, must be parsed the same way as the 'font' property of CSS (but without supporting property-independent style sheet syntax like 'inherit') "

That's why I thought the CSSParser has to behave differently.

[Mustafizur Rahaman (rahaman)]

Created attachment 95573 [details]
Alternate solution proposed after Adam Barth's comment

As Adam mentioned that it is not a good idea to fix this in CSS Parser, so I tried to modify CanvasRenderingContext2D::setFont() so that if the new font contains"inherit" it should not allow assigning the new font value.

[WebKit Review Bot]

Attachment 95573 [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/html/canvas/CanvasRenderingContext2D.cpp:1800:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mustafizur Rahaman (rahaman)]

Created attachment 95575 [details]
Fix the coding style issue

I have fixed the coding style issue being reported in my previous patch (95573)

[Adam Barth]

Comment on attachment 95575 [details]
Fix the coding style issue

This looks like a better approach.  You probably don't need the explicit call to String.  I don't know enough about how this is supposed to work to complete the review, however.

[Eric Seidel (no email)]

Comment on attachment 95575 [details]
Fix the coding style issue

View in context: https://bugs.webkit.org/attachment.cgi?id=95575&action=review

It's not immediately clear why ignoring the entire value is correct.  Also, what about "INHERIT"?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1755
> +    if (newFont.contains(String("inherit")))

Yeah, explicit String() shouldn't be needed here.

[Eric Seidel (no email)]

The approach seems fine.  We just need better testing.  "inherit 20px" and "inherit" and "INHERIT" and "Inheritance 20px", etc.

We should also consider testing for invalid values like "default" and "initial" as well.  I assume we already handle those correctly?

If this is an unmodified test from a test suite, it's fine to add it as-is, but we should add it in whatever directory that imported test suite is.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #13)
> The approach seems fine.  We just need better testing.  "inherit 20px" and "inherit" and "INHERIT" and "Inheritance 20px", etc.

I have prepared a spreadsheet with different font string & comparing their behavior in Winlauncher, FF & Chrome. I will attach it soon.

> 
> We should also consider testing for invalid values like "default" and "initial" as well.  I assume we already handle those correctly?

If you see the result, the tests in winlauncher/Chrome are failing if the font string contains "inherit" (Case insensitive) OR "initial". Therefor I am checking for both "initial Or "initial" in the new font string.

Currently "default" works perfectly fine. For "20px initial" it works fine but for "initial" it fails. The reason being in CSSParser::parseValue(), we have the following code

    else if (id == CSSValueInitial) {
        if (num != 1)
            return false;
        addProperty(propId, CSSInitialValue::createExplicit(), important);
        return true;
    }

For "20px initial" num!=1 & therefore it returns false & ignores the parsed properties. For "initial" it adds the property(num=1) with value=0 & therefore I assume it is rendering with default font (10 Sans Serif)

> 
> If this is an unmodified test from a test suite, it's fine to add it as-is, but we should add it in whatever directory that imported test suite is.

I could not find any directory inside LayoutTests to contains test cases from http://w3c-test.org/html/tests/approved/canvas/ & therefore I have kept the test inside fast/

[Mustafizur Rahaman (rahaman)]

Created attachment 95741 [details]
Comparison of behavior for Winlauncher/Chrome/FF for font "inherit" property

This attachment contains browser comparison result for font "inherit" property based on Eric's question.

[Mustafizur Rahaman (rahaman)]

Created attachment 95743 [details]
Patch:Incorporating Eric's comment

Patch to incorporate Eric's comments.
==> checking for "inherit" OR "initial"
==> checking is done without case-sensitive.
==> Added more test scenarios in test case as per Eric's suggestion

[Eric Seidel (no email)]

Comment on attachment 95743 [details]
Patch:Incorporating Eric's comment

You've used a dumpAsText test (which is good!), but I don't think you can actually evaluate on the text output.  As-written, you have to actually look at the pixels of your test to see if it worked.

I think instead you want to be checking what ctx.font returns after you set it to "inherit" to make sure that the set was ignored.  Then you don't even need to draw the strings.

If you look at fast/js/resources/js-test-pre.js (and the many many tests which include this script) you can find a shouldBe function which can be helpful for writing dumpAsText tests.  you can use shouldBe("ctx.font", "40px Times New Roman") to make sure that the font doesn't change when you try to assign it "inherit" etc.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #17)
> (From update of attachment 95743 [details])
> You've used a dumpAsText test (which is good!), but I don't think you can actually evaluate on the text output.  As-written, you have to actually look at the pixels of your test to see if it worked.
> 
> I think instead you want to be checking what ctx.font returns after you set it to "inherit" to make sure that the set was ignored.  Then you don't even need to draw the strings.
> 
> If you look at fast/js/resources/js-test-pre.js (and the many many tests which include this script) you can find a shouldBe function which can be helpful for writing dumpAsText tests.  you can use shouldBe("ctx.font", "40px Times New Roman") to make sure that the font doesn't change when you try to assign it "inherit" etc.

Sure, will take care of it my next patch.

Also as per discussion on IRC, will try either of the below
=>either split a string on whitespace OR
=>better yet have a loop which skips whitspace, and checks for one of these in a loop.

So this  little parser in Canvas code would help to identify "inherit" in the font string.

[Mustafizur Rahaman (rahaman)]

Created attachment 96217 [details]
Incorporating Eric & Kling's comment

After discussion with Eric & Kling, it was suggested to write a small parser to skip whitespace & then compare the string with "inherit"/"initial".

Also Eric gave the comment to modify the test case.

All the proposed changes are being incorporated in this patch.

[Eric Seidel (no email)]

Comment on attachment 96217 [details]
Incorporating Eric & Kling's comment

I don't understand why we need to remove/append to substrings for your algorithm to work.

Isn't it just:

while (start < end) {
  if (isSpaceOrNewLine(*start))
     continue;
   bool equalIgnoringCase(start, "inherit") || equalIgnoringCase(start, "initial") {
      start += 6;
      if (start == end || isSpaceOrNewline(start))
          return true;
   }
}

That's not quite real code because I'm not updating start quite correctly.  But that's the idea.

If we don't have an elegant way to write this, I'm not sure it should be written.  This is a very low-priority fix.
}

[Mustafizur Rahaman (rahaman)]

(In reply to comment #20)
> (From update of attachment 96217 [details])
> I don't understand why we need to remove/append to substrings for your algorithm to work.

i needed to use append/remove to get the exact substring(I was getting my substring separated by space) to be compared with "inherit"/"initial".
> 
> Isn't it just:
> 
> while (start < end) {
>   if (isSpaceOrNewLine(*start))
>      continue;
>    bool equalIgnoringCase(start, "inherit") || equalIgnoringCase(start, "initial") {
>       start += 6;
>       if (start == end || isSpaceOrNewline(start))
>           return true;
>    }
> }
> 
Though I am not completely sure which equalIgnoringCase() you have suggested me to use, but from my understanding I came up with the below algo.

What is your thoughts on this?

    while (fontStringStart <= fontStringEnd) {
        if (isSpaceOrNewline((*stringImpl)[fontStringStart])) {
            fontStringStart++;
            continue;
        }
        if (equalIgnoringCase("inherit",stringImpl->characters() + fontStringStart,7) 
            || equalIgnoringCase("initial",stringImpl->characters() + fontStringStart, 7)) {
            fontStringStart = fontStringStart + 7;
            if (fontStringStart > fontStringEnd || isSpaceOrNewline((*stringImpl)[fontStringStart]))
                return true;
        }
        fontStringStart++;
    }
    return false;

[Dominik Röttsches (drott)]

Fixed in r125118, using fontValue->isInheritedValue() - so no need to parse "inherit" value locally.

*** This bug has been marked as a duplicate of bug 93491 ***