Last Comment Bug 629882 - Support currentColor in nsCSSParser::ParseColorString
: Support currentColor in nsCSSParser::ParseColorString
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: Other Other
: -- normal (vote)
: mozilla16
Assigned To: :Ms2ger
:
Mentors:
http://www.whatwg.org/html/#2dcontext
Depends on:
Blocks: 622842
  Show dependency treegraph
 
Reported: 2011-01-29 05:45 PST by :Ms2ger
Modified: 2012-09-04 01:05 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds special color resolver "callback" object to the ParseColorString() and it implementation for canvas.2d (20.19 KB, patch)
2011-02-21 13:28 PST, Yury Delendik (:yury)
dbaron: review-
Details | Diff | Review
Patch v2 (18.60 KB, patch)
2011-10-01 08:06 PDT, :Ms2ger
dbaron: feedback+
Details | Diff | Review
Patch v3 (35.95 KB, patch)
2011-10-02 09:19 PDT, :Ms2ger
dbaron: review-
Details | Diff | Review
Patch v4 (35.96 KB, patch)
2011-10-08 07:43 PDT, :Ms2ger
no flags Details | Diff | Review
Patch v4.1 (35.62 KB, patch)
2011-10-09 04:48 PDT, :Ms2ger
no flags Details | Diff | Review
Patch v5 (33.44 KB, patch)
2011-10-30 15:31 PDT, :Ms2ger
dbaron: review-
Details | Diff | Review
Patch v6 (34.77 KB, patch)
2012-04-14 13:04 PDT, :Ms2ger
dbaron: review+
Details | Diff | Review

Description :Ms2ger 2011-01-29 05:45:43 PST
CSSParserImpl::ParseColorString has

// XXX - this is NS_COLOR_CURRENTCOLOR, NS_COLOR_MOZ_HYPERLINKTEXT, etc.
// which we don't handle as per the ParseColorString definition.  Should
// remove this limitation at some point.

However, HTML requires us to support currentColor, and this causes us to fail the following tests:

http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.fillStyle.parse.current.basic.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.fillStyle.parse.current.changedhtml
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.fillStyle.parse.current.removed.html

Should these be fixed by removing the limitation here or should this be special-cased in Canvas code?
Comment 1 Boris Zbarsky [:bz] 2011-01-29 22:38:17 PST
Canvas is the only real consumer of this function.  So we should, imo, fix it here.  It would need to take something from which it can derive the current color as an argument.
Comment 2 Yury Delendik (:yury) 2011-02-21 13:28:52 PST
Created attachment 514070 [details] [diff] [review]
Adds special color resolver "callback" object to the ParseColorString() and it implementation for canvas.2d

The patch adds additional parameter to the ParseColorString function -- aResolver. If aResolver is not nsnull, the function is trying to resolve the
special enum colors NS_COLOR_CURRENTCOLOR, NS_COLOR_MOZ_HYPERLINKTEXT, etc. 

The introduced SpecialColorResolver object contains the ResolveRuntimeEnumValue that performs conversion of the int values to runtime colors. (I could not think of the better names for the object and the method, please feel free to suggest another names) The goal was to use the callback mechanism to not kill the performance of the SetFillStyle() .
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-02-25 08:13:16 PST
Comment on attachment 514070 [details] [diff] [review]
Adds special color resolver "callback" object to the ParseColorString() and it implementation for canvas.2d

This seems more like a css change and I'm not the best to review that. It might also be good to split out the CSS changes and then have a Canvas changes as a patch on top of that.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-28 15:48:54 PDT
Comment on attachment 514070 [details] [diff] [review]
Adds special color resolver "callback" object to the ParseColorString() and it implementation for canvas.2d

Sorry for the delay in getting to this.

I think a callback function is too general an approach here -- it leads to duplicating a bunch of code in order to solve a very general problem that we don't actually have.

