Closed Bug 1261578 Opened 8 years ago Closed 8 years ago

-webkit-text-fill-color should take effect while rendering texts under selection

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: chenpighead, Assigned: u459114)

References

Details

Attachments

(8 files, 12 obsolete files)

58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
378 bytes, text/html
Details
Attached file testcase 1 (obsolete) —
Since we've shipped -webkit-text-fill-color in Bug 1247777, selected texts should be rendered w/ -webkit-text-fill-color instead of color.
Blocks: 1247777
Why should the color of selected text should be rendered in text-fill-color? It doesn't seem we do that for property color. Chrome doesn't do that for text-fill-color either.
It may make sense to say -webkit-text-fill-color should take effect like color for ::-moz-selection, though.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> It may make sense to say -webkit-text-fill-color should take effect like
> color for ::-moz-selection, though.

I guess you're just saying that I should re-edit the bug summary, right?
Thank you for the noticing and prevent me from resolving this in a wrong way.
Assignee: nobody → jeremychen
Summary: Selected texts is not rendered w/ -webkit-text-fill-color → -webkit-text-fill-color should take effect while rendering texts under selection
Well, I meant it should probably take effect when it is specified in ::-moz-selection pseudo-element. I don't think this is an important issue, though.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Well, I meant it should probably take effect when it is specified in
> ::-moz-selection pseudo-element. I don't think this is an important issue,
> though.

I didn't have time to dig deeper yet. I'm guessing that somewhere in nsTextFrame::GetSelectionTextColors() is not taking -webkit-text-fill-color into consideration. This should fix the problem incluing specifying
webkit-text-fill-color in ::-moz-selection pseudo-element as well, I guess.

[1] https://dxr.mozilla.org/mozilla-central/rev/17a0ded9bb99c05c25729c306b91771483109067/layout/generic/nsTextFrame.cpp#5650
Except selection, all the rendering parts that -webkit-text-fill-color take effect should be patched as well. I plan to cover those rendering parts in this bug as well. Will start work on this tomorrow.

Here's a list of affected rendering parts I found so far:
1. selection text color
2. <math> MathML element color
3. nsDisplayTextOverflowMarker::Paint for TextOverflow
4. nsTextBoxFrame for xul

Not sure if item 4 should be patched or not...
Status: NEW → ASSIGNED
Attached file testcase2 (math element) (obsolete) —
Attached file testcase 1b (::-moz-selection) (obsolete) —
Looks like people don't use --manifest-update very often, so I have to update this from time to time. :(
Attached patch part1.2: fix ini files. (obsolete) — Splinter Review
Make it align w/ test file name.
Attached patch part1.3: add reftest. (obsolete) — Splinter Review
Test whether -webkit-text-fill-color takes effect for selected text,
text-decoration, text-overflow, and MathML.
Attachment #8740359 - Attachment description: part1.1: update manifest before adding test. r?jgraham → part1.1: update manifest before adding test.
Attachment #8740360 - Attachment description: part1.2: fix ini files. r?jfkthame → part1.2: fix ini files.
Expect fails on following 5 files:
webkit-text-fill-color-property-002.html
webkit-text-fill-color-property-003.html
webkit-text-fill-color-property-004.html
webkit-text-fill-color-property-005.html
webkit-text-fill-color-property-006.html

The fails have been already verified locally.
Let's see how it goes on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> Expect fails on following 5 files:
> webkit-text-fill-color-property-002.html
> webkit-text-fill-color-property-003.html
> webkit-text-fill-color-property-004.html
> webkit-text-fill-color-property-005.html
> webkit-text-fill-color-property-006.html
> 
> The fails have been already verified locally.
> Let's see how it goes on try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257

Retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=19335169

