Implement the -webkit-text-fill-color property

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: miketaylr, Assigned: jeremychen)

Tracking

(Depends on 1 bug, {dev-doc-complete})

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

()

Attachments

(7 attachments, 19 obsolete attachments)

4.03 KB, text/html
Details
38 bytes, text/plain
Details
1.13 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
5.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.36 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
22.24 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
8.38 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
We should add support for `-webkit-text-fill-color: transparent`, which is used by sites to get fancy `-webkit-background-clip: text` effects. 

Setting Bug 759568 as a blocker for this, because these two go hand-in-hand to fix the various "-webkit-background-clip: text" visual-regressions/bugs we've seen. (And by itself it's not very useful or interesting.)

https://github.com/whatwg/compat/issues/38 is the issue to spec this. We *might* be able to just alias this to `color: transparent` (or, rather, just color). I'll be looking into that, but initial (quick) tests seem promising.
Reporter

Comment 1

3 years ago
Defined here: https://compat.spec.whatwg.org/#the-webkit-text-fill-color

Basically, this should work as an alias to the color property, but declarations should always be treated as !important.
Summary: Implement -webkit-text-fill-color: transparent → Implement the -webkit-text-fill-color property
See Also: → 1248708
(In reply to Mike Taylor [:miketaylr] from comment #1)
> Basically, this should work as an alias to the color property, but
> declarations should always be treated as !important.

I wonder if a better way to represent this is:
 (1) -webkit-text-fill-color is now what should actually determines the color of text -- *not* the "color" property.
 (2) The initial value of -webkit-text-fill-color is "currentColor"

This seems to be how Blink implements it, I think (and I presume WebKit as well).  See in particular:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blink/core/StyleBuilderFunctions.cpp&sq=package:chromium&type=cs&l=4365&rcl=1455669015
> void StyleBuilderFunctions::applyInitialCSSPropertyWebkitTextFillColor(StyleResolverState& state)
> {
>     StyleColor color = StyleColor::currentColor();
>     if (state.applyPropertyToRegularStyle())
>         state.style()->setTextFillColor(color);
>     if (state.applyPropertyToVisitedLinkStyle())
>         state.style()->setVisitedLinkTextFillColor(color);
If I'm reading this correctly, it indicates that the initial value of -webkit-text-fill-color is "currentColor".

Local testing in Chrome confirms to me that this is how it operates, too -- e.g.
 * If I create an element with style="color: red", then I can see that its computed -webkit-text-fill-color is also red (in RGB form). This proves that the initial value of -webkit-
 * If I instead set "-webkit-text-fill-color: green", then I can see that its computed 'color' is rgb(0,0,0) -- i.e. black, the default color value. So -webkit-text-fill-color *does not* influence the computed 'color', AFAICT.  This means treating it as an alias for color (with or without "!important") would be incorrect.
Reporter

Comment 3

3 years ago
Thanks, I've reopened the spec issue and will update how we define this (and thanks for looking into this!).
Assignee: nobody → cku
Status: NEW → ASSIGNED
After a brief offline discussion w/ CJ and Astley, I'm taking this bug. Will start to work on this tomorrow.
Assignee: cku → jeremychen
Comment on attachment 8730180 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color property (v1)

Hi Xidorn, could you take a look and give me some feedback? Thank you.
Attachment #8730180 - Flags: feedback?(quanxunzhen)
Comment on attachment 8730180 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color property (v1)

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

::: layout/style/nsCSSPropList.h
@@ +3333,5 @@
> +    _webkit_text_fill_color,
> +    WebkitTextFillColor,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_HASHLESS_COLOR_QUIRK,

Does WebKit allow hashless color quirk like |-webkit-text-fill-color: 0000FF|? If not we shouldn't do this. If it does, we should get the compat spec changed to reflect this.

FWIW, the hashless color quirk is mentioned in Quirks Mode Standard: https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk

::: layout/style/nsRuleNode.cpp
@@ +4613,5 @@
> +    text->mTextFillColor = parentText->mTextFillColor;
> +  } else if ((webkitTextFillColorValue->GetUnit() == eCSSUnit_EnumColor &&
> +              webkitTextFillColorValue->GetIntValue() == NS_COLOR_CURRENTCOLOR) ||
> +             webkitTextFillColorValue->GetUnit() == eCSSUnit_Initial) {
> +    text->mTextFillColor = mPresContext->DefaultColor();

This doesn't seem correct. currentcolor is a special keyword value in CSS, which is computed to itself (per the latest CSS Color spec [1]). It becomes the value of 'color' at the time it is used (not computed).

You may want to use the same logic I used for text-emphasis-color property, which has the same behavior as this property (inherited, initial is currentcolor).

[1] https://drafts.csswg.org/css-color/#currentcolor-color

::: layout/style/nsStyleContext.cpp
@@ +1059,5 @@
>          change = true;
>        }
> +      if (thisVisText->mTextFillColor != otherVisText->mTextFillColor) {
> +        change = true;
> +      }

This needs to be changed as well with the suggestion above.
Attachment #8730180 - Flags: feedback?(quanxunzhen) → feedback-
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #7)
> Comment on attachment 8730180 [details] [diff] [review]
> Part1: parse and compute -webkit-text-fill-color property (v1)
> 
> Review of attachment 8730180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSPropList.h
> @@ +3333,5 @@
> > +    _webkit_text_fill_color,
> > +    WebkitTextFillColor,
> > +    CSS_PROPERTY_PARSE_VALUE |
> > +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> > +        CSS_PROPERTY_HASHLESS_COLOR_QUIRK,
> 
> Does WebKit allow hashless color quirk like |-webkit-text-fill-color:
> 0000FF|? If not we shouldn't do this. If it does, we should get the compat
> spec changed to reflect this.
> 
> FWIW, the hashless color quirk is mentioned in Quirks Mode Standard:
> https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk

Thank you for the feedback. After doing some verifications, hashless color quirk seems not valid for -webkit-text-fill-color on Chrome. I'll fix this and other comments you've addressed.
Comment hidden (obsolete)
Attachment #8730180 - Attachment is obsolete: true
Attachment #8730533 - Attachment is obsolete: true
Attachment #8730547 - Attachment is obsolete: true
Comment on attachment 8730555 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color propertyi (v2)

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

::: layout/style/nsCSSPropList.h
@@ +3332,5 @@
> +    -webkit-text-fill-color,
> +    _webkit_text_fill_color,
> +    WebkitTextFillColor,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE,

Do we need CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED here?
Attach this link to be another source for verification. This test is originally written by Mike [:miketaylr]. For more discussion, please refer to https://github.com/whatwg/compat/issues/38
Attachment #8730555 - Flags: feedback?(quanxunzhen)
Attachment #8730599 - Flags: feedback?(quanxunzhen)
Comment on attachment 8730555 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color propertyi (v2)

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

::: layout/style/nsCSSPropList.h
@@ +3332,5 @@
> +    -webkit-text-fill-color,
> +    _webkit_text_fill_color,
> +    WebkitTextFillColor,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE,

I agree that we need CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED here.

::: layout/style/test/property_database.js
@@ +7007,5 @@
> +    domProp: "webkitTextFillColor",
> +    inherited: true,
> +    type: CSS_TYPE_LONGHAND,
> +    prerequisites: { "color": "black" },
> +    initial_values: [ "currentColor", "black", "rgb(0,0,0)" ],

Probably add #000 and #000000 here.
Attachment #8730555 - Flags: feedback?(quanxunzhen) → feedback+
Comment on attachment 8730599 [details] [diff] [review]
Part2: implement -webkit-text-fill-color rendering (v1)

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

If it works, it works.
Attachment #8730599 - Flags: feedback?(quanxunzhen) → feedback+
Comment on attachment 8730599 [details] [diff] [review]
Part2: implement -webkit-text-fill-color rendering (v1)

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

::: layout/generic/nsTextFrame.cpp
@@ +3607,5 @@
> +  if (textStyle->mTextFillColorForeground) {
> +    return nsLayoutUtils::GetColor(mFrame, eCSSProperty_color);
> +  } else {
> +    return nsLayoutUtils::GetColor(mFrame, eCSSProperty__webkit_text_fill_color);
> +  }

This can probably be simplifed to something like:
> nsCSSProperty property = mFrame->StyleText()->mTextFillColorForeground ?
>   eCSSProperty_color : eCSSProperty__webkit_text_fill_color;
> return nsLayoutUtils::GetColor(mFrame, property);
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #18)
> Comment on attachment 8730599 [details] [diff] [review]
> Part2: implement -webkit-text-fill-color rendering (v1)
> 
> Review of attachment 8730599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrame.cpp
> @@ +3607,5 @@
> > +  if (textStyle->mTextFillColorForeground) {
> > +    return nsLayoutUtils::GetColor(mFrame, eCSSProperty_color);
> > +  } else {
> > +    return nsLayoutUtils::GetColor(mFrame, eCSSProperty__webkit_text_fill_color);
> > +  }
> 
> This can probably be simplifed to something like:
> > nsCSSProperty property = mFrame->StyleText()->mTextFillColorForeground ?
> >   eCSSProperty_color : eCSSProperty__webkit_text_fill_color;
> > return nsLayoutUtils::GetColor(mFrame, property);

Xidorn and TY, thank you for the feedback. :)
I'll address all your comments before sending review requests.
Depends on: 760345
Attachment #8730555 - Attachment is obsolete: true
Address reviewer's comments.
Attachment #8731103 - Flags: review?(dbaron)
Attachment #8731103 - Flags: feedback+
Attachment #8730599 - Attachment is obsolete: true
Attachment #8731103 - Flags: review?(dbaron) → review?(jfkthame)
Attachment #8731101 - Flags: review?(dbaron) → review?(jfkthame)
Attachment #8731098 - Flags: review?(dbaron) → review?(cam)
Comment on attachment 8731101 [details] [diff] [review]
Part3: reftests for -webkit-text-fill-color (v1)

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