I think the right thing to do here is to share our existing code *better*.  In particular:
 * make CSSParserImpl::ParseColorString fill in an nsCSSValue rather than an nscolor, and just return a boolean
 * expose the SetColor method on nsRuleNode through a static method on nsRuleNode, say,
  /**
   * @param aValue The color value, returned from nsCSSParser::ParseColorString
   * @param aPresContext Presentation context whose preferences are used
   *                     for certain enumerated colors
   * @param aStyleContext Style context whose color is used for 'currentColor'
   */
  static nscolor nsRuleNode::ComputeColor(const nsCSSValue& aValue,
                                          nsPresContext* aPresContext,
                                          nsStyleContext* aStyleContext);
   which:
     * asserts that the value doesn't have eCSSUnit_Inherit
     * calls SetColor with a dummy aParentColor (NS_RGB(0, 0, 0)) and a dummy
       aCanStoreInRuleTree parameter
     * asserts that SetColor returned true
     * returns the resulting color

Does that seem like it would work?
Comment 5 Yury Delendik (:yury) 2011-04-09 18:52:24 PDT
The callback function was introduced to reduce the impact on the performance of canvas.2d context when nsPresContext is required. As I understand the nsPresContext or nsRuleNode are not really applicable for the canvas.2d operations. Not sure how to proceed with the recommended approach.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-10 01:27:13 PDT
I don't understand what comment 5 means.
Comment 7 :Ms2ger 2011-10-01 08:06:41 PDT
Created attachment 563966 [details] [diff] [review]
Patch v2