No idea why 002 could pass...
Attachment #8740359 - Attachment is obsolete: true
Attachment #8740360 - Attachment is obsolete: true
Attachment #8740361 - Attachment is obsolete: true
Attached patch part1.2: add reftest (v2) (obsolete) — Splinter Review
Split this from adding test.
Make rebase for manifest.json a lot easier.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> > Expect fails on following 5 files:
> > webkit-text-fill-color-property-002.html
> > webkit-text-fill-color-property-003.html
> > webkit-text-fill-color-property-004.html
> > webkit-text-fill-color-property-005.html
> > webkit-text-fill-color-property-006.html
> > 
> > The fails have been already verified locally.
> > Let's see how it goes on try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257
> 
> Retry:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=1
> 9335169
> 
> No idea why 002 could pass...

I guess 002 could really pass on Linux, however, it shall not pass on MacOS I think.
After discussing with CJ and Astley offline, CJ could help take care of this bug from here. So, I could get back to Bug 1248708.
Assignee: jeremychen → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(cku)
Assignee: nobody → cku
Flags: needinfo?(cku)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #19)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> > > Expect fails on following 5 files:
> > > webkit-text-fill-color-property-002.html
> > > webkit-text-fill-color-property-003.html
> > > webkit-text-fill-color-property-004.html
> > > webkit-text-fill-color-property-005.html
> > > webkit-text-fill-color-property-006.html
> > > 
> > > The fails have been already verified locally.
> > > Let's see how it goes on try:
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257
> > 
> > Retry:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=1
> > 9335169
> > 
> > No idea why 002 could pass...
> 
> I guess 002 could really pass on Linux, however, it shall not pass on MacOS
> I think.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97119dc845b
Assumption proved!!

Accordingly, we may need to try MacOS platform to guarantee the patch do work.
Attachment #8740800 - Attachment is obsolete: true
Attachment #8737494 - Attachment is obsolete: true
Attachment #8738998 - Attachment is obsolete: true
Attachment #8739865 - Attachment is obsolete: true
Attachment #8739940 - Attachment is obsolete: true
Attachment #8739044 - Attachment is obsolete: true
Review commit: https://reviewboard.mozilla.org/r/46127/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46127/
Attachment #8741028 - Flags: review?(jfkthame)
Attachment #8741029 - Flags: review?(jfkthame)
Attachment #8740962 - Flags: review?(jfkthame)
Attachment #8740963 - Flags: review?(jfkthame)
Attachment #8741010 - Flags: review?(jfkthame)
Attachment #8740781 - Attachment is obsolete: true
Blocks: 1259345
Comment on attachment 8740962 [details]
MozReview Request: Bug 1261578 - Part 1. Correct text color in selection range;

https://reviewboard.mozilla.org/r/46101/#review42625

::: layout/base/nsLayoutUtils.h:1537
(Diff revision 3)
> +  // Get a suitable text color property from aCtx.
> +  static nsCSSProperty GetTextColorProp(nsStyleContext *const aCtx);

IMO, a more natural home for this would be as part of nsStyleContext; it doesn't depend on anything else besides the context itself, so it can be a simple inline method in nsStyleContext.h such as:

    nsCSSProperty TextColorProperty() {
      return StyleText()->mWebkitTextFillColorForeground
             ? eCSSProperty_color : eCSSProperty__webkit_text_fill_color;
    }

(See also the existing GetTextFillColor() method; this would logically go alongside it.)

Obviously, this will have a knock-on effect on the callsites...
Attachment #8740962 - Flags: review?(jfkthame)
Comment on attachment 8740963 [details]
MozReview Request: Bug 1261578 - Part 2. Correct text decoration color;

https://reviewboard.mozilla.org/r/46103/#review42635

This basically looks like it should be fine; just a few nits I'd like to get cleaned up, please.

::: layout/generic/nsTextFrame.cpp:410
(Diff revision 3)
>      free(userData);
>    }
>  }
>  
> +static nsCSSProperty
> +ChooseDecorationTextColorProp(nsStyleContext *const aCtx)

We don't normally use `const` on pointers like this; there's no real benefit here, AFAICS, it just adds clutter. (If we could declare the `nsStyleContext` itself as being `const`, that'd be different -- but we can't do that because accessors like StyleText() are not `const` methods.)