One note: in general, green means pass, red means fail. Could you follow this convention?

Also I think it makes sense to just add these tests as web-platform-tests directly instead of our internal reftest. You can read https://hg.mozilla.org/mozilla-central/file/tip/testing/web-platform/README.md for more information.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #23)
> Comment on attachment 8731101 [details] [diff] [review]
> Part3: reftests for -webkit-text-fill-color (v1)
> 
> Review of attachment 8731101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One note: in general, green means pass, red means fail. Could you follow
> this convention?

Sure.

> 
> Also I think it makes sense to just add these tests as web-platform-tests
> directly instead of our internal reftest. You can read
> https://hg.mozilla.org/mozilla-central/file/tip/testing/web-platform/README.
> md for more information.

Cool. Thank you.
Comment on attachment 8731101 [details] [diff] [review]
Part3: reftests for -webkit-text-fill-color (v1)

Remove review request for now due to feedback from Comment 23.
Attachment #8731101 - Flags: review?(jfkthame)
Comment on attachment 8731098 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color property (v3,carry f+:Xidorn)

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

r=me with the following changes.

::: layout/style/nsCSSPropList.h
@@ +3333,5 @@
> +    _webkit_text_fill_color,
> +    WebkitTextFillColor,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED,

I think we should include CSS_PROPERTY_APPLIES_TO_PLACEHOLDER in here.  Although it's unlikely someone will apply -webkit-text-fill-color to an input::placeholder, it's consistent with the other set of properties we allow there to allow this one too.