David, does this look like what you had in mind?
Comment 8 :Ms2ger 2011-10-01 08:09:37 PDT
(The patch isn't yet ready for review; the same changes will need to be made to the Azure fork, and the resulting dead code will need to be removed.)
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-01 10:16:38 PDT
Comment on attachment 563966 [details] [diff] [review]
Patch v2

Roughly, yes.  A few notes, though, but I didn't review in detail:

Not sure why you change SetColor to allow a null style context.  And the assertion at the top of nsRuleNode::ComputeColor needs something else right of the !=.

And your canvas rendering context changes have an overlong line or two, and I wonder whether the canvas gradient stuff should also use the GetColor function.  And also you seem inconsistent about whether you need to null-check HTMLCanvasElement().
Comment 10 :Ms2ger 2011-10-02 09:16:57 PDT
(In reply to David Baron [:dbaron] from comment #9)
> Comment on attachment 563966 [details] [diff] [review] [diff] [details] [review]
> Patch v2
> 
> Roughly, yes.  A few notes, though, but I didn't review in detail:
> 
> Not sure why you change SetColor to allow a null style context.

That seemed like the easiest way to support the requirement that canvas elements that aren't in the DOM and gradients should use #000 for currentColor; I could also special-case it in nsRuleNode::ComputeColor if you prefer.
Comment 11 :Ms2ger 2011-10-02 09:19:22 PDT
Created attachment 564056 [details] [diff] [review]
Patch v3
Comment 12 :Ms2ger 2011-10-02 09:21:23 PDT
And I missed this:

(In reply to David Baron [:dbaron] from comment #9)
> I wonder whether the canvas gradient stuff should also use the GetColor
> function.

Given that it doesn't need a style context, and there's not that much duplicated code, I haven't changed that.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-02 14:22:18 PDT
Comment on attachment 564056 [details] [diff] [review]
Patch v3

Review of attachment 564056 [details] [diff] [review]:
-----------------------------------------------------------------

Was there a reason to switch reviewer here?
Comment 14 :Ms2ger 2011-10-03 05:31:27 PDT
The fact that I really just transcribed David's implementation in comment 4 into C++; either way is fine for me, though.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-04 08:09:11 PDT
Comment on attachment 564056 [details] [diff] [review]
Patch v3

The CSS parser's ParseColorString should take nsCSSValue& instead of
nsCSSValue*, to match the way nsCSSValue objects are usually passed
around.

Please add a test for the case that would crash if you didn't have the
null-check in SetColor. (i.e., a mochitest for drawWindow with
currentColor as the backstop color)

In the null-check in SetColor, instead of using black, I think it would
actually be better to fall through to the next case and use the default
color, since that's what color:currentcolor on the root element does.

nsCanvasRenderingContext2D::GetColor should probably take nscolor*
instead of nscolor& so that it's clearer at callsites that it assigns to
its color parameter.  (same for azure)

>-/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

I think that was intentional, so that TABs would stick out.  Leave it.

nsCanvasRenderingContext2DAzure::GetColor has its second line badly
indented.


I'm a little disturbed that this now allows currentColor in canvas
gradients to work, but doesn't make it work correctly.  What does the
spec say about that?

I'm ready to grant r=dbaron with those changes, except for the last
question.
Comment 16 :Ms2ger 2011-10-08 07:42:01 PDT
(In reply to David Baron [:dbaron] from comment #15)
> Comment on attachment 564056 [details] [diff] [review] [diff] [details] [review]
> Patch v3
> 
> The CSS parser's ParseColorString should take nsCSSValue& instead of
> nsCSSValue*, to match the way nsCSSValue objects are usually passed
> around.

OK.

> Please add a test for the case that would crash if you didn't have the
> null-check in SetColor. (i.e., a mochitest for drawWindow with
> currentColor as the backstop color)

I also pass null for the parent-less canvas element case, so this is tested by the last test case I change in test_canvas.html.

> In the null-check in SetColor, instead of using black, I think it would
> actually be better to fall through to the next case and use the default
> color, since that's what color:currentcolor on the root element does.

The spec explicitly requires fully opaque black, so I'd rather implement that.

> nsCanvasRenderingContext2D::GetColor should probably take nscolor*
> instead of nscolor& so that it's clearer at callsites that it assigns to
> its color parameter.  (same for azure)

OK.

> >-/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> 
> I think that was intentional, so that TABs would stick out.  Leave it.

Unfortunately, that makes gedit indent with 20 spaces; I can change it on checkin, though.

> nsCanvasRenderingContext2DAzure::GetColor has its second line badly
> indented.

Fixed.

> I'm a little disturbed that this now allows currentColor in canvas
> gradients to work, but doesn't make it work correctly.  What does the
> spec say about that?

http://www.whatwg.org/html/#dom-context-2d-canvas

> In the case of addColorStop() on CanvasGradient, the "computed value of
> the 'color' property" for the purposes of determining the computed value
> of the currentColor keyword is always fully opaque black (there is no
> associated element). [CSSCOLOR]

This is because CanvasGradients can be used with multiple canvas elements, whose value for currentColor may be different.

New patch up in a moment.
Comment 17 :Ms2ger 2011-10-08 07:43:49 PDT
Created attachment 565736 [details] [diff] [review]
Patch v4
Comment 18 :Ms2ger 2011-10-09 04:48:15 PDT
Created attachment 565795 [details] [diff] [review]
Patch v4.1

Tryserver didn't like how I forgot ReleaseScanner().
Comment 19 :Ms2ger 2011-10-09 07:02:24 PDT
Comment on attachment 565795 [details] [diff] [review]
Patch v4.1

Looks like this leaks as well.
Comment 20 Boris Zbarsky [:bz] 2011-10-11 14:29:57 PDT
Note that bug 682088 just added a ParseColorString caller.
Comment 21 :Ms2ger 2011-10-12 01:44:04 PDT
Ack.
Comment 22 :Ms2ger 2011-10-30 15:31:07 PDT
Created attachment 570575 [details] [diff] [review]
Patch v5
Comment 23 :Ms2ger 2011-10-30 15:37:21 PDT
Comment on attachment 570575 [details] [diff] [review]
Patch v5

This is taking a different approach. Rather than trying to find a sensible nsPresContext for gradients (which doesn't exist, as they are not tied to a particular document, and might not be possible, as we don't always have an element), I just pass null and deal with that in SetColor. (This patch returns fully opaque black for all our extensions in that case, but it doesn't matter to me what we fall back to.)
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-06 17:00:13 PST
Comment on attachment 570575 [details] [diff] [review]
Patch v5

Don't use default parameters for nsRuleNode::ComputeColor -- the
function generally produces better results when the parameters are
passed, so null isn't a sensible default since it produces results that
aren't as good.  Callers should have to think about whether they have an
object to pass; you should add a comment that it's ok to pass null when
there's no sensible pres context or style context available but doing so
causes some values to revert to silly defaults.


So I think a bunch of the callers here do want useful reporting, and
this is suppressing that by accepting values that don't actually do
sensible things.  We're better off reporting those errors -- it gives us
more options in the future.  So I think you should make you should make
SetColor return false in those cases (when null was passed) and make
nsRuleNode::ComputeColor propagate that error, which requires changing 
its signature -- you have a few options.  (You should also make it
assert that it doesn't have an error when both a pres context and a
style context were passed, and make it assert that eCSSUnit_Initial
isn't given in the value.)


Sorry for the delay getting to this -- I think unfortunately I do want to look at another patch here, but I think one more should be sufficient.
Comment 25 :Ms2ger 2012-04-14 13:04:34 PDT
Created attachment 615077 [details] [diff] [review]
Patch v6

Sorry for the delay. Is this something like what you had in mind?
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-14 14:49:47 PDT
Comment on attachment 615077 [details] [diff] [review]
Patch v6

In DocumentRendererChild::RenderDocument, use the variable name
|bgColorValue| rather than just |value|.

In both canvas contexts, I tend to think GetColor should be protected,
probably declared next to StyleColorToString, and probably be called
ParseColor.


>-  rv = presShell->RenderDocument(r, renderDocFlags, bgColor, thebes);
>+  unused << presContext->PresShell()->
>+    RenderDocument(r, renderDocFlags, bgColor, thebes);

Leave this alone; it's not part of the scope of this patch.


Mention nsRuleNode::ComputeColor in the comment for
nsCSSParser::ParseColorString in nsCSSParser.h


In SetColor in nsRuleNode.cpp, instead of having
  result = !!aPresContext
or
  result = !!aContext
and also another null-check of the same in each case,
just put:
  result = false;
  aResult = NS_RGB(0, 0, 0)
before the switch, and make each entry in the switch do things like:
  if (aPresContext) {
    result = true;
    aResult = aPresContext->DefaultLinkColor();
  }


Don't use MOZ_ASSERT_IF; it's confusing since its parameters are in the
wrong order.  (A function called assert if, with semantics "assert X if
Y", shouldn't take its parameters in the order Y, X.  Yes, it's a
convention from SmallTalk, but the code is confusing.)  Instead, write
out the implication and assert
  ok || !(aPresContext && aStyleContext)


r=dbaron with those things fixed
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-31 19:59:34 PDT
Ms2ger, are you planning to land this?
Comment 28 :Ms2ger 2012-06-01 01:31:17 PDT
I think try wasn't happy with my patch... Pushing again now to try and figure out what went wrong.
Comment 29 :Ms2ger 2012-06-06 03:25:52 PDT
Turned out I missed a null check for presShell.

https://hg.mozilla.org/mozilla-central/rev/d77cc86cb723
Comment 30 Jean-Yves Perrier [:teoli] 2012-08-15 08:58:41 PDT
This looks mainly as a bug fix. I'm wondering if this need more documentation that a mere mention in Firefox 16 for developers. Any advice?
Comment 31 :Ms2ger 2012-08-15 09:01:59 PDT
Yeah, I guess that's fine
Comment 32 Jean-Yves Perrier [:teoli] 2012-09-04 01:05:12 PDT
Added a line to: https://developer.mozilla.org/en-US/docs/Firefox_16_for_developers

Note You need to log in before you can comment on or make changes to this bug.