::: layout/generic/nsTextFrame.cpp:412
(Diff revision 3)
> +  const nsStyleTextReset*const styleTextReset = aCtx->StyleTextReset();
> +  const nsStyleText*const styleText = aCtx->StyleText();

Likewise, drop the second `const` from each of these.

::: layout/generic/nsTextFrame.cpp:413
(Diff revision 3)
>  
> +static nsCSSProperty
> +ChooseDecorationTextColorProp(nsStyleContext *const aCtx)
> +{
> +  const nsStyleTextReset*const styleTextReset = aCtx->StyleTextReset();
> +  const nsStyleText*const styleText = aCtx->StyleText();

Also, don't look up `StyleText()` here, as it may not be needed, depending what gets returned in `foreground`. Just use `StyleText()` directly in the `return` expression below, as there's only a single use of it.

And for that matter, there's no real need for the `styleTextReset` variable either, as it just has the one simple usage; might as well write `StyleTextReset()->GetDecorationColor(....)` directly there.

::: layout/generic/nsTextFrame.cpp:419
(Diff revision 3)
> +  return (foreground && !styleText->mWebkitTextFillColorForeground) ?
> +           eCSSProperty__webkit_text_fill_color :
> +           eCSSProperty_text_decoration_color;

The style guide wants us to break lines *before* the operator, so this becomes

    return (....)
           ? eCSS....
           : eCSS...;

(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators)

::: layout/style/StyleAnimationValue.cpp:3306
(Diff revision 3)
> -            color = aStyleContext->StyleColor()->mColor;
> +            const nsStyleText *styleText = aStyleContext->StyleText();
> +            color = styleText->mWebkitTextFillColorForeground ?
> +              aStyleContext->StyleColor()->mColor :
> +              styleText->mWebkitTextFillColor;

I think this is equivalent to:

    color = aStyleContext->GetTextFillColor();
Attachment #8740963 - Flags: review?(jfkthame)
Attachment #8740782 - Attachment is obsolete: true
Attachment #8741652 - Flags: review?(jfkthame)
Attachment #8740779 - Attachment is obsolete: true
Attachment #8740963 - Flags: review?(jfkthame) → review+
Comment on attachment 8740963 [details]
MozReview Request: Bug 1261578 - Part 2. Correct text decoration color;

https://reviewboard.mozilla.org/r/46103/#review43619

::: accessible/base/TextAttrs.cpp:705
(Diff revision 4)
>    mStyle = textReset->GetDecorationStyle();
>  
>    bool isForegroundColor = false;
>    textReset->GetDecorationColor(mColor, isForegroundColor);
>    if (isForegroundColor)
> -    mColor = aFrame->StyleColor()->mColor;
> +    mColor = aFrame->StyleContext()->GetTextFillColor();

While you're here, it'd be nice to add the missing braces for the if-statement.
Comment on attachment 8740962 [details]
MozReview Request: Bug 1261578 - Part 1. Correct text color in selection range;

https://reviewboard.mozilla.org/r/46101/#review43617
Attachment #8740962 - Flags: review?(jfkthame) → review+
Comment on attachment 8741010 [details]
MozReview Request: Bug 1261578 - Part 3. Correct MathML text color;

https://reviewboard.mozilla.org/r/46117/#review43621
Attachment #8741010 - Flags: review?(jfkthame) → review+
Attachment #8741028 - Flags: review?(jfkthame) → review+
Comment on attachment 8741028 [details]
MozReview Request: Bug 1261578 - Part 4. Correct text overflow color;

https://reviewboard.mozilla.org/r/46127/#review43623
Comment on attachment 8741029 [details]
MozReview Request: Bug 1261578 - Part 5. web-platform-test reftest;

https://reviewboard.mozilla.org/r/46129/#review43627

Looks fine, with a couple minor fixups.

::: testing/web-platform/tests/compat/webkit-text-fill-color-property-002.html:17
(Diff revision 2)
> +  color: red;
> +  -webkit-text-fill-color: green;
> +}
> +</style>
> +<body onload="onload()">
> +<p>Pass if color of <span id="selectMe">selected text</span> is green!!!</p>

Although what this test actually does -- checking that `-webkit-text-fill-color` has the same effect as `color` in the reference -- should be fine, ISTM the phrasing of the "pass" condition here is a problem, because (depending on the platform conventions) we don't necessarily render selected text using the foreground color at all -- on Windows, we invert the text to white (discarding its specified color).

Maybe something like "Pass if color of selected text is green or inverted (depending on the platform convention), but not red." Or am I confused about what's happening here?

::: testing/web-platform/tests/compat/webkit-text-fill-color-property-006.html:24
(Diff revision 2)
> +  font-family: Ahem;
> +  font-size: 30px;
> +}
> +</style>
> +<body>
> +<p>Test passes if there is an <strong>green ellipsis</strong> after "TestChecks".</p>

s/an/a/, here and in reference
Attachment #8741029 - Flags: review?(jfkthame) → review+
Comment on attachment 8741652 [details]
MozReview Request: Bug 1261578 - Part 6. Before update web-platform-test manifest;

https://reviewboard.mozilla.org/r/46665/#review43629

::: testing/web-platform/meta/MANIFEST.json
(Diff revision 1)
> -        "path": "web-animations/animation-model/keyframes/effect-value-context.html",
> -        "url": "/web-animations/animation-model/keyframes/effect-value-context.html"
> -      },
> -      {

Shouldn't this file still be mentioned somewhere? It looks like it was out-of-place in the list, so I'd expect to see it move, but not disappear altogether.
Attachment #8741652 - Flags: review?(jfkthame)
https://reviewboard.mozilla.org/r/46665/#review43629

> Shouldn't this file still be mentioned somewhere? It looks like it was out-of-place in the list, so I'd expect to see it move, but not disappear altogether.

I follow how jeremy update manifest in Bug 1247777
1. run ./mach web-platform-tests --manifest-update without new test cases add
2. add test cases
3. run ./mach web-platform-tests --manifest-update again

The result looks like noraml after following these steps.
Review commit: https://reviewboard.mozilla.org/r/47425/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47425/
Attachment #8741652 - Attachment description: MozReview Request: Bug 1261578 - Part 6. Update web-platform-test manifest; → MozReview Request: Bug 1261578 - Part 6. Before update web-platform-test manifest;
Attachment #8741652 - Flags: review?(jfkthame)
Comment on attachment 8741652 [details]
MozReview Request: Bug 1261578 - Part 6. Before update web-platform-test manifest;

https://reviewboard.mozilla.org/r/46665/#review44105
Attachment #8741652 - Flags: review?(jfkthame) → review+
Comment on attachment 8742700 [details]
MozReview Request: Bug 1261578 - Part 7. Update web-platform-test manifest for fill-color reftest;

https://reviewboard.mozilla.org/r/47425/#review44107
Attachment #8742700 - Flags: review+
how soon... thanks Jonathan.
Nice job!! \O/
CJ, thank you for taking care of this.
It seems like this requires spec changes, in that it changes the definition of the currentcolor value for text-decoration-color from using 'color' to using '-webkit-text-fill-color'.

I'm also curious why it made that change for text-decoration but not text-emphasis.

Xidorn, would you be able to deal with sending the appropriate email to www-style to discuss these spec issues, or should I?

(This probably should have been done before it landed.)
Flags: needinfo?(bugzilla)
Also, do all of the different changes (parts 1, 2, 3, and 4) in this bug match the webkit/blink behavior?
Flags: needinfo?(cku)
I think so. I verified my changes, in Part 1~4, by test cases in part 5, and using those test cases to make sure we have the same behavior with safari & google chrome.
Flags: needinfo?(cku)
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #69)
> It seems like this requires spec changes, in that it changes the definition
> of the currentcolor value for text-decoration-color from using 'color' to
> using '-webkit-text-fill-color'.
> 
> I'm also curious why it made that change for text-decoration but not
> text-emphasis.
Attached an example with -webkit-text-fill-color + text-emphasis
1. webkit does not apply text-fill-color to emphasis.
2. blink does not support text-emphasis(unless I am wrong)
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #69)
> It seems like this requires spec changes, in that it changes the definition
> of the currentcolor value for text-decoration-color from using 'color' to
> using '-webkit-text-fill-color'.
> 
> I'm also curious why it made that change for text-decoration but not
> text-emphasis.

Leaving the needinfo? for text-emphasis.

> Xidorn, would you be able to deal with sending the appropriate email to
> www-style to discuss these spec issues, or should I?

I'll send the email to www-style.

> (This probably should have been done before it landed.)

I'd like to re-emphasize this.  We shouldn't be landing changes that require spec changes without pointing out those spec changes in the appropriate working groups.  That's something for both the patch author and reviewer to be paying attention to.
I think we should handle text-emphasis-color and text-decoration-color the same way, as they are both decoration. But I don't think we should change "text-decoration-color: currentcolor" to use the value from -webkit-text-fill-color. I've replied the post in www-style. As I said there, that could make implementing interpolation between currentcolor and non-currentcolor value unnecessarily more complicated.
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #76)
> I think we should handle text-emphasis-color and text-decoration-color the
> same way, as they are both decoration. But I don't think we should change
> "text-decoration-color: currentcolor" to use the value from
> -webkit-text-fill-color. I've replied the post in www-style. As I said

That means some of the changes in this bug need to be backed out.
Flags: needinfo?(cku)
Hmm, it looks like Safari & Chrome do something rather weird with decorations, actually: if -webkit-text-stroke-width is greater than 0px, they actually paint the decoration using the color from -webkit-text-stroke-color rather than ...fill-color. (But they don't change the width of the line to follow -webkit-text-stroke-width, so they're not entirely treating it as a "stroke".)
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #77)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #76)
> > I think we should handle text-emphasis-color and text-decoration-color the
> > same way, as they are both decoration. But I don't think we should change
> > "text-decoration-color: currentcolor" to use the value from
> > -webkit-text-fill-color. I've replied the post in www-style. As I said
> 
> That means some of the changes in this bug need to be backed out.
Here is what I am going to do
1. Back out change in Part 2.
2. "expected:FAIL" in ini files of text decoration test. I think it's better then just remove them.
3. Change string in test case. For example, change "Pass if text underline is green!!!" to "Fail if text underline is green!!!"
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku] from comment #79)
> 2. "expected:FAIL" in ini files of text decoration test. I think it's better
> then just remove them.
> 3. Change string in test case. For example, change "Pass if text underline
> is green!!!" to "Fail if text underline is green!!!"

I guess it is better just remove that test directly if that was written by you. jgraham, what do you think?
Flags: needinfo?(james)
bug 1266948 filed
Depends on: 1266948
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #80)
> (In reply to C.J. Ku[:cjku] from comment #79)
> > 2. "expected:FAIL" in ini files of text decoration test. I think it's better
> > then just remove them.
> > 3. Change string in test case. For example, change "Pass if text underline
> > is green!!!" to "Fail if text underline is green!!!"
> 
> I guess it is better just remove that test directly if that was written by
> you. jgraham, what do you think?

If the tests are now believed to be wrong i.e. not testing the behaviour in the spec, remove them. If they are right per-spec and our implementation is temporarily wrong in a way that we expect to fix in the future, mark them expected:FAIL. If they are wrong per spec and the correct-per-spec behaviour is not tested elsewhere, fix them to test the correct behaviour. If the spec is wrong and we don't intend to implement it things are a little more complex, but generally editing the tests to match the expected spec is fine if the change is uncontroversial (e.g. just matches the behaviour of existing implementations).
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #82)
> If they are wrong per spec and the correct-per-spec behaviour
> is not tested elsewhere, fix them to test the correct behaviour.

This case. CJ has filed bug 1266948 to change the behavior and the test.
You need to log in before you can comment on or make changes to this bug.