1261568 - -webkit-text-fill-color is not working if setting color to be transparent

Status: RESOLVED FIXED

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

Description

[Jeremy Chen ([e-mail])]

Attachment: testcase 1

Looks like Bug 1216001 causing this.

STR:

Open a web page w/ text's style as follows:
{ color: transparent; -webkit-text-fill-color: black;}

Expect:
Texts are showed w/ black color

Actual:
Texts are not showed (transparent color)

Comment 1

[Jeremy Chen ([e-mail])]

I also found that texts under selection would be showed w/ color instead of -webkit-text-fill-color.

xidorn, do you think this is the same problem in this bug? Or should I file another one?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

That sounds like a different problem. You may want to check nsTextFrame::GetSelectionTextColors() and its callsites, which may relate to that.

Comment 3

[Jeremy Chen ([e-mail])]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> That sounds like a different problem. You may want to check
> nsTextFrame::GetSelectionTextColors() and its callsites, which may relate to
> that.

Thanks. File Bug 1261578 for that.

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: part1: take -webkit-text-fill-color into consideration while building displaylist for text frame.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8737501 [details] [diff] [review]
part1: take -webkit-text-fill-color into consideration while building displaylist for text frame.

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

::: layout/generic/nsTextFrame.cpp
@@ +4828,3 @@
>    Maybe<bool> isSelected;
>    if (((GetStateBits() & TEXT_NO_RENDERED_GLYPHS) ||
> +       (isTextTransparent && !StyleText()->HasTextShadow())) &&

There are two StyleText() here. You may want to use a variable to store it rather than calling it twice.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8737501 [details] [diff] [review]
part1: take -webkit-text-fill-color into consideration while building displaylist for text frame.

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

Discussed with Jeremy in IRC.

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: part1: take -webkit-text-fill-color into consideration while building displaylist for text frame. (v2)

Address xidorn's comments.

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: part2.1: update manifest before adding test.

This is just for reviewer's convenience.
All part2 will be landed as a single commit after review process.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: part2.2: add reftest.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8737510 [details] [diff] [review]
part1: take -webkit-text-fill-color into consideration while building displaylist for text frame. (v2)

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

::: layout/generic/nsTextFrame.cpp
@@ +4825,5 @@
>  
> +  const nsStyleText* textStyle = StyleText();
> +  bool isTextTransparent = NS_GET_A(textStyle->mWebkitTextFillColorForeground
> +                                    ? StyleColor()->mColor
> +                                    : textStyle->mWebkitTextFillColor) == 0;

How about adding a helper method to nsStyleContext.h, something like this:

  /**
   * Get a reference to the color that should be used to fill text: either
   * the current foreground color, or a separately-specified text fill color.
   */
  const nscolor& GetTextFillColor() {
    const nsStyleText* textStyle = StyleText();
    return textStyle->mWebkitTextFillColorForeground
           ? StyleColor()->mColor : textStyle->mWebkitTextFillColor;
  }


Then the code here would simplify to:

  bool isTextTransparent =
    NS_GET_A(StyleContext()->GetTextFillColor()) == 0;

There must be other places we'd want the same thing, no? Looks like at least nsComputedDOMStyle::DoGetWebkitTextFillColor and StyleAnimationValue::ExtractComputedValue have versions of the same conditional expression already.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8737512 [details] [diff] [review]
part2.1: update manifest before adding test.

I'd assume this is fine, but I don't actually know anything about this test runner & manifests. I wonder why they're currently out of order? Accident?

jgraham, I think you know about these things, yes? Is the ordering at all important here?

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8737512 [details] [diff] [review]
part2.1: update manifest before adding test.

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

The order is not important at all, but the autogeneration script keeps things in alphabetical order to minimise diffs. However, it seems that sometimes patches get checked in that have edited this file by hand and do not preserve order.

Filed bug 1261788 on that.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Comment on attachment 8737510 [details] [diff] [review]
> part1: take -webkit-text-fill-color into consideration while building
> displaylist for text frame. (v2)
> 
> Review of attachment 8737510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrame.cpp
> @@ +4825,5 @@
> >  
> > +  const nsStyleText* textStyle = StyleText();
> > +  bool isTextTransparent = NS_GET_A(textStyle->mWebkitTextFillColorForeground
> > +                                    ? StyleColor()->mColor
> > +                                    : textStyle->mWebkitTextFillColor) == 0;
> 
> How about adding a helper method to nsStyleContext.h, something like this:
> 
>   /**
>    * Get a reference to the color that should be used to fill text: either
>    * the current foreground color, or a separately-specified text fill color.
>    */
>   const nscolor& GetTextFillColor() {
>     const nsStyleText* textStyle = StyleText();
>     return textStyle->mWebkitTextFillColorForeground
>            ? StyleColor()->mColor : textStyle->mWebkitTextFillColor;
>   }

It doesn't seem to make much sense here to return a const reference. nscolor is currently just an uint32_t, which is cheap to be passed by value.

Comment 14

[Jonathan Kew [:jfkthame]]

True -- I guess I was imagining it as a heavier struct, but for a uint32, returning by value is simplest and safest.

Comment 15

[Jeremy Chen ([e-mail])]

Attachment: part1: take -webkit-text-fill-color into consideration while building displaylist for text frame. (v3)

Add a helper method to get text color in nsStyleContext.h

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8737806 [details] [diff] [review]
part1: take -webkit-text-fill-color into consideration while building displaylist for text frame. (v3)

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

::: layout/generic/nsTextFrame.cpp
@@ +3599,5 @@
>          return NS_RGBA(0, 0, 0, 255);
>      }
>    }
>  
> +  return mFrame->StyleContext()->GetTextFillColor();

This isn't equivalent to the existing code: nsLayoutUtils::GetColor does more than simply reading the field from the style struct.

Comment 17

[Jeremy Chen ([e-mail])]

Attachment: part1: take -webkit-text-fill-color into consideration while building displaylist for text frame. (v4)

Address reviewer's comment.

Comment 18

[Jeremy Chen ([e-mail])]

jfkthame, thank you for the review. :)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/066926557b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a00403cb58f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d928b340e5

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/066926557b77
https://hg.mozilla.org/mozilla-central/rev/8a00403cb58f
https://hg.mozilla.org/mozilla-central/rev/d7d928b340e5