::: layout/style/nsStyleContext.cpp
@@ +1073,5 @@
>          change = true;
>        }
> +      if (thisVisText->mWebkitTextFillColorForeground !=
> +          otherVisText->mWebkitTextFillColorForeground ||
> +          thisVisText->mWebkitTextFillColor != otherVisText->mWebkitTextFillColor) {

I think it would be fine to just combine it with the |if| statement above.

::: layout/style/nsStyleStruct.cpp
@@ +3764,5 @@
>  
> +  if (mWebkitTextFillColorForeground != aOther.mWebkitTextFillColorForeground ||
> +      mWebkitTextFillColor != aOther.mWebkitTextFillColor) {
> +    return nsChangeHint_SchedulePaint |
> +           nsChangeHint_RepaintFrame;

This block needs to come before the one where we might return nsChangeHint_NeutralChange.  (Returning nsChangeHint_NeutralChange means that we have some difference in style data stored in the struct, but that no processing is required for it, so if we compared mTextEmphasisPosition and returned nsChangeHint_NeutralChange we wouldn't repaint if we had a change in mWebkitTextFillColor.)

You could just combine it into the mTextEmphasis/Foreground check just above.
Attachment #8731098 - Flags: review?(cam) → review+
Comment on attachment 8731103 [details] [diff] [review]
Part2: implement -webkit-text-fill-color rendering (v2,carry f+:Xidorn)

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

::: layout/generic/nsTextFrame.cpp
@@ +3604,5 @@
>      }
>    }
> +
> +  nsCSSProperty property = mFrame->StyleText()->mWebkitTextFillColorForeground ?
> +    eCSSProperty_color : eCSSProperty__webkit_text_fill_color;

The style guidelines say you should wrap the line *before* the operator.[1] Given the length here, I'd probably break it over 3 lines, as either:

  nsCSSProperty property = mFrame->StyleText()->mWebkitTextFillColorForeground
                           ? eCSSProperty_color
                           : eCSSProperty__webkit_text_fill_color;

or:

  nsCSSProperty property =
    mFrame->StyleText()->mWebkitTextFillColorForeground
    ? eCSSProperty_color : eCSSProperty__webkit_text_fill_color;

whichever you think is more readable.


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
Attachment #8731103 - Flags: review?(jfkthame) → review+
Comment on attachment 8731098 [details] [diff] [review]
Part1: parse and compute -webkit-text-fill-color property (v3,carry f+:Xidorn)

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

::: layout/style/nsComputedDOMStylePropertyList.h
@@ +298,5 @@
>  COMPUTED_STYLE_PROP(_moz_window_shadow,            WindowShadow)
>  
> +/* ******************************* *\
> + * Implementations of -webkit- styles *
> +\* ******************************* */

Nit: you might want to add three more * so that the last * is aligned.
Add this test into web-platform-tests.

Since -webkit-text-fill-color is on compat spec, we might want this
feature to be tested on different browsers.
Attachment #8731098 - Attachment is obsolete: true
Attachment #8731103 - Attachment is obsolete: true
Attachment #8731978 - Attachment is obsolete: true
Add this test into web-platform-tests.

Since -webkit-text-fill-color is on compat spec, we might want this
feature to be tested on different browsers.
Attachment #8731101 - Attachment is obsolete: true
Attachment #8731979 - Attachment is obsolete: true
Attachment #8731981 - Attachment is obsolete: true
Attachment #8731982 - Attachment is obsolete: true
Attachment #8731992 - Flags: review?(jfkthame)
Attachment #8731994 - Flags: review?(bzbarsky)
Attachment #8731995 - Flags: review?(bzbarsky)
Comment on attachment 8731994 [details] [diff] [review]
Part4.1: replace windows-style line endings with unix-style line endings.

A diff -w in addition to the normal diff would sure have been nice here, or some note about it being empty.
Attachment #8731994 - Flags: review?(bzbarsky) → review+
Comment on attachment 8731995 [details] [diff] [review]
Part4.2: add compatible webkit prefixed properties in CSS properties ordering check test.

r=me
Attachment #8731995 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8731994 [details] [diff] [review]
> Part4.1: replace windows-style line endings with unix-style line endings.
> 
> A diff -w in addition to the normal diff would sure have been nice here, or
> some note about it being empty.

Yeah, I should've at least left some note (maybe in the commit message as well). I'll definitely do that next time. Thank you for the review.
Attachment #8731994 - Attachment is obsolete: true
Attachment #8731985 - Attachment is obsolete: true
try looks positive: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e6007040077&selectedJob=18327144

except for OS X 10.10 debug web-platform-tests W5, which is a known issue (see Bug 1222277)


Let's wait for test case review.
Comment on attachment 8731992 [details] [diff] [review]
Part3: reftests for -webkit-text-fill-color (v2)

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

This looks fine, but how about adding a third case to test that an inherited -webkit-text-fill-color still wins over a subsequently-specified color?

If I understand correctly, something like

  <div style="color: red; -webkit-text-fill-color: green;">These texts <span style="color: red">should be green</span></div>

should still be completely green, right?
(In reply to Jonathan Kew (:jfkthame) from comment #44)
> Comment on attachment 8731992 [details] [diff] [review]
> Part3: reftests for -webkit-text-fill-color (v2)
> 
> Review of attachment 8731992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but how about adding a third case to test that an inherited
> -webkit-text-fill-color still wins over a subsequently-specified color?
> 
> If I understand correctly, something like
> 
>   <div style="color: red; -webkit-text-fill-color: green;">These texts <span
> style="color: red">should be green</span></div>
> 
> should still be completely green, right?

Yeah, you're right. This is the test we should add in this bug. Thank you for the feedback. I'm updating the patch and would send review request again then.
Add this test into web-platform-tests.

Since -webkit-text-fill-color is on compat spec, we might want this
feature to be tested on different browsers.
Attachment #8731992 - Attachment is obsolete: true
Attachment #8731992 - Flags: review?(jfkthame)
Comment on attachment 8732770 [details] [diff] [review]
Part3: reftests for -webkit-text-fill-color (v3)

Add one more test to test that an inherited -webkit-text-fill-color still wins over a subsequently-specified color.
Attachment #8732770 - Flags: review?(jfkthame)
Comment on attachment 8732770 [details] [diff] [review]
Part3: reftests for -webkit-text-fill-color (v3)

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

Looks good, thanks.
Attachment #8732770 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #48)
> Comment on attachment 8732770 [details] [diff] [review]
> Part3: reftests for -webkit-text-fill-color (v3)
> 
> Review of attachment 8732770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks.

Thank you for the review. :)
update MANIFEST.json for web-platform-tests.
Attachment #8732817 - Flags: review+
Attachment #8732770 - Attachment is obsolete: true
jgraham, is it normal to have so many unrelated items in manifest when updating it? Are we supposed to update manifest file every time for new tests?
Flags: needinfo?(james)
Nope, that looks totally wrong, you should just see changes related to tests that you added (although it doesn't *know* which tests you added, it just looks at all the files in the tests directory on disk, so therefore if someone else added some tests but forgot to update the manifest you would also see those changes. But this doesn't look like that).

When I run the manifest update against the tip of m-i all I see is the removal of some local-changes that are now in the main data. So I don't know quite how you managed to generate such a huge diff. Do you have unrelated changes in your working directory?
Flags: needinfo?(james)
Attachment #8732817 - Attachment is obsolete: true
The tests are added to both reftest and reftest_nodes. I wonder whether it is expected. It seems to me the last time I added new tests, this didn't happen. jgraham, thoughts?

I guess this is probably the root reason of the massive addition of unrelated items.
Flags: needinfo?(james)
After a little investigation, I found 2 problems:

1. We use reftest_nodes.item.references.ref_url as a set index in [1], but use reftest_nodes.item.url in [2] as a condition. This disagreement may cause the condition never work under any circumstance I guess. I get that we might want to eliminate repeated reftests. But, I'm not sure which index we should use since I'm not sure what purpose of this elimination.

2. It seems that self.update_reftests() in [3] is a redundant step since local changes have been extended in [4] already. Not very sure about this...


jgraham, could you enlighten me on these 2 as well? Maybe they are related to the reason why so many unrelated items are generated in manifest when updating it.


[1] https://dxr.mozilla.org/mozilla-central/rev/f14898695ee0dd14615914f3e1401f17df57fdd7/testing/web-platform/tests/tools/manifest/manifest.py#186
[2] https://dxr.mozilla.org/mozilla-central/rev/f14898695ee0dd14615914f3e1401f17df57fdd7/testing/web-platform/tests/tools/manifest/manifest.py#197
[3] https://dxr.mozilla.org/mozilla-central/rev/f14898695ee0dd14615914f3e1401f17df57fdd7/testing/web-platform/tests/tools/manifest/manifest.py#166
[4] https://dxr.mozilla.org/mozilla-central/rev/f14898695ee0dd14615914f3e1401f17df57fdd7/testing/web-platform/tests/tools/manifest/manifest.py#158
Adding to both reftest and reftest_nodes is expected. Removing dom/collections/HTMLCollection-supported-property-indices.html and fetch/api/request/request-cache.html, not so much.
Adding to reftest nodes is expected. I *think* without looking at the code, that the huge diff is just the sort order not being fixed. The two tests Ms2ger mentioned that were "removed" were, I think, bugs in the checked-in manifest; they are removed from local_changes but not the upstream section.

Apologies for all the confusion, I think you are the first person to land a new reftest direct into m-i.
Flags: needinfo?(james)
Attachment #8733195 - Attachment is obsolete: true
After running --manifest-update in a clean source tree, two items are removed.
So, I'm separating this commit from the original test patch.

Hi jgraham, could you help review this?
Attachment #8733691 - Flags: review?(james)
Hi jgraham, if you think this is just a bug and this patch should never be landed, then I can skip it in this bug.
Attachment #8733691 - Flags: review?(james) → review+
No longer depends on: 759568
Target Milestone: --- → mozilla48
Looks like MANIFEST.json had been updated in Bug 1258391, so I'll skip part3.1 in this bug.
Comment on attachment 8733691 [details] [diff] [review]
Part3.1: update manifest.json before adding web-platform-tests.

obsolete this part due to Comment 62.
Attachment #8733691 - Attachment is obsolete: true
FYI, the followup patch landed in bug 1216001 today could affect this property.

If a page has
> color: transparent;
> -webkit-text-fill-color: black;
the text may not be displayed properly.

This is actually a rare case, but the fix should be simple, so you probably want to land a followup bug as well.
Flags: needinfo?(jeremychen)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #66)
> FYI, the followup patch landed in bug 1216001 today could affect this
> property.
> 
> If a page has
> > color: transparent;
> > -webkit-text-fill-color: black;
> the text may not be displayed properly.
> 
> This is actually a rare case, but the fix should be simple, so you probably
> want to land a followup bug as well.

Thank you for noticing me. I've filed Bug 1261568 for this.
Flags: needinfo?(jeremychen)
Depends on: 1261578
No longer depends on: 760345
(In reply to Sebastian Zartner [:sebo] from comment #68)
> Documented at
> https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-text-fill-color and
> https://developer.mozilla.org/en-US/Firefox/Releases/48#CSS.

Thanks! One nit, though -- this feature (like all webkit-prefixed CSS features) is actually behind an about:config pref (layout.css.prefixes.webkit), and it's *disabled* by default in version 48 release.


(Technically we had it enabled as a trial during the early-beta period, via bug 1277092, but we're past that now, so it's correct at this point to just say it's preffed off in 48.)

sebo, would you mind updating the documentation (on that Releases/48 page as well as the compat tables on the per-feature pages) to reflect that?
Flags: needinfo?(sebastianzartner)
(In reply to Daniel Holbert [:dholbert] from comment #69)
> Thanks! One nit, though -- this feature (like all webkit-prefixed CSS
> features) is actually behind an about:config pref
> (layout.css.prefixes.webkit), and it's *disabled* by default in version 48
> release.

(It'll be enabled by default in Firefox 49, via bug 1259345.)
I've added a note for the pref to the release page and a compatibility note to the feature page for this one property. I still need to adjust the release and feature pages in regard of their implementation and the pref change, but I'll do that in bug 1259345 and the related implementation bugs.

Sebastian
Flags: needinfo?(sebastianzartner)
Excellent, thanks so much!

Updated

3 years ago
Depends on: 1313757
You need to log in before you can comment on or make changes to this bug.