Closed Bug 1063162 Opened 10 years ago Closed 7 years ago

[CSS3-UI] Support caret-color property on all elements

Categories

(Core :: Layout: Form Controls, defect, P3)

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Eli, Assigned: xidorn)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

Bug 1063162 part 2 - Implement caret-color property.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1063162 part 1 - Add auto value support to StyleComplexColor.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Example setting caret-color on a visited link
494 bytes, text/html
Details 
In bug 1063037 it was requested by UX to change the color of an input caret, but I could not find any way this was currently possible through CSS. There should exist a mechanism to control the color of an input caret. In a csss4-ui thread [1] this same issue was raised, but I didn't see any further progress on it. Also looking through all the current Mozilla CSS extensions I did not find anything that would let me control caret style.

I can't say whether the form of this should take a `caret`-type property or introducing a `::-moz-caret` element, I just know that the biggest request for control is for color.

[1] http://lists.w3.org/Archives/Public/www-style/2011Nov/0772.html
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Mozilla_Extensions
This is pretty easy. Probably you should contact Jet Villegas with your request so he can prioritize it relative to the other work the layout team is doing.
We should get slightly tighter consensus on www-style before we implement this. I don't expect that to be difficult or take long.
ni? for comment 1.
Flags: needinfo?(bugs)
From the www-style thread you posted, I gather this proposed property:

Name: caret
Value: <color> | invert
Initial: initial
Applies to: Any element that accepts textual input
Inherited: Yes
Percentages: N/A
Media: Interactive
Computed value: The computed value for 'invert' is 'invert'. For <color> values, the computed value is as defined for the [CSS3COLOR] 'color' property.

Will that cover the use cases you need?
Flags: needinfo?(bugs)
Yes, I believe so. At the moment my only necessitated use case would be (using that proposal):

input[type=text] {
  caret: #00aacc;
}

