Bug 40273 – Canvas: "currentColor" should inherit the canvas element's color

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Jan Erik Hanssen]

The HTML5 canvas 2d.fillStyle.parse.current.* tests do not pass, currentColor is not parsed to the "computed value of the 'color' property" of the canvas element style as specified in the spec (http://philip.html5.org/tests/canvas/suite/tests/spec.html#testrefs.2d.currentColor.outofdoc).

Tests:
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.current.basic.html
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.current.changed.html
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.current.removed.html

[Andreas Kling]

*** Bug 41246 has been marked as a duplicate of this bug. ***

[Andreas Kling]

Created attachment 59826 [details]
Proposed patch

Proposed patch. Personally I'm not a big fan of the string literal, so if anyone has a better idea, do tell.

[WebKit Review Bot]

Attachment 59826 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/canvas/CanvasRenderingContext2D.cpp:82:  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 5 files


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

[Andreas Kling]

Created attachment 59827 [details]
Proposed patch

Same patch, style fixed.

[Darin Adler]

Comment on attachment 59827 [details]
Proposed patch

The bug title should be "add support for special color named currentColor". This patch is about adding a missing feature, more than about fixing a particular test case in a test suite!

> +    RGBA32 rgba = Color::black;
> +    if (canvas->inDocument())
> +        CSSParser::parseColor(rgba, canvas->style()->getPropertyValue(CSSPropertyColor));
> +    return rgba;

WebKit normally uses early return style. So it would be like this:

    if (!canvas->inDocument())
        return Color::black;
    RGBA32 color = Color::black;
    CSSParser::parseColor(color, canvas->style()->getPropertyValue(CSSPropertyColor));
    return rgba;

I don't think this function needs "from canvas" in its name. I think currentColor would be fine.

> +    if (color.lower() == "currentcolor")

The most efficient way to do this check is:

    equalIgnoringCase(color, "currentcolor")

Among other things, calling lower() means we'll allocate a new string if there are any uppercase characters.

I'm not that fond of a function named "useCurrentColor". I think you would want to name it something more like parseColorOrCurrentColor. Another possibility would be to name it parseColor and make it a private member of CanvasRenderingContext2D. I think I like that design best.

> +    if (!useCurrentColor(canvas(), state().m_shadowColor, color)) {
> +        if (!CSSParser::parseColor(state().m_shadowColor, color))
> +            return;
> +    }

This code calls parseColor twice. Once inside useCurrentColor and once here at the call site. I suggest the parseColorOrCurrentColor design and removing the extra call to parseColor.

> +    if (color.lower() == "currentcolor")
> +        return adoptRef(new CanvasStyle());

We should not have the string "currentcolor" in two different places in the code. We should find a way to share this. A single that would have a parseColor function that can return either failure, flag indicating current color, or an RGBA value. Then CanvasRenderingContext2D and CanvasStyle can both use it. I think it would be good to put it in the CanvasStyle header. One design would be:

    enum ColorParseResult { ParsedRGBA, ParsedCurrentColor, ParseFailed };
    ColorParseResult parseColor(const String& colorString, RGBA& parsedColor);

This could be a free function in CanvasStyle.h, or a static function member of CanvasStyle.

review- because at the very least we want to use equalIgnoringCase for this.

Do you think the test suite covers all the code paths here? Do we need to add any new tests?

[Andreas Kling]

Created attachment 59848 [details]
Proposed patch v2

The first revision wasn't covering all API's. This one does, including CanvasGradient::addColorStop() and WebKit's non-standard CRC2D methods.

I've also included a layout test which covers the various code paths.

[Andreas Kling]

Created attachment 59849 [details]
Proposed patch v2

Click the right file this time..

[WebKit Review Bot]

Attachment 59849 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/canvas/CanvasGradient.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


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

[Darin Adler]

Comment on attachment 59849 [details]
Proposed patch v2

This looks really good.

> -        static PassRefPtr<CanvasGradient> create(const FloatPoint& p0, const FloatPoint& p1)
> +        static PassRefPtr<CanvasGradient> create(HTMLCanvasElement* canvas, const FloatPoint& p0, const FloatPoint& p1)

I am pretty sure this is wrong. There is no guarantee that a gradient will be used only with the canvas it was created from. Instead, the canvas or its current color should be passed to functions in CanvasStyle, and in turn passed in to the functions on the CanvasGradient.

We should create at least one test case that creates a gradient on one canvas and then uses it with another, which has a different current color, to test that the behavior is correct.

> +    RGBA32 currentColor(HTMLCanvasElement* canvas);
> +    bool parseColorOrCurrentColor(RGBA32& parsedColor, const String& colorString, HTMLCanvasElement* canvas);

The argument name "canvas" should be omitted from both of these function declarations.

[Andreas Kling]

(In reply to comment #9)
> (From update of attachment 59849 [details])
> > -        static PassRefPtr<CanvasGradient> create(const FloatPoint& p0, const FloatPoint& p1)
> > +        static PassRefPtr<CanvasGradient> create(HTMLCanvasElement* canvas, const FloatPoint& p0, const FloatPoint& p1)
> 
> I am pretty sure this is wrong. There is no guarantee that a gradient will be used only with the canvas it was created from. Instead, the canvas or its current color should be passed to functions in CanvasStyle, and in turn passed in to the functions on the CanvasGradient.

I spoke with Philip Taylor about this and ended up filing a bug on w3.org:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=10023

[Andreas Kling]

Created attachment 66213 [details]
Proposed patch v3

HTML5 has now been updated to say that "currentColor" means fully opaque black in the context of addColorStop().
Diff: http://html5.org/tools/web-apps-tracker?from=5388&to=5389

I've updated the patch to reflect this behavior.

[Eric Seidel (no email)]

Attachment 66213 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3937021

[Andreas Kling]

Created attachment 66216 [details]
Proposed patch v4

Fixed GCC warnings (missing cases.)

[Andreas Kling]

Created attachment 70040 [details]
Proposed patch v5

Rebased patch. Remove relevant tests from Gtk+ skiplist as well.

[Darin Adler]

Comment on attachment 70040 [details]
Proposed patch v5

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

> WebCore/html/canvas/CanvasStyle.cpp:81
> +    if (parseResult == ParseFailed)
> +        return false;
> +    if (parseResult == ParsedCurrentColor)
> +        parsedColor = currentColor(canvas);
> +    return true;

This should use a switch statement, because then the compiler will check to be sure we cover all values of ColorParseResult.

> WebCore/html/canvas/CanvasStyle.cpp:135
> +    if (parseResult == ParseFailed)
>          return 0;
> +    if (parseResult == ParsedCurrentColor)
> +        return adoptRef(new CanvasStyle(CurrentColor));
>      return adoptRef(new CanvasStyle(rgba));

This should use a switch statement, because then the compiler will check to be sure we cover all values of ColorParseResult.

> WebCore/html/canvas/CanvasStyle.h:53
> +        float overrideAlpha() const { return m_overrideAlpha; }

This should assert that m_type is CurrentColorWithOverrideAlpha.

> WebCore/html/canvas/CanvasStyle.h:55
>          String color() const { return Color(m_rgba).serialized(); }

This should assert that m_type is RGBA.

[Andreas Kling]

(In reply to comment #15)
> > WebCore/html/canvas/CanvasStyle.h:55
> >          String color() const { return Color(m_rgba).serialized(); }
> 
> This should assert that m_type is RGBA.

That would assert when retrieving a stroke or fill color that was set from CMYKA components. I will land with ASSERT(m_type == RGBA || m_type == CMYKA);

Thanks for the review!

[Andreas Kling]

Committed r69755: <http://trac.webkit.org/changeset/69755>

[Csaba Osztrogonác]

Created attachment 70728 [details]
proposed warning fix for r69755 

http://trac.webkit.org/changeset/69755 caused warnings
because of missing default cases:

../../../WebCore/html/canvas/CanvasStyle.cpp:157: warning: control reaches end of non-void function
../../../WebCore/html/canvas/CanvasStyle.cpp:143: warning: control reaches end of non-void function
../../../WebCore/html/canvas/CanvasStyle.cpp:86: warning: control reaches end of non-void function

[Csaba Osztrogonác]

Comment on attachment 70728 [details]
proposed warning fix for r69755 

Clearing flags on attachment: 70728

Committed r69759: <http://trac.webkit.org/changeset/69759>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69755 might have broken Chromium Mac Release