1260543 - twitter notifications count flashes when highlighting headers

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This just started happening to me today on nightly 48.0a1 (2016-03-28).

STR:

1) Log into twitter
2) Have another account do something to generate notifications to you.  Favoriting your tweets or tweeting @ you.
3) Note the notifications header at the top of the page indicates how many notifications you have.
4) Mouse over the Home, Moments, Notifications, and Messages page headers.

At this point you should see the notifications count flash off and on as you highlight each header.  The bubble itself does not go away; just the text inside the bubble.

I don't see this behavior with dev edition or chrome.

Note sure if this is style, layout, or something else.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Firefox: 48.0a1, Build ID: 20160329030246
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0

Hi Ben,

I have tested  this issue on the latest Nightly (48.0a1) build and I have managed to reproduce it using the provided steps. I have performed a regression and this are the results:

Last good revision: b45ee3e065b7c9defd8877d01fe948db18230c87
First bad revision: 46b58297c2dee95fea660c3fae987fda7b3e36fb
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b45ee3e065b7c9defd8877d01fe948db18230c87&tochange=46b58297c2dee95fea660c3fae987fda7b3e36fb

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1247777

Thanks,
Cosmin.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Thanks!

Jeremy, any ideas?

Comment 3

[Mike Taylor [:miketaylr]]

Attachment: testcase 1

Here's a reduced test case.

Seems when there's a flicker when we transition the color property.

Comment 4

[Mike Taylor [:miketaylr]]

(Now with actual English: *Seems that there is a flicker when we transition the color property.)

Comment 5

[Daniel Holbert [:dholbert]]

(I can reproduce this bug in Nightly *regardless* of whether the "layout.css.prefixes.webkit" pref is enabled or disabled.  So, I don't think it makes sense to tie this to bug 1259345 [the bug on flipping that pref for release users].)

Comment 6

[Mike Taylor [:miketaylr]]

(My bad, I should have tested that -- thanks Daniel!)

Comment 7

[Daniel Holbert [:dholbert]]

So, as of bug 1247777 (the regressing bug), the color of text is controlled by the "-webkit-text-fill-color" CSS property, which happens to be set to currentColor by default, which means it should match the value of the "color" property.

This bug seems to indicate that we're resolving its 'currentColor' to the wrong value during the transition, though.  So, something seems to be busted with style resolution during transitions here.  Maybe we're using the wrong style context / wrong style information for this text during the transition, or something...

Comment 8

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]: Regression in Firefox 48.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (background is also styled with "currentColor")

This testcase demonstrates that we get this correct for "background:currentColor" with a transitioning "color" property, at least.

(The background correctly transitions in the first part of the testcase, and it correctly stays fixed in the second part of the testcase.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: (ignore, contains typo)

If I explicitly set "-webkit-text-fill-color: currentColor" on the .text element, then the bug goes away.

So the problem only happens when we *inherit* "-webkit-text-fill-color:currentColor" (as we do by default), when "color" is changing on the parent but fixed on the child.

I'll bet we're handling that inheritance inconsistently -- we must be inheriting the actual resolved color value in some cases (BAD), vs. inheriting the "currentColor" placeholder value in other cases (GOOD).

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (doesn't repro bug, due to explicit "-webkit-text-fill-color: currentColor", somehow)

(Sorry, previous version of testcase 3 had a copypaste-typo'd comment which made things confusing. Fixed here.)

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: testcase 4 (doesn't repro bug, when resolving -webkit-text-fill-color w/o css-transition)

dholbert seems reply the root cause in Comment 10 already. I'm guessing that resolving color value for text under transition effect is different from that w/o transition.

Here is another test to prove that, w/o css-transition, a child element inherits the "currentColor" instead of the actual resolved color value from parent.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

It seems text-emphasis-color actually has the same issue.

So basically, the issue is that, we insert a rule node on <a> for the transition (in the context of testcase 1), which specifies all properties affected by color with their final used value. Then, -webkit-text-fill-color (and text-emphasis-color) on the child node <p class="text"> inherits the value from that rule node, and thus this happens.

I have no idea how this could be fixed...

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

I probably have an idea how to fix it. Taking it for now.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: wip patch

This is what I'm thinking.

This patch adds a new unit "CurrentColor" to StyleAnimationValue, so that we can distinguish between a concrete color value and "currentcolor". It treats all currentcolor to be equal, and allow interpolating currentcolor and a color value via storing StyleColor()->mColor with this unit.

In my preliminary testing, it looks promising. More tests are necessary, but before I go deeper, I'd like to hear from heycam first: do you think this is the right way to go? (Actually I wanted to ask dbaron since he has ni?ed himself here, but it seems he's not accepting feedback request currently.)

Also it seems to me the spec is not clear about the interpolation between color value and currentcolor, and there was a discussion in www-style about this several years ago: https://lists.w3.org/Archives/Public/www-style/2012Apr/0827.html

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ac0ada97d7

So it seems the only failure so far is that, with this change, 'transitionend' event is not triggered for -webkit-text-fill-color and text-emphasis-color for change on color property, which I guess is actually the expected behavior, given currentcolor itself has become a computed value in the spec.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> So it seems the only failure so far is that, with this change,
> 'transitionend' event is not triggered for -webkit-text-fill-color and
> text-emphasis-color for change on color property, which I guess is actually
> the expected behavior, given currentcolor itself has become a computed value
> in the spec.

Yeah, that sounds like the correct behavior.  If there's no transition *directly* happening on a given property on a given node, then there shouldn't be any transitionend event posted for that property.

(I expect this should also be what happens for "-webkit-text-fill-color: inherit" when the inherited value is being transitioned, as well as for currentColor on other color-valued properties like "background-color".)

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> > So it seems the only failure so far is that, with this change,
> > 'transitionend' event is not triggered for -webkit-text-fill-color and
> > text-emphasis-color for change on color property, which I guess is actually
> > the expected behavior, given currentcolor itself has become a computed value
> > in the spec.
> 
> Yeah, that sounds like the correct behavior.  If there's no transition
> *directly* happening on a given property on a given node, then there
> shouldn't be any transitionend event posted for that property.
> 
> (I expect this should also be what happens for "-webkit-text-fill-color:
> inherit" when the inherited value is being transitioned, as well as for
> currentColor on other color-valued properties like "background-color".)

It is probably not what the spec is saying. It seems to me all the events are based on the computed style rather than specified value, which means if a value is changed, the event should also be fired on elements which inherit that value.

However, currentcolor is changed to a computed value a while ago, so changing color shouldn't cause the change of computed value of currentcolor any more. We haven't change our behavior here, but we do want to change it eventually in bug 760345.

Comment 19

[Cameron McCormack (:heycam)]

Brian is more familiar with StyleAnimationValue, so I might move the feedback? to him.  (Sorry for the delay in replying here.)

This issue can happen with any property that takes colors, right?  I wonder if it is not too much more work to just fix bug 760345?  Although I see that it's important to get it fixed for -webkit-text-fill-color quickly as that is going to be much more obvious than with these other properties.

Comment 20

[Cameron McCormack (:heycam)]

(One thing I'm not sure about with the patch is what the implication is of making two currentColor value have distance 0, even if their actual colors are different.)

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #19)
> This issue can happen with any property that takes colors, right?

It can happen with any these property if it has "inherit" specified. However, with "currentcolor" not being a computed value, that is the expected behavior.

> I wonder
> if it is not too much more work to just fix bug 760345?  Although I see that
> it's important to get it fixed for -webkit-text-fill-color quickly as that
> is going to be much more obvious than with these other properties.

I actually consider the work here being part of bug 760345.

Making "currentcolor" a computed value itself could be as easy as changing the code in nsComputedDOMStyle to return the keyword rather than the concrete value. (Converting properties which do not use the bool-nscolor pair could be a non-trivial work, though.) However, to get the right behavior for animation, you may still need to do something here.

Comment 22

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #21)
> It can happen with any these property if it has "inherit" specified.
> However, with "currentcolor" not being a computed value, that is the
> expected behavior.

The flickering cannot be the expected behaviour, though.  What is the expected behaviour if currentColor is not a computed value?

> > I wonder
> > if it is not too much more work to just fix bug 760345?  Although I see that
> > it's important to get it fixed for -webkit-text-fill-color quickly as that
> > is going to be much more obvious than with these other properties.
> 
> I actually consider the work here being part of bug 760345.

Yes.
 
> Making "currentcolor" a computed value itself could be as easy as changing
> the code in nsComputedDOMStyle to return the keyword rather than the
> concrete value. (Converting properties which do not use the bool-nscolor
> pair could be a non-trivial work, though.) However, to get the right
> behavior for animation, you may still need to do something here.

OK.  I wonder whether it makes sense to do only "half" of the change, by making transitions work as if currentColor were a computed value, but not the rest.

I'll leave it to Brian to say whether the StyleAnimationValue change by itself marks sense.  If you have time to complete bug 760345 it might be a good time to do it. :-)

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #22)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #21)
> > It can happen with any these property if it has "inherit" specified.
> > However, with "currentcolor" not being a computed value, that is the
> > expected behavior.
> 
> The flickering cannot be the expected behaviour, though.  What is the
> expected behaviour if currentColor is not a computed value?

If currentcolor (note that in CSS it is actually now lowercased) is just a specified value, and would be computed to a concrete color value then, the flicking here would be the expected result from the spec's point of view, because the animating of a concrete color value could certainly be inherited by its children.

I suspect that the spec being changed is because of this kind of issues.

Comment 24

[Brian Birtles (:birtles)]

Comment on attachment 8739307 [details] [diff] [review]
wip patch

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

Sorry for the delay here, I spent quite a while trying to work out if this is ok. Given that currentcolor is going to become a computed value, and StyleAnimationValue nominally represents computed values, I think this might be ok.

A few minor points:

* As Cameron points out, returning zero as the distance probably is less than useful. Presumably we should be able to calculate the distance between "red; currentColor; green". I guess that currently doesn't work, however?
* I wonder what UncomputeValue should do for this value. For the nsCSSValue case this needs to map to a currentColor value. For the nsAString case we could either return "currentColor" or the actual rgb value. The former is probably more accurate, the latter more useful (this will show up, for example, when you inspect the transition endpoints in the animation inspector).
* We probably should rebase this patch on m-c to check it still works with all the changes that landed in this area on Monday.

One thing I still don't quite understand from a spec point of view, however, is if currentcolor is supposed to be computed value and transitions operate on computed values, should we even have a transition on the -webkit-text-fill-color property?

(If currentcolor is not supposed to be a computed value then I think this patch is not correct.)

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brian Birtles (:birtles) from comment #24)
> Comment on attachment 8739307 [details] [diff] [review]
> wip patch
> 
> Review of attachment 8739307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay here, I spent quite a while trying to work out if this
> is ok. Given that currentcolor is going to become a computed value, and
> StyleAnimationValue nominally represents computed values, I think this might
> be ok.

Thanks for giving feedback.

> A few minor points:
> 
> * As Cameron points out, returning zero as the distance probably is less
> than useful. Presumably we should be able to calculate the distance between
> "red; currentColor; green". I guess that currently doesn't work, however?

That should work. Distance between an actual color and currentcolor is not zero, because the GetCommonUnit returns Color if one side is Color and the other is CurrentColor. And since CurrentColor uses the same field to store its corresponding color, it is handled directly in the Color branch. It returns 0 only when both sides are CurrentColor.

> * I wonder what UncomputeValue should do for this value. For the nsCSSValue
> case this needs to map to a currentColor value. For the nsAString case we
> could either return "currentColor" or the actual rgb value. The former is
> probably more accurate, the latter more useful (this will show up, for
> example, when you inspect the transition endpoints in the animation
> inspector).

Probably. Is the return value from the nsAString overload accessible via any DOM API for content? If so, I guess we should not do that. If it is there only for animation inspector, then that's probably worth doing so.

> * We probably should rebase this patch on m-c to check it still works with
> all the changes that landed in this area on Monday.

I rebase all my local patches on a daily basis, and I did not see any conflict there, so I guess it should still work.

> One thing I still don't quite understand from a spec point of view, however,
> is if currentcolor is supposed to be computed value and transitions operate
> on computed values, should we even have a transition on the
> -webkit-text-fill-color property?

I don't think so, but that may be worth raising a discussion in www-style. I think transition should not happen if the computed value is not changed.

> (If currentcolor is not supposed to be a computed value then I think this
> patch is not correct.)

currentcolor should be a computed value per spec. If it is not, this bug would indicate an issue of the spec.

Comment 26

[Brian Birtles (:birtles)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> That should work. Distance between an actual color and currentcolor is not
> zero, because the GetCommonUnit returns Color if one side is Color and the
> other is CurrentColor. And since CurrentColor uses the same field to store
> its corresponding color, it is handled directly in the Color branch. It
> returns 0 only when both sides are CurrentColor.

Oh, sorry, I missed the interaction with GetCommonUnit there. It might be nice not to return 0 for two currentcolor values with different mValue members but I can't really think of a situation where that would occur and actually matter (we don't do paced animation with transitions).

> > * I wonder what UncomputeValue should do for this value. For the nsCSSValue
> > case this needs to map to a currentColor value. For the nsAString case we
> > could either return "currentColor" or the actual rgb value. The former is
> > probably more accurate, the latter more useful (this will show up, for
> > example, when you inspect the transition endpoints in the animation
> > inspector).
> 
> Probably. Is the return value from the nsAString overload accessible via any
> DOM API for content? If so, I guess we should not do that. If it is there
> only for animation inspector, then that's probably worth doing so.

Yes, it's only exposed via KeyframeEffectReadOnly.getAnimationProperties() which is chrome-only.

> > * We probably should rebase this patch on m-c to check it still works with
> > all the changes that landed in this area on Monday.
> 
> I rebase all my local patches on a daily basis, and I did not see any
> conflict there, so I guess it should still work.

Ok, great.

> > One thing I still don't quite understand from a spec point of view, however,
> > is if currentcolor is supposed to be computed value and transitions operate
> > on computed values, should we even have a transition on the
> > -webkit-text-fill-color property?
> 
> I don't think so, but that may be worth raising a discussion in www-style. I
> think transition should not happen if the computed value is not changed.

If that's the case, then once currentcolor becomes a computed value, we won't need this patch, right?

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brian Birtles (:birtles) from comment #26)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> > > One thing I still don't quite understand from a spec point of view, however,
> > > is if currentcolor is supposed to be computed value and transitions operate
> > > on computed values, should we even have a transition on the
> > > -webkit-text-fill-color property?
> > 
> > I don't think so, but that may be worth raising a discussion in www-style. I
> > think transition should not happen if the computed value is not changed.
> 
> If that's the case, then once currentcolor becomes a computed value, we
> won't need this patch, right?

currentcolor is actually already a computed value internally for at least text-emphasis-color and text-fill-color. We have special flags in StyleText to store them. And this patch is making it behave like a computed value for animation.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8739307 [details] [diff] [review]
wip patch

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

dbaron, what do you think? Do you think this is the right way to go?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8739307 [details] [diff] [review]
wip patch

So I think what should really happen here is that we should have a currentcolor unit that does *not* store a color along with it.  However, this would require that we be able to handle some sort of value expression (a calc()?) that interpolates between currentcolor and an actual color, and that's a lot of additional work.

So I think this is OK for now, but it could use some more comments explaining that having the color unit as part of the StyleAnimationValue is a hack that should be replaced by having values that interpolate between currentcolor and real colors.  And probably a followup bug on that topic, and probably also a spec issue.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #29)
> explaining that having the color unit as part of the StyleAnimationValue is
> a hack that should be replaced by having values that interpolate between

sorry, I meant having the color *value* along with the currentcolor unit in StyleAnimationValue is a hack

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

It's probably really a spec issue. I guess what you are thinking is that, the value of currentcolor in transition should also follow the value of color at every moment?

e.g. if I have:
> color: #f00 -> #0f0
> text-fill-color: currentcolor -> #00f
and in 50% of the transition, you think text-fill-color should have a used value
> ((#f00 + #0f0) / 2 + #00f) / 2 = #404080
rather than what this patch would do as
> (#f00 + #00f) / 2 = #800080
right?

That does look a lot more complicated, and could potentially be a breaking change for existing properties? I'll email www-style to raise a topic around this later.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #31)
> It's probably really a spec issue. I guess what you are thinking is that,
> the value of currentcolor in transition should also follow the value of
> color at every moment?

If currentcolor is a computed value, I think that is the reasonable way to do it.  But I don't think we need to do that right now.

> e.g. if I have:
> > color: #f00 -> #0f0
> > text-fill-color: currentcolor -> #00f
> and in 50% of the transition, you think text-fill-color should have a used
> value
> > ((#f00 + #0f0) / 2 + #00f) / 2 = #404080
> rather than what this patch would do as
> > (#f00 + #00f) / 2 = #800080
> right?
> 
> That does look a lot more complicated, and could potentially be a breaking
> change for existing properties? I'll email www-style to raise a topic around
> this later.

Yes, it's complicated.

It's also probably a rare case.

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

https://lists.w3.org/Archives/Public/www-style/2016Apr/0281.html

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase shows the issue directly

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

It seems Microsoft Edge also has this bug :)

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

Review commit: https://reviewboard.mozilla.org/r/47969/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47969/

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

It seems we handle animation and transition in different way, and thus this bug doesn't affect animation. But it seems there is some issue around currentcolor in animation, which may need to be addressed separately.

Comment 38

[Brian Birtles (:birtles)]

Comment on attachment 8743636 [details]
MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

https://reviewboard.mozilla.org/r/47969/#review44753

r=me with comments addressed

::: layout/reftests/bugs/1260543-1.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<style>
> +#outer {
> +  font-size: 52px;
> +  transition: all 1576800000s step-start;

Nit: I'm not sure we need quite such a long transition? We haven't seen any intermittent failures from animation tests due to them finishing before the test does since we started making them 100s long. Unless there's a reason for this long duration, it might be more consistent to use 100s, but up to you.

::: layout/reftests/bugs/1260543-1.html:16
(Diff revision 1)
> +}
> +#outer.red > #inner {
> +  color: green;
> +}
> +</style>
> +<p>Test passes if the block below is green.</p>

Please add a comment saying what this is testing.

::: layout/style/StyleAnimationValue.cpp:3013
(Diff revision 1)
>      case eUnit_Color:
>        // colors can be alone, or part of a paint server
>        aSpecifiedValue.SetColorValue(aComputedValue.GetColorValue());
>        break;
> +    case eUnit_CurrentColor:
> +      aSpecifiedValue.SetIntValue(NS_COLOR_CURRENTCOLOR, eCSSUnit_Enumerated);

Should this be:

  aSpecifiedValue.SetIntValue(NS_COLOR_CURRENTCOLOR, eCSSUnit_EnumColor);

?

From what I can see from nsCSSValue::AppendToString, that should give us back the right serialization for this.

Is it possible to add a test like the one you provided in [1] that steps through an animation/transition and queries the style--or does that not work because getComputedStyle returns the used values in this case?

(I can help with the set up for stepping through animations/transitions if needed--you basically just need to use a negative delay.)

[1] https://lists.w3.org/Archives/Public/www-style/2016Apr/0281.html

::: layout/style/StyleAnimationValue.cpp:3157
(Diff revision 1)
>  {
>    return reinterpret_cast<char*>(aStyleStruct) + aOffset;
>  }
>  
>  static void
> -ExtractBorderColor(nsStyleContext* aStyleContext, const void* aStyleBorder,
> +ExtractColor(bool aIsForground, nscolor aColor,

Can we call aColor something else like aFallbackColor / aDefaultColor / aBackgroundColor or whatever makes sense here?

It seems odd to pass aColor to a function call ExtractColor.

Also, s/aIsForground/aIsForeground/

(I don't normally like bool parameters but since we pass a local variable called 'foreground' at each of the call sites I think this is fine.)

(Really minor nit: Calling this 'ExtractColor' seems a little odd to me--SetCurrentOrBackgroundColor or something like that might make sense--but oh well.)

::: layout/style/StyleAnimationValue.cpp:3173
(Diff revision 1)
> +ExtractBorderColor(const void* aStyleBorder,
>                     mozilla::css::Side aSide,
>                     StyleAnimationValue& aComputedValue)
>  {
>    nscolor color;
>    bool foreground;

Call this useForeground?

::: layout/style/StyleAnimationValue.cpp:3554
(Diff revision 1)
>  
>          case eCSSProperty_outline_color: {
>            const nsStyleOutline *styleOutline =
>              static_cast<const nsStyleOutline*>(styleStruct);
>            nscolor color;
> -          if (!styleOutline->GetOutlineColor(color))
> +          bool foreground = !styleOutline->GetOutlineColor(color);

Again 'useForeground'?

::: layout/style/nsStyleContext.cpp:1308
(Diff revision 1)
> -  return val.GetColorValue();
> +  return val.GetUnit() == StyleAnimationValue::eUnit_CurrentColor
> +    ? aStyleContext->StyleColor()->mColor : val.GetColorValue();
>  }
>  
>  static nscolor
>  ExtractColorLenient(nsCSSProperty aProperty,

Do we need to update this as well or will it never be called for something with a currentcolor value?

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/47969/#review44753

> Do we need to update this as well or will it never be called for something with a currentcolor value?

I think it should be updated as well even if it is currently never called with a currentcolor value.

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8743636 [details]
MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47969/diff/1-2/

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8743636 [details]
MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

I'm afraid that you would need to re-review this patch.

The test failure isn't too hard to fix, but it makes me realize that this patch is a rather larger change than I thought, and the change may need a longer period to verify and document, and thus I don't want to take the risk and land it at the end of a cycle.

So I decide to take a step back and only convert the behavior of the newly-added text-emphasis-color and -webkit-text-fill-color, which are the only inherited color-valued properties (except color and SVG properties) so far.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm curious:  why did you switch from making it interpolable in a slightly wrong way (in attachment 8739307 [details] [diff] [review]) to making it not interpolable at all (in attachment 8743636 [details])?

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Because it's less hacky and has fewer code, and conforms to the current spec which says nothing about interpolation between currentcolor and an actual color value. It also matches Blink's behavior so it shouldn't have serious webcompat issue.

I didn't consider attachment 8739307 [details] [diff] [review] as a hack, but given both you and Tab think we would eventually interpolate them in the a different ideal way, I'd prefer not landing a hack at this point.

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

Try push all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1cbaa7dd3cc

Comment 45

[Brian Birtles (:birtles)]

Comment on attachment 8743636 [details]
MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

https://reviewboard.mozilla.org/r/47969/#review45057

Thanks Xidorn. Looks good.

Can we file a bug to do this for all affected properties like your previous patch? I'd rather not be in the odd state where some properties behave differently to others for too long.

::: layout/style/test/test_transitions_events.html:25
(Diff revision 2)
>    text-emphasis-color: black; /* don't derive from color */
>    -webkit-text-fill-color: black; /* don't derive from color */

Do we still need these?

::: layout/style/test/test_transitions_per_property.html:1241
(Diff revision 2)
>                         "75%");
>    check_distance(prop, "150px", "calc(125px + 12.5%)",
>                         "calc(50% + 50px)");
>  }
>  
> -function test_color_transition(prop, get_color=(x => x), is_shorthand=false) {
> +function test_general_color_transition(prop, get_color=(x => x), is_shorthand=false) {

As discussed on IRC I think it would be easier to understand this test if we do:

  s/test_color_transition/test_currentcolor_transition/
  s/test_general_color_transition/test_color_transition/

Basically, though, I'd be happy with anything more descriptive than 'general'.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brian Birtles (:birtles) from comment #45)
> Comment on attachment 8743636 [details]
> MozReview Request: Bug 1260543 - Treat currentcolor as computed value which
> is not interpolatable with actual color for text-emphasis-color and
> -webkit-text-fill-color. r?birtles
> 
> https://reviewboard.mozilla.org/r/47969/#review45057
> 
> Thanks Xidorn. Looks good.
> 
> Can we file a bug to do this for all affected properties like your previous
> patch? I'd rather not be in the odd state where some properties behave
> differently to others for too long.

So make them not interpolatable as well? Also not all of those properties actually uses currentcolor. Some of them use -moz-use-text-color enumerated, which we probably want to convert to currentcolor as well.

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8743636 [details]
MozReview Request: Bug 1260543 - Treat currentcolor as computed value which is not interpolatable with actual color for text-emphasis-color and -webkit-text-fill-color. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47969/diff/2-3/

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29debcd8e53a

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/479e9cb32dc93bb6cbfc343f97468c6e46b4035d
Bug 1260543 followup - Fix the function name in test_transitions_per_property.html. DONTBUILD

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29debcd8e53a
https://hg.mozilla.org/mozilla-central/rev/479e9cb32dc9

Comment 51

[Jeremy Chen ([e-mail])]

hooray!! \o/
xidorn, thank you for the help.

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 48 in case this reopens.

Comment 53

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I verified this issue on 48.0b2 build1 (20160620091522) using 
- Ubuntu 14.04 x86
- Windows 10 x64
- Mac OS X 10.11
I can confirm that the fix landed and it works as expected. Therefore, I'll mark this build as Verified and the bug as Verified Fixed.