Thanks!
Wouldn't a caret-color make more sense? caret could be reserved for a possible future shorthand. Different caret styles like underline come to mind as future additions.
Which users is such a feature in a user-agent supposed to benefit? How can such a non-user power benefit users? How easily will users be able to prevent its use? Is this yet another stylist power that user attempts to thwart will make otherwise familiar page elements even harder to distinguish among? Would this really be limited to affecting only Mac users?
(In reply to silverwind from comment #6)
> Wouldn't a caret-color make more sense? caret could be reserved for a
> possible future shorthand. Different caret styles like underline come to
> mind as future additions.

Yes, that does make sense. Please propose that in the public standards thread here:
http://lists.w3.org/Archives/Public/www-style/2014Aug/0237.html
(In reply to Felix Miata from comment #7)
> Which users is such a feature in a user-agent supposed to benefit? How can
> such a non-user power benefit users? How easily will users be able to
> prevent its use? Is this yet another stylist power that user attempts to
> thwart will make otherwise familiar page elements even harder to distinguish
> among? Would this really be limited to affecting only Mac users?

The benefit here is to specify and indicate the author's purpose for editable text fields. User stylesheets can override this feature, just as they do for other style attributes that may affect accessibility. I'm not sure what you mean about only affecting Mac users.
There is no consensus on 'invert' so that should be omitted.

Captured here as an issue in progress for CSS3-UI: https://wiki.csswg.org/spec/css3-ui#issue-31

Will land in the editor's draft shortly.
I have a patch+tests for this bug, implemented for Bloomberg, working absolutely fine on mozilla-central and matching the discussions in www-style. Let me ask them to contribute it here.
Comment on attachment 8539247 [details] [diff] [review]
Proposed patch, tests pending

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

Needs style system peer review.

This needs a reftest. Should be easy to add something to test_reftests_with_caret.html.

::: layout/style/nsStyleStruct.cpp
@@ +3522,5 @@
>      NS_UpdateHint(hint, nsChangeHint_SchedulePaint);
>    }
>  
> +  if (mAutoCaretColor != aOther.mAutoCaretColor || mCaretColor != aOther.mCaretColor) {
> +    NS_UpdateHint(hint, nsChangeHint_NeutralChange);

I think you need nsChangeHint_RepaintFrame
Attachment #8539247 - Flags: review?(roc)
Attachment #8539247 - Flags: review?(cam)
Attachment #8539247 - Flags: review+
Attached patch in answer to roc's comment (obsolete) — — Splinter Review 
Tweak of the changehint in answer to roc's comment. Reftest pending (sorry it takes more time than expected, had 36 hours of power outage at the office after a power transformer burned in the neighbourhood...)
Attachment #8539247 - Attachment is obsolete: true
Attachment #8539247 - Flags: review?(cam)
Attachment #8539762 - Flags: review?(roc)
Comment on attachment 8539762 [details] [diff] [review]
in answer to roc's comment

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

::: layout/style/nsStyleStruct.cpp
@@ +2804,5 @@
>      if (transformHint) {
>        if (HasTransformStyle()) {
>          NS_UpdateHint(hint, transformHint);
>        } else {
> +        NS_UpdateHint(hint, nsChangeHint_RepaintFrame);

I don't understand why you're changing this line.

@@ +3523,5 @@
>    }
>  
> +  if (mAutoCaretColor != aOther.mAutoCaretColor || mCaretColor != aOther.mCaretColor) {
> +    NS_UpdateHint(hint, nsChangeHint_NeutralChange);
> +  }

This is the place where you need RepaintFrame.
Attachment #8539762 - Flags: review?(roc) → review-
Assignee: nobody → daniel
I'm not sure it makes sense for auto to compute to currentColor: won't that mean you inherit the caret color from ancestors when using auto, rather than using the color on the descendant element?

  <div style="caret-color: auto; color: red;">
    <span style="color: green;">hello</span>
  </div>

Surely you'd want the caret to be green in the middle of the span.
(In reply to Cameron McCormack (:heycam) from comment #17)
> Surely you'd want the caret to be green in the middle of the span.

Which your patch already allows.  Can you raise this issue on www-style Daniel?
Comment on attachment 8540005 [details] [diff] [review]
wow, dunno how I messed that up. Thanks for catching that Roc and sorry for the annoyance...

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

::: layout/generic/nsFrame.cpp
@@ +1579,5 @@
>  nsIFrame::GetCaretColorAt(int32_t aOffset)
>  {
>    // Use text color.
> +  if (StyleUserInterface()->mAutoCaretColor) {
> +    return StyleColor()->mColor;

Won't we already have the correct computed value for the caret color in mCaretColor, because we computed it in nsRuleNode::ComputeUserInterfaceData, in which case we don't need this if statement?

::: layout/style/nsCSSPropList.h
@@ +1393,5 @@
>      kCaptionSideKTable,
>      CSS_PROP_NO_OFFSET,
>      eStyleAnimType_None)
> +CSS_PROP_USERINTERFACE(
> +    -moz-caret-color,

We shouldn't be adding a -moz-prefixed property, especially given that the property is being standardised.  Please call it caret-color and put it behind a pref.  (Ideally we would be sending out an "intent to implement" mail to dev-platform, and then later an "intent to ship" when we feel it's ready to unpref.)

@@ +1400,5 @@
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
> +        CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED |
> +        CSS_PROPERTY_HASHLESS_COLOR_QUIRK,

There shouldn't be any compatibility requirement that needs CSS_PROPERTY_HASHLESS_COLOR_QUIRK.

::: layout/style/nsComputedDOMStyle.cpp
@@ +3342,5 @@
>  
>  CSSValue*
> +nsComputedDOMStyle::DoGetCaretColor()
> +{
> +  nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;

"*" next to type in new code.

::: layout/style/nsRuleNode.cpp
@@ +4663,5 @@
> +  if ((colorValue->GetUnit() == eCSSUnit_EnumColor &&
> +       colorValue->GetIntValue() == NS_COLOR_CURRENTCOLOR) ||
> +      colorValue->GetUnit() == eCSSUnit_Auto ||
> +      colorValue->GetUnit() == eCSSUnit_Initial ||
> +      colorValue->GetUnit() == eCSSUnit_Unset ||

Since this is an inherited property, unset should behave the same as inherit rather than initial.

@@ +4664,5 @@
> +       colorValue->GetIntValue() == NS_COLOR_CURRENTCOLOR) ||
> +      colorValue->GetUnit() == eCSSUnit_Auto ||
> +      colorValue->GetUnit() == eCSSUnit_Initial ||
> +      colorValue->GetUnit() == eCSSUnit_Unset ||
> +      colorValue->GetUnit() == eCSSUnit_Null) {

We shouldn't be setting the value if we have eCSSUnit_Null.

@@ +4665,5 @@
> +      colorValue->GetUnit() == eCSSUnit_Auto ||
> +      colorValue->GetUnit() == eCSSUnit_Initial ||
> +      colorValue->GetUnit() == eCSSUnit_Unset ||
> +      colorValue->GetUnit() == eCSSUnit_Null) {
> +    ui->mAutoCaretColor = true;

Note that the SetColor call below will set canStoreInRuleTree to false if it encounters currentColor, so you'll need to do that in this branch too for auto/currentColor.

@@ +4668,5 @@
> +      colorValue->GetUnit() == eCSSUnit_Null) {
> +    ui->mAutoCaretColor = true;
> +    ui->mCaretColor = aContext->StyleColor()->mColor;
> +  }
> +  else {

"else {" on previous line.

@@ +4669,5 @@
> +    ui->mAutoCaretColor = true;
> +    ui->mCaretColor = aContext->StyleColor()->mColor;
> +  }
> +  else {
> +    ui->mAutoCaretColor = (colorValue->GetUnit() == eCSSUnit_Inherit &&

Check for unset here too.

::: layout/style/nsStyleStruct.cpp
@@ +3465,5 @@
>  
>    mCursor = NS_STYLE_CURSOR_AUTO; // fix for bugzilla bug 51113
>  
> +  mAutoCaretColor = true;
> +  mCaretColor = NS_RGB(0,0,0);

mCaretColor needs to be set to the prescontext's default colour, just like nsStyleColor::mColor is, otherwise you'll get a black caret in a document with no color or caret-colors set even if user text colour preferences use a different colour.

@@ +3521,5 @@
>    if (mWindowDragging != aOther.mWindowDragging) {
>      NS_UpdateHint(hint, nsChangeHint_SchedulePaint);
>    }
>  
> +  if (mAutoCaretColor != aOther.mAutoCaretColor || mCaretColor != aOther.mCaretColor) {

Please wrap to fit 80 columns.

I think we can actually keep the nsChangeHint_NeutralChange hint if mAutoCaretColor changes; it's really only mCaretColor differences that need nsChangeHint_RepaintFrame.
(In reply to Cameron McCormack (:heycam) from comment #19)
> ::: layout/generic/nsFrame.cpp
> @@ +1579,5 @@
> >  nsIFrame::GetCaretColorAt(int32_t aOffset)
> >  {
> >    // Use text color.
> > +  if (StyleUserInterface()->mAutoCaretColor) {
> > +    return StyleColor()->mColor;
> 
> Won't we already have the correct computed value for the caret color in
> mCaretColor, because we computed it in nsRuleNode::ComputeUserInterfaceData,
> in which case we don't need this if statement?

Oh, I guess not; we'll end up with the inherited mColor value from the start struct.
Comment on attachment 8540005 [details] [diff] [review]
wow, dunno how I messed that up. Thanks for catching that Roc and sorry for the annoyance...

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

::: layout/style/nsComputedDOMStyle.cpp
@@ +3343,5 @@
>  CSSValue*
> +nsComputedDOMStyle::DoGetCaretColor()
> +{
> +  nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
> +  SetToRGBAColor(val, StyleUserInterface()->mCaretColor);

If auto does not compute to currentColor, then you'll need to return auto rather than a colour in that case.
> wow, dunno how I messed that up. Thanks for catching that Roc and sorry for the annoyance...

The patch got named after that sentence.

If I understand this issue well, it allows the user to fix bug 579760 manually, right?

Sebastian
URL: http://dev.w3.org/csswg/css-ui/#propd...
OS: Mac OS X → All
Hardware: x86 → All
Spec co-editor here.

The spec for caret-color just got updated, and should now be pretty stable.

http://dev.w3.org/csswg/css-ui/#caret-color

Note that "auto" should not compute to "currentColor", but to "auto".

The behavior of "auto" should still be the same as currentColor, with the added possibility for the UA to add some extra logic to make sure there is sufficient contrast. You don't have to do that, but you may if you want to.
Daniel, your last patch is from one and a half years ago. Do you still plan to work on this? If not, let's unassign it, so others can have a look at finishing up the work.

Sebastian
Flags: needinfo?(daniel)
As I didn't get feedback from Daniel in the last three months, I think it's fine to remove him as assignee.
Hopefully somebody will step up soon to finish his patch!

Sebastian
Assignee: daniel → nobody
Flags: needinfo?(daniel)
Spec issue: https://github.com/w3c/csswg-drafts/issues/725
CSSWG Telecon minutes: https://lists.w3.org/Archives/Public/www-style/2016Nov/0123.html
Summary: Provide a mechanism for styling the color of an input caret → [CSS3-UI] Support caret-color property on all elements
The specification’s Editor’s Draft has been updated to reflect the above resolution:
https://drafts.csswg.org/css-ui-3/#caret-color
Though I think this is a good first bug for people new to... style system, the discussion seems to be too long for inexperienced people to follow, so I decide to just take it.

I've had a local patch which implements it, just need some tests...
Assignee: nobody → xidorn+moz
There's a fair pile of tests in the CSSWG repo already: caret-color-*.html, under https://github.com/w3c/csswg-test/tree/master/css-ui-3

As noted under https://github.com/w3c/csswg-drafts/issues/781#issuecomment-264625405, two of these tests (caret-color-009.html and caret-color-013.html) will need updating if we resolve on https://github.com/w3c/csswg-drafts/issues/566 as you suggest. But the rest should be good to go.

If Mozilla wants to contribute more, I'll be happy to review them.
I've seen the tests in CSSWG's repo, but those tests are not really usable for us since they are not reftests.

It is probably hard to write interoperable reftests for caret since caret is usually very small and blinking.

We have internal pref to make caret large and not blinking which I'm going to use, but that's not something can be put in CSSWG's test repo, so sadly I have to write new test and that test cannot be contributed to CSSWG :(
Can you point me to your thing to make caret large and not blinking, and if there's any write-up about that, to that as well? That sounds like interesting material for the caret-animation and caret-shape properties.
> so sadly I have to write new test and that test cannot be contributed to CSSWG :(

Some of the CSSWG tests are testharness.js tests, so hopefully you can use these.

For those that should but cannot be reftets, can you just make a copy of the CSSWG ones and add the ref to it using your large and non-blinking caret? You'll still end up with a separate set of tests, but at least that gives an extra round of reviews to those we already have.

Alternatively, if you write additional tests that we don't have, do you mind sending them my way without the ref so that we can all benefit?
(In reply to Florian Rivoal from comment #31)
> Can you point me to your thing to make caret large and not blinking, and if
> there's any write-up about that, to that as well? That sounds like
> interesting material for the caret-animation and caret-shape properties.

By default we read the blink interval from system (or follow the convention of the system), while we have a hidden preference called "ui.caretBlinkTime" which can be used to override the forementioned value. Actually, caret blinking is by default disabled in our reftests.

There is another hidden preference called "ui.caretWidth" which can be used to control the width of the caret. That value seems to be consistently "1" across different systems, though.

(In reply to Florian Rivoal from comment #32)
> > so sadly I have to write new test and that test cannot be contributed to CSSWG :(
> 
> Some of the CSSWG tests are testharness.js tests, so hopefully you can use
> these.

Unfortunately, currently we only use reftests from CSSWG :/

I think there is some discussion about merging CSSWG's test repo into web platform test, which may make the js tests usable. But that's off topic here.

> For those that should but cannot be reftets, can you just make a copy of the
> CSSWG ones and add the ref to it using your large and non-blinking caret?
> You'll still end up with a separate set of tests, but at least that gives an
> extra round of reviews to those we already have.
> 
> Alternatively, if you write additional tests that we don't have, do you mind
> sending them my way without the ref so that we can all benefit?

That's probably not worth. You can see the test (caret-color-01.html) from the patch I just submitted. It is a very simple rendering test which just tries to display a caret in a textarea with border / outline / resize removed.
> That's probably not worth. You can see the test (caret-color-01.html) from the patch I just submitted. It is a very simple rendering test which just tries to display a caret in a textarea with border / outline / resize removed.

Right. If you only get this kind of tests, we're already covered. Let me know if you write more advanced / corner cases tests.
You might want to pick another reviewer -- I probably won't be able to get to this review for a little while.  (I've got a pile of high priority grid code-reviews which will occupy much/all of my reduced review throughput this week (during the work week) -- and then I'm out for all of next week.)

If you're OK waiting a couple weeks, though, feel free to leave this in my review queue, & I'll get to it when I can. :)
I'm totally fine with waiting for a couple of weeks, feel free to review at your convenience.

This isn't an urgent thing. I picked it up just because it is easy and I need some easy thing to kill time when waiting for the plane :)
Comment on attachment 8816845 [details]
Bug 1063162 part 2 - Implement caret-color property.

https://reviewboard.mozilla.org/r/97368/#review100366

::: layout/style/nsCSSPropList.h:1394
(Diff revision 1)
> +        CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED,
> +    "",
> +    VARIANT_AUTO | VARIANT_HC,
> +    nullptr,
> +    offsetof(nsStyleUserInterface, mCaretColor),
> +    eStyleAnimType_ComplexColor)

Two notes about this eStyleAnimType:
 (1) If we end up encoding 'auto' in the computed style (as I'm suggesting in another review-comment below), we'll need to be sure our eStyleAnimType_ComplexColor code won't barf when encountering that special value.  (Our StyleAnimation code can probably treat 'auto' the same as currentcolor for now.)

 (2) You should add a test for transitioning this property to/from "auto", in the mochitest test_transitions_per_property.html.  (And also run the standard color test-functions that we use for other color-valued properties in that file.) I think that mochitest will actually fail, if you don't add any tests for this new property -- it's supposed to do that, at least.

::: layout/style/nsRuleNode.cpp:5219
(Diff revision 1)
>    COMPUTE_START_INHERITED(UserInterface, ui, parentUI)
>  
> +  // caret-color: auto, color, inherit
> +  const nsCSSValue* caretColorValue = aRuleData->ValueForCaretColor();

Nit: you should probably be inserting this new mCaretColor-setting code at the *end* of this function (rather than at the beginning), for consistency with the ordering of the member-variables in nsStyleStruct.h.

(This function is *nearly* consistent with the nsStyleStruct ordering right now, with the exception of mCursor/mCursorImages.  Let's keep it nearly-consistent, rather than making it less consistent. :))

::: layout/style/nsRuleNode.cpp:5224
(Diff revision 1)
> +    // Using currentcolor as default value is our behavior before we
> +    // implement caret-color. We may want to do something different in

The wording here is a bit confusing -- in particular:
 * "before we implement caret-color" sounds like we haven't implemented it yet, and 

How about rewording to:
    // When current-color is at its default value, we match our behavior from
    // before we supported this property -- i.e. we use 'currentcolor'. But we
    // may want to [...]

THOUGH, this clause also might need some reworking which might obsolete this comment anyway -- see my review-comment below this one, RE 'auto'.

::: layout/style/nsRuleNode.cpp:5227
(Diff revision 1)
> +    // do that, it is indistinguishable between keeping the 'auto' value
> +    // and computing it to currentcolor, so for simplicity, we just

'auto' is not quite indistinguishable from 'currentcolor' (as this comment suggests it is) -- because we need to still return "auto" from getComputedStyle.

The spec says:
   The computed value for 'auto' is 'auto'.
https://drafts.csswg.org/css-ui-3/#caret-color

And I think that means we need to preserve "auto" in our internal representation -- we can't just eagerly convert it to currentcolor (even if that's the behavior that it matches for now).

::: layout/style/nsStyleStruct.cpp:3974
(Diff revision 1)
>    , mPointerEvents(NS_STYLE_POINTER_EVENTS_AUTO)
>    , mCursor(NS_STYLE_CURSOR_AUTO)
>  {

You need to insert mCaretColor into this constructor init list, with its initial value.  (You've added it to the copy-constructor, but not to this more-basic constructor).

I think it needs to be initialized here to whatever your representation of "auto" is going to be.
Attachment #8816845 - Flags: review?(dholbert) → review-
Also: you should send an intent-to-implement-and-ship email for this feature.
Quick comment as the spec editor. I'm only responding to the prose in the review, not to the code (as I am not familiar with Gecko), so I may be missing some context which would make my answer somewhat off or irrelevant, but here it goes.

> 'auto' is not quite indistinguishable from 'currentcolor' (as this comment suggests it is)
> -- because we need to still return "auto" from getComputedStyle.

There's an open issue about whether getComputerStyle() should return the used value rather than the computed value for color properties, and it seems likely we'll go with used values https://github.com/w3c/csswg-drafts/issues/566

However, auto and currentcolor would still be observably different in transitions and animations: currentcolor would animate as a color but auto would trigger a discrete jump. (and if you animate the color property, both would follow continuously, so the current behavior is preserved).
(In reply to Florian Rivoal from comment #41)
> However, auto and currentcolor would still be observably different in
> transitions and animations: currentcolor would animate as a color but auto
> would trigger a discrete jump. (and if you animate the color property, both
> would follow continuously, so the current behavior is preserved).

Right. I was wrong in the code here. I'll update the patch.
Comment on attachment 8820966 [details]
Bug 1063162 part 1 - Add auto value support to StyleComplexColor.

https://reviewboard.mozilla.org/r/100346/#review100918

::: layout/style/StyleComplexColor.h:27
(Diff revision 1)
>   */
>  struct StyleComplexColor
>  {
>    nscolor mColor;
>    uint8_t mForegroundRatio;
> +  // Whether the complex color represents an computed-value time auto

(Nit: a computed-value time auto)
Attachment #8820966 - Flags: review?(bbirtles) → review+
Comment on attachment 8816845 [details]
Bug 1063162 part 2 - Implement caret-color property.

https://reviewboard.mozilla.org/r/97368/#review100960

r=me with nits addressed as you see fit:

::: layout/style/nsRuleNode.cpp:1140
(Diff revisions 1 - 3)
> +    nscolor resultColor;
>      if (!SetColor(aValue, aParentColor.mColor, aPresContext,
> -                  nullptr, aResult.mColor, aConditions)) {
> +                  nullptr, resultColor, aConditions)) {
>        MOZ_ASSERT_UNREACHABLE("Unknown color value");
>        return;
>      }
> -    aResult.mForegroundRatio = 0;
> +    aResult = StyleComplexColor::FromColor(resultColor);

I think this part is unrelated to the behavior-changes here -- look like this change is just cleaning this StyleComplexColor code to use a cleaner API, right? (but not changing the actual result/behavior)

It'd probably be best to split this part out into a trivial "part 3" here (rs=me on that part), to make the main behavior-changing patch here more targeted.

::: layout/style/nsRuleNode.cpp:5293
(Diff revisions 1 - 3)
> +  if (caretColorValue->GetUnit() == eCSSUnit_Auto) {
> +    ui->mCaretColor = StyleComplexColor::Auto();
> +  } else {
> +    SetComplexColor<eUnsetInherit>(*caretColorValue,

Perhaps we should just handle the "auto" special case inside of the SetComplexColor impl?

(SetComplexColor seems to aim to be a generic function to convert from nsCSSValue to StyleComplexColor, and as of part 1 here, "auto" is a *legitimate* StyleComplexColor value. So seems like something that SetComplexColor can/should handle for us.)
Attachment #8816845 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #47)
> It'd probably be best to split this part out into a trivial "part 3" here
> (rs=me on that part)

(Hmm -- regarding "rs=me", I'm just now remembering MozReview won't let you trigger autoland until an explicit r+ has been granted on this hypothetical part 3. :)  I promise to grant swift r+ though, if you do take me up on this and spin off a part 3 patch here.)
Comment on attachment 8816845 [details]
Bug 1063162 part 2 - Implement caret-color property.

https://reviewboard.mozilla.org/r/97368/#review100960

> I think this part is unrelated to the behavior-changes here -- look like this change is just cleaning this StyleComplexColor code to use a cleaner API, right? (but not changing the actual result/behavior)
> 
> It'd probably be best to split this part out into a trivial "part 3" here (rs=me on that part), to make the main behavior-changing patch here more targeted.

It changes, actually, because otherwise `aResult.mIsAuto` would be kept unchanged here, which could lead to undesired issue. (Actually this was caught by the `test_transition_per_property.html`, and thus I decided to slightly revise this part to make it more robust.)
Comment on attachment 8816845 [details]
Bug 1063162 part 2 - Implement caret-color property.

https://reviewboard.mozilla.org/r/97368/#review100960

> Perhaps we should just handle the "auto" special case inside of the SetComplexColor impl?
> 
> (SetComplexColor seems to aim to be a generic function to convert from nsCSSValue to StyleComplexColor, and as of part 1 here, "auto" is a *legitimate* StyleComplexColor value. So seems like something that SetComplexColor can/should handle for us.)

Sounds reasonable. So we will rely on the parser not to pass in `auto` if syntax doesn't allow it.
Comment on attachment 8816845 [details]
Bug 1063162 part 2 - Implement caret-color property.

https://reviewboard.mozilla.org/r/97368/#review100960

> It changes, actually, because otherwise `aResult.mIsAuto` would be kept unchanged here, which could lead to undesired issue. (Actually this was caught by the `test_transition_per_property.html`, and thus I decided to slightly revise this part to make it more robust.)

Ah, ok. This change might arguably belong in Part 1 then? (as part of the mechanics of setting up mIsAuto, independent of "caret-color" in particular)

It's fine here too, though.

> Sounds reasonable. So we will rely on the parser not to pass in `auto` if syntax doesn't allow it.

Yup, exactly.
"You must have scm_level_3 access to land" /_\
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5bc2005e01a6
part 1 - Add auto value support to StyleComplexColor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/148e65bd3f3b
part 2 - Implement caret-color property. r=dholbert
Keywords: checkin-needed
Backed out for failing own test caret-color-01.html at least on OSX and Windows 8 x64:

https://hg.mozilla.org/integration/autoland/rev/4151f3a5d01f6d2e7874b35837c2c22288de813e
https://hg.mozilla.org/integration/autoland/rev/d9226ec48879c0da76bc41b48d6b14091cc224f7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=148e65bd3f3b156d58836dca9fe8055ae5c2c5d1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8332144&repo=autoland
> caret-color-01.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/css-ui/caret-color-01-ref.html | image comparison, max difference: 255, number of differing pixels: 16
Flags: needinfo?(xidorn+moz)
(In the reftest snapshot, the reference case's small-green-rect is not quite as tall as the testcase's.)
Is a RepaintFrame hint guaranteed to be good enough to repaint the caret?  I'm not sure of this, since I'd think the caret could extend outside the bounds repainted by processing a RepaintFrame hint.
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #58)
> Is a RepaintFrame hint guaranteed to be good enough to repaint the caret? 
> I'm not sure of this, since I'd think the caret could extend outside the
> bounds repainted by processing a RepaintFrame hint.

I *think* it is enough, because caret blink also just schedules a paint on the frame. [1] As far as we never saw a problem on blinking, we shouldn't see any here either. I have no idea how that works, though.

I also think RepaintFrame is supposed to work. If it's not, we should fix the paint area or frame overflow area calculation, rather than using a different hint here, because otherwise we need to change the hint for color property as well, which would have a larger impact.


[1] https://dxr.mozilla.org/mozilla-central/rev/1156db49e976173fc3cf90d2126456fd1e2bae4b/layout/base/nsCaret.cpp#947
Keywords: checkin-needed
It seems the try run is good enough now.
Flags: needinfo?(xidorn+moz)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/320d6acadc2a
part 1 - Add auto value support to StyleComplexColor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/45fa28cafc03
part 2 - Implement caret-color property. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/320d6acadc2a
https://hg.mozilla.org/mozilla-central/rev/45fa28cafc03
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
As you can see in the attached example, "caret-color" cannot be applied to visited links.
If you've a custom style for visited links you might want to change the caret-color for them too.

This causes that Firefox doesn't pass this W3C test:
http://test.csswg.org/source/css-ui-3/caret-color-015.html

I don't know if you prefer that I fill a new bug for this, or you'll fix it as part of this bug.
(In reply to Manuel Rego Casasnovas from comment #64)
> Created attachment 8822414 [details]
> Example setting caret-color on a visited link
> 
> 
> As you can see in the attached example, "caret-color" cannot be applied to
> visited links.
> If you've a custom style for visited links you might want to change the
> caret-color for them too.
> 
> This causes that Firefox doesn't pass this W3C test:
> http://test.csswg.org/source/css-ui-3/caret-color-015.html

Oh, I forgot that :/

> I don't know if you prefer that I fill a new bug for this, or you'll fix it
> as part of this bug.

It should be in a new bug.
Depends on: 1326189
Documented this property at https://developer.mozilla.org/en-US/docs/Web/CSS/caret-color, added syntax and basic information about it in https://github.com/mdn/data/pull/29 (which will be displayed where the MDN page currently says that the values are not found in the DB) and added a release note to https://developer.mozilla.org/en-US/Firefox/Releases/53.

Xidorn, can you please have a quick look whether the documentation is ok?

Sebastian
Flags: needinfo?(xidorn+moz)
It may be worth a note that, <color> value can be "currentcolor", which is effectively the same as "auto" keyword in most cases, but they have different characteristic on interpolation (transition / animation). "auto" is not interpolatible with any <color> value, for forward compatibility, while "currentcolor" is interpolatible with other <color> values.

It's probably also worth mentioning that it affects color of both normal caret and navigation caret.

Otherwise, it looks good. Thanks!
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #67)
> It may be worth a note that, <color> value can be "currentcolor", which is
> effectively the same as "auto" keyword in most cases, but they have
> different characteristic on interpolation (transition / animation). "auto"
> is not interpolatible with any <color> value, for forward compatibility,
> while "currentcolor" is interpolatible with other <color> values.

Is that actually documented within the specifications? I couldn't find a clear statement about that.
Anyway, I've added the note now.

> It's probably also worth mentioning that it affects color of both normal
> caret and navigation caret.

Done.

> Otherwise, it looks good. Thanks!

Thank you for the review!

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Sebastian Zartner [:sebo] from comment #68)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #67)
> > It may be worth a note that, <color> value can be "currentcolor", which is
> > effectively the same as "auto" keyword in most cases, but they have
> > different characteristic on interpolation (transition / animation). "auto"
> > is not interpolatible with any <color> value, for forward compatibility,
> > while "currentcolor" is interpolatible with other <color> values.
> 
> Is that actually documented within the specifications? I couldn't find a
> clear statement about that.
> Anyway, I've added the note now.

Actually, per current spec, currentcolor is not interpolatable either, but we want currentcolor to be interpolatable with other colors but not auto. It is a consensus between implementor of Chrome, the editor of the spec, and I. See https://github.com/w3c/csswg-drafts/issues/781 for the discussion.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #69)
> (In reply to Sebastian Zartner [:sebo] from comment #68)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #67)
> > > It may be worth a note that, <color> value can be "currentcolor", which is
> > > effectively the same as "auto" keyword in most cases, but they have
> > > different characteristic on interpolation (transition / animation). "auto"
> > > is not interpolatible with any <color> value, for forward compatibility,
> > > while "currentcolor" is interpolatible with other <color> values.
> > 
> > Is that actually documented within the specifications? I couldn't find a
> > clear statement about that.
> > Anyway, I've added the note now.
> 
> Actually, per current spec, currentcolor is not interpolatable either, but
> we want currentcolor to be interpolatable with other colors but not auto. It
> is a consensus between implementor of Chrome, the editor of the spec, and I.
> See https://github.com/w3c/csswg-drafts/issues/781 for the discussion.

I see. Thank you for pointing me at that! So it looks like we're just waiting for consensus of the working group, then.

According to the spec. the computed value of the implementation is currently wrong, though. 'auto' computes to an rgb() value instead of 'auto'.
Should I file a new bug for this or should I wait for the CSSWG resolution?

Sebastian
Flags: needinfo?(xidorn+moz)
(In reply to Sebastian Zartner [:sebo] from comment #70)
> According to the spec. the computed value of the implementation is currently
> wrong, though. 'auto' computes to an rgb() value instead of 'auto'.
> Should I file a new bug for this or should I wait for the CSSWG resolution?

There is no issue here, either in the spec or in the impl.

It has been a consensus that 'currentcolor' and 'auto' are computed to themselves, while getComputedStyle returns the **used value**, which is always rgb() / rgba(), rather than their computed value, which can be a keyword. See https://drafts.csswg.org/cssom/#resolved-values
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #71)
> (In reply to Sebastian Zartner [:sebo] from comment #70)
> > According to the spec. the computed value of the implementation is currently
> > wrong, though. 'auto' computes to an rgb() value instead of 'auto'.
> > Should I file a new bug for this or should I wait for the CSSWG resolution?
> 
> There is no issue here, either in the spec or in the impl.
> 
> It has been a consensus that 'currentcolor' and 'auto' are computed to
> themselves, while getComputedStyle returns the **used value**, which is
> always rgb() / rgba(), rather than their computed value, which can be a
> keyword. See https://drafts.csswg.org/cssom/#resolved-values

Thank you for clarifying this!
Ok, so, then the issue is rather in Blink's implementation, because in Chrome getComputedStyle() returns 'auto' in this case.
On the other hand, why does getComputedStyle() return 'transparent' in Gecko when the computed value for 'transparent' is already defined as 'rgba(0, 0, 0, 0)'[1] and the used value is the "absolute theoretical value used in the layout"? (FWIW, Blink returns the rgba() value in this case, Trident/EdgeHTML also return 'transparent'.)

Sorry if we're drifting out of the scope of this bug, though I'd like to have this clarified for browser compatibility and documentation reasons.

Sebastian

[1] https://drafts.csswg.org/css-color-3/#transparent-def
Flags: needinfo?(xidorn+moz)
(In reply to Sebastian Zartner [:sebo] from comment #72)
> Ok, so, then the issue is rather in Blink's implementation, because in
> Chrome getComputedStyle() returns 'auto' in this case.

Blink has a pending patch to fix this: https://codereview.chromium.org/2577633002/
I'll be sure that it's going to be part of Chromium 57.
(In reply to Sebastian Zartner [:sebo] from comment #72)
> Ok, so, then the issue is rather in Blink's implementation, because in
> Chrome getComputedStyle() returns 'auto' in this case.

What Manuel said.

> On the other hand, why does getComputedStyle() return 'transparent' in Gecko
> when the computed value for 'transparent' is already defined as 'rgba(0, 0,
> 0, 0)'[1] and the used value is the "absolute theoretical value used in the
> layout"? (FWIW, Blink returns the rgba() value in this case,
> Trident/EdgeHTML also return 'transparent'.)

This seems to be a very old code [1] which I guess we should fix. Given that Blink returns rgba value, I guess there shouldn't be much compatibility concern. Could you file a bug for this?


[1] https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/layout/style/nsComputedDOMStyle.cpp#973-976
Flags: needinfo?(xidorn+moz)
See Also: → 1328224
(In reply to Manuel Rego Casasnovas from comment #73)
> (In reply to Sebastian Zartner [:sebo] from comment #72)
> > Ok, so, then the issue is rather in Blink's implementation, because in
> > Chrome getComputedStyle() returns 'auto' in this case.
> 
> Blink has a pending patch to fix this:
> https://codereview.chromium.org/2577633002/
> I'll be sure that it's going to be part of Chromium 57.

Very good! Thanks for the note!

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #74)
> > On the other hand, why does getComputedStyle() return 'transparent' in Gecko
> > when the computed value for 'transparent' is already defined as 'rgba(0, 0,
> > 0, 0)'[1] and the used value is the "absolute theoretical value used in the
> > layout"? (FWIW, Blink returns the rgba() value in this case,
> > Trident/EdgeHTML also return 'transparent'.)
> 
> This seems to be a very old code [1] which I guess we should fix. Given that
> Blink returns rgba value, I guess there shouldn't be much compatibility
> concern. Could you file a bug for this?

Created bug 1328224. Thank you for the fast reply!

Sebastian
Attachment #8540005 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: