Closed Bug 629882 Opened 13 years ago Closed 12 years ago

Support currentColor in nsCSSParser::ParseColorString

Categories

(Core :: CSS Parsing and Computation, defect)

Other
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 5 obsolete files)

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?
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.
Assignee: nobody → async.processingjs
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() .
Attachment #514070 - Flags: review?(jmuizelaar)
Attachment #514070 - Flags: feedback?
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.
Attachment #514070 - Flags: review?(jmuizelaar) → review?(dbaron)
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?
Attachment #514070 - Flags: review?(dbaron) → review-
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.
Assignee: async.processingjs → nobody
Attachment #514070 - Flags: feedback?
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
David, does this look like what you had in mind?
Attachment #563966 - Flags: feedback?(dbaron)
(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 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().
Attachment #563966 - Flags: feedback?(dbaron) → feedback+
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #563966 - Attachment is obsolete: true
Attachment #564056 - Flags: review?(roc)
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 on attachment 564056 [details] [diff] [review]
Patch v3

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

Was there a reason to switch reviewer here?
Attachment #564056 - Flags: review?(roc) → review?(dbaron)
The fact that I really just transcribed David's implementation in comment 4 into C++; either way is fine for me, though.
Status: ASSIGNED → NEW
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.
Attachment #564056 - Flags: review?(dbaron) → review-
(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.
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #564056 - Attachment is obsolete: true
Attachment #565736 - Flags: review?(dbaron)
Attached patch Patch v4.1 (obsolete) — Splinter Review
Tryserver didn't like how I forgot ReleaseScanner().
Attachment #565736 - Attachment is obsolete: true
Attachment #565795 - Flags: review?(dbaron)
Attachment #565736 - Flags: review?(dbaron)
Comment on attachment 565795 [details] [diff] [review]
Patch v4.1

Looks like this leaks as well.
Attachment #565795 - Flags: review?(dbaron)
Note that bug 682088 just added a ParseColorString caller.
Ack.
Attached patch Patch v5 (obsolete) — Splinter Review
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.)
Attachment #570575 - Attachment description: Support currentColor in the 2D canvas context; changes nsCSSParser::ParseColorString to fill in an nsCSSValue instead → Patch v5
Attachment #570575 - Flags: review?(dbaron)
Attachment #565795 - Attachment is obsolete: true
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.
Attachment #570575 - Flags: review?(dbaron) → review-
Attached patch Patch v6Splinter Review
Sorry for the delay. Is this something like what you had in mind?
Attachment #570575 - Attachment is obsolete: true
Attachment #615077 - Flags: review?(dbaron)
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
Attachment #615077 - Flags: review?(dbaron) → review+
Ms2ger, are you planning to land this?
I think try wasn't happy with my patch... Pushing again now to try and figure out what went wrong.
Turned out I missed a null check for presShell.

https://hg.mozilla.org/mozilla-central/rev/d77cc86cb723
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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?
Yeah, I guess that's fine
You need to log in before you can comment on or make changes to this bug.