Implement -webkit-background-clip: text (as background-clip: text)

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: jaws, Assigned: u459114)

Tracking

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

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

()

Attachments

(8 attachments)

Webkit has two CSS properties (-webkit-background-clip:text and -webkit-text-fill-color) that, when combined, allow text to use the element's background as the font color.

For example, this will produce text with a vertical linear gradient color:
> h1 {
>   background: -webkit-gradient(linear, left top, left bottom, from(#eee), to(#333));
>   -webkit-background-clip: text;
>   -webkit-text-fill-color: transparent;
> }

This would be useful for fading out text in our tabstrip as well as our location bar.

I was unable to find a standards document from the Webkit team though.

Comment 2

7 years ago
This really should be implemented. It allows for really nice font effects such as inset fonts, or animated font colors/images, the list goes on.
Blocks: 815442
Duplicate of this bug: 858981
The front-end team could use this bug or bug 877294 to implement the tab label fadeout.
Whiteboard: [Australis:P?][Australis:M-]
Whiteboard: [Australis:P?][Australis:M-]
Whiteboard: p=0

Comment 5

6 years ago
There is no spec for background-clip:text (CSS3-Background[1] does not cover this) nor text-fill-color, AFAICS, and I assume we need a spec (draft) first. Also, is there a reason that the effect explained in comment 0 would not work with color:transparent or color:rgba(*,*,*,0)? Is transparent text being optimized away? If not, we don't need text-fill-color.


[1] http://www.w3.org/TR/css3-background/#the-background-clip
Summary: Implement -webkit-background-clip:text and -webkit-text-fill-color → Implement background-clip:text and -webkit-text-fill-color
(In reply to Florian Bender from comment #5)
> Also, is there a reason that the effect explained in comment 0 would
> not work with color:transparent or color:rgba(*,*,*,0)? Is transparent text
> being optimized away? If not, we don't need text-fill-color.

The reason that `-webkit-text-fill-color` was used instead of `color` was for backward-compat with browsers that don't support background-clip:text. -webkit-text-fill-color will override `color` if it is present, this way sites don't show transparent text when there is no background-clip:text.

Comment 7

5 years ago
I see. But @supports should be able to ensure a sensible fallback (or progressive enhancement), so I (tentatively) removed the request for text-fill-color support.
Summary: Implement background-clip:text and -webkit-text-fill-color → Implement background-clip: text;
No longer blocks: fxdesktopbacklog
Whiteboard: p=0

Comment 8

5 years ago
I made a draft for supporting "textures", it also includes a "font-fill" property that relates to this bug : https://github.com/nt1m/CSS-Texture-Specification/wiki/New-properties
FWIW, MS Edge supports this feature, (both unprefixed & webkit-prefixed, e.g. they support both "background-clip: text" and "-webkit-background-clip: text").
Here's a site that depends on this: http://alva.link/
Blocks: 1238527
Blocks: 1238550
Depends on: 1243885
As noted in bug 1238550 comment 5: since "text" is not a valid value for the standard "background-clip" property (and is frowned upon by the CSSWG per reply to the thread in comment 1), I think we should only support the "text" value for the alias property (-webkit-background-clip) -- not for the standard unprefixed property.

The parsing codepath for the alias and the unprefixed property are shared, but we can distinguish between them as described in bug 1231682 comment 1.
Blocks: 1170774, 1243654
Summary: Implement background-clip: text; → Implement -webkit-background-clip: text;
No longer depends on: 1243885
Duplicate of this bug: 1238550
No longer blocks: 1238550
Depends on: 1225661
Blocks: 1225661
No longer depends on: 1225661

Comment 15

3 years ago
We're tracking this in the compat spec:

https://github.com/whatwg/compat/issues/26
Blocks: 1247777
Depends on: 1248644
(fixing dependency for Bug 1248644)
Blocks: 1248644
No longer depends on: 1248644
See Also: → 1248708
As a short-term hackaround, I think we could avoid shipping most of this bug's dependency-bugs as regressions if we fixed bug 1248785.
Assignee: nobody → cku
Status: NEW → ASSIGNED
Assignee

Comment 19

3 years ago
Working on this from today.
Assignee

Comment 24

3 years ago
Part 2 is not complete yet, but it reveals my approach of background-clip-text rendering.
dbaron mentioned that clip-path(instead of mask) might be another approach. Need to check with gfx engineers later.
https://reviewboard.mozilla.org/r/39487/#review36149

::: layout/generic/nsTextFrame.h:822
(Diff revision 1)
> +  bool mForce;

I suppose this unlikely works. I believe we pretty care about the size of nsTextFrame, and want to avoid bloating it at all cost.

If this is used only in rare case, you probably want to try frame property instead, like this one: https://dxr.mozilla.org/mozilla-central/rev/102886e9ac63b3fa33a6f1b394aea054abce2dfd/layout/base/RestyleManager.cpp#648

Since reading frame property costs one hash table lookup and one linear search, you shouldn't use it in hot path. You can use some other cheaper condition to avoid putting it on the hot path. (e.g. check the style data)

If frame property doesn't work, you probably should consider frame state bit, but I don't believe there is any slot available for text frame now.
Assignee

Comment 26

3 years ago
(In reply to C.J. Ku[:cjku] from comment #21)
> Created attachment 8729552 [details]
> MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;
> 
> Review commit: https://reviewboard.mozilla.org/r/39485/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/39485/

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #25)
> https://reviewboard.mozilla.org/r/39487/#review36149
> 
> ::: layout/generic/nsTextFrame.h:822
> (Diff revision 1)
> > +  bool mForce;
> 
> I suppose this unlikely works. I believe we pretty care about the size of
> nsTextFrame, and want to avoid bloating it at all cost.
I think I will add one more parameter in nsTextFrame::PaintText function later, instead of using this terrible hack(mForce).
For example:
nsTextFrame::PaintText(..., bool paintAsMask)

And thanks for tell me that we should not bloating the size of frame structures.
(In reply to C.J. Ku[:cjku] from comment #26)
> (In reply to C.J. Ku[:cjku] from comment #21)
> I think I will add one more parameter in nsTextFrame::PaintText function
> later, instead of using this terrible hack(mForce).
> For example:
> nsTextFrame::PaintText(..., bool paintAsMask)

That way, it'd probably better to add it in one of the parameter structs (probably PaintTextParams [1]). Bool param in param list could sometimes significantly decrease readability of callsites. (And that was one of the reasons for the refactor in bug 1251995 :)


[1] https://dxr.mozilla.org/mozilla-central/rev/102886e9ac63b3fa33a6f1b394aea054abce2dfd/layout/generic/nsTextFrame.h#394-402
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Assignee

Comment 31

3 years ago
Update clip-path-solution. Rendering result is pretty much the same with chrome.
text-shadow and decoration is not included in clip-path
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Assignee

Comment 36

3 years ago
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

Hi dbaron,
Please have a check on Part 1.
Since webkit-background-clip is an alias of background-clip, but has an extra prop value("text"), change in css parser may not trivial.
Attachment #8729552 - Flags: feedback?(dbaron)
(In reply to C.J. Ku[:cjku] from comment #36)
> Since webkit-background-clip is an alias of background-clip, but has an
> extra prop value("text"), change in css parser may not trivial.

My "part 1" patch over on bug 1231682 will soon make it quite straightforward to support an alias-specific property-value, actually.

(I might spin that part off into its own bug; but I expect it should land soon, wherever it is.)
Assignee

Comment 38

3 years ago
(In reply to Mike Taylor [:miketaylr] (PTO March 10 - 21) from comment #20)
> Spec at https://compat.spec.whatwg.org/#the-webkit-background-clip-property
> (feedback welcome, cjku!)
Hi Mike,
I had submit some issue in spec, please check when you have time.
(In reply to Daniel Holbert [:dholbert] from comment #37)
> My "part 1" patch over on bug 1231682 will soon make it quite
> straightforward to support an alias-specific property-value, actually.

...though on second thought, my patch wouldn't actually help here, because you're adding a completely new value & behavior here.  (My patch aims to let us support an alternate alias-specific value (or syntax) which we can then represent internally [& re-serialize if asked] using only standard stuff.  So, it's not applicable here, since we can't represent "-webkit-background-clip:text" using only standard stuff, and we wouldn't be able to reserialize it via e.g. getComputedStyle().backgroundClip in a way that could be fed back into style.backgroundClip.

So, probably-disregard my comment 37.
(In reply to Daniel Holbert [:dholbert] from comment #39)
> So, probably-disregard my comment 37.

...though maybe your patch has the same problems that I was worried my patch might cause. In which case, don't necessarily disregard comment 37. :)

With your patch, when an author sets "-webkit-background-clip:text" on an element, what will we report in devtools and via elem.style & elem.getComputedStyle, for the "background-clip" property?  If we report the element as having "background-clip: text", that's somewhat problematic, because we'll be reporting the element as having styling that we don't actually accept, if the author were to e.g. serialize the style & try to assign it to a new instance of the same element.

Maybe it's not too problematic, though, since we're aiming for Chrome compat, and they have this same problem at least for computed-style & for devtools (though not for elem.style).

So, unless we can figure out a sane way to address this serialization issue, you might just want to build on top of my "part 1" patch over on bug 1231682 for simplicity.
If we feel we need to support -webkit-background-clip: text, maybe we should also just be supporting it as background-clip:text?
MS Edge already does support unprefixed "background-clip:text", as noted in comment 9, BTW (presumably just for simplicity.)

There's an argument to be made that we should keep this feature behind a vendor prefix, since it's nonstandard behavior that's been frowned on by CSSWG spec editors (https://lists.w3.org/Archives/Public/www-style/2012May/1228.html ), and since Chrome/Safari-compatibility only requires that we support it with "-webkit-background-clip".

However: it's getting hard to argue that it's "nonstandard behavior", now that it'll be implemented in all modern browser engines & specced in http://compat.spec.whatwg.org/ (& perhaps eventually specced by CSSWG).  So, I tend to agree with dbaron's suggestion that we should just support it in "background-clip" -- the upside of hiding this de-facto standard CSS behind a prefix doesn't seem worth the downside of the complexity & serialization awkwardness that I hinted at above.
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

As dholbert points out, this doesn't work with serialization.

I think we should just add background-clip: text, but conditional on the webkit prefix pref.  See the way we handle values of 'display' that are pref-controlled for examples.
Attachment #8729552 - Flags: feedback?(dbaron) → feedback-
FWIW, although Chrome does not accept "text" for background-clip, parsing is the only difference between the two properties, and they are handled in the identical way afterwards. That says, if you specify "-webkit-background-clip: text", you would get "text" for the computed value of "background-clip".
(In reply to C.J. Ku[:cjku] from comment #38)
> I had submit some issue in spec, please check when you have time.

Thanks! (Digging out of a billion PTO email/notifications, responses will be slightly delayed)
As noted in posts from Jet & myself on the dev-platform "intent to ship" thread*: it might be worth putting this new "text" value behind its own about:config pref (which would be independent of webkit prefix support -- see my post for more details).

(Though I'm also not sure that a new pref would be worthwhile [except maybe from a local testing/diagnosis perspective?], since I don't forsee a world in which we'd toggle one pref for users without also toggling the other pref.)

*https://groups.google.com/d/msg/mozilla.dev.platform/eEm0CPr8lGI/s-uPwN0RCQAJ
Assignee

Comment 48

3 years ago
To use background-clip:text, we have to enable prefix.webkit, so
  prefix.webkit >= background-clip.text
But, without background-clip:text support, we can not enable prefix.webkit, so
  background-clip.text >= prefix.webkit

The result:
  background-clip.text == prefix.webkit

That's my thought.
(In reply to C.J. Ku[:cjku] from comment #48)
> To use background-clip:text, we have to enable prefix.webkit

This part isn't strictly true. It does *happen* to be true for existing usage of this feature on the web, but presumably there will be unprefixed usage going forward, now that 2 browser-engines will support it unprefixed (and Chrome will surely follow suit & unprefix, given the CSSWG reluctant blessing [if I'm interpreting the www-style post correctly).

So, I think we should just consider this bug as being about implementing this cool new feature "background-clip:text" with its own pref "layout.css.background-clip-text.enabled".

(And as it happens, we also support a -webkit prefixed alias of this property, though that alias is controlled by a different pref.  And it'll turn out that we'll get useful behavior from existing web content once both the feature pref and the webkit-alias pref are enabled -- but I don't think your code here needs to care about the webkit-alias pref.)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
No longer blocks: 1247777
Target Milestone: --- → mozilla48
Assignee

Comment 56

3 years ago
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39485/diff/4-5/
Attachment #8729552 - Flags: review?(dholbert)
Attachment #8729552 - Flags: review?(dbaron)
Assignee

Comment 57

3 years ago
Comment on attachment 8729553 [details]
MozReview Request: Bug 759568 - Part 2. Render background-clip:text;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39487/diff/4-5/
Attachment #8729553 - Flags: review?(jfkthame)
Assignee

Comment 58

3 years ago
Comment on attachment 8729554 [details]
MozReview Request: Bug 759568 - Part 3. Render text-selection beneath background image;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39489/diff/4-5/
Attachment #8729554 - Attachment description: MozReview Request: Bug 759568 - Part 3. Render text-selection-color beneath background-clip-text; → MozReview Request: Bug 759568 - Part 3. Render text-selection beneath background image;
Attachment #8729554 - Flags: review?(jfkthame)
Assignee

Updated

3 years ago
Attachment #8732328 - Flags: review?(dbaron)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Assignee

Comment 62

3 years ago
Hi dbaron, dholbert,
Modified part 1 base on discussion, please help to review.

Hi jfkthame,
Since I changed text rendering code path, please help to review my patches.
Attachment #8729552 - Flags: review?(dholbert)
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

https://reviewboard.mozilla.org/r/39485/#review38805

::: layout/base/nsLayoutUtils.cpp:350
(Diff revision 5)
>      isFloatLogicalValuesEnabled ? eCSSKeyword_inline_end : eCSSKeyword_UNKNOWN;
>  }
>  
> +static void
> +BackgroundClipTextEnabledPrefChangeCallback(const char* aPrefName,
> +                                      void* aClosure)

Please fix indentation here.

::: layout/base/nsLayoutUtils.cpp:356
(Diff revision 5)
> +{
> +  NS_ASSERTION(strcmp(aPrefName, BG_CLIP_TEXT_ENABLED_PREF_NAME) == 0,
> +               "Did you misspell " BG_CLIP_TEXT_ENABLED_PREF_NAME " ?");
> +
> +  static bool sIsBackgroundClipKeywordIndexInitialized;
> +  static int32_t sIndexOfTextBackgroundClipTable;

s/Text/TextIn/
(this variable is meant to represent "index of 'text' *in* the background-clip table" -- "in" is a key word.)

Also: you might want to just replace "Background" with "BG" in all of the local variables in this function, for brevity & consistency. (You've already got "BG" in some of them, like BG_CLIP_TEXT_ENABLED_PREF_NAME and "isBGClipTextEnabled".)

::: layout/style/Declaration.cpp:299
(Diff revision 5)
>                 "should not have inherit/initial within list");
>  
>      if (clip->mValue.GetIntValue() != NS_STYLE_IMAGELAYER_CLIP_BORDER ||
>          origin->mValue.GetIntValue() != NS_STYLE_IMAGELAYER_ORIGIN_PADDING) {
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> +#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> +      MOZ_ASSERT_IF(aTable == nsStyleImageLayers::kMaskLayerTable,

Since you're making these assertions a bit more complex, could you also add a comment to document the point of these assertions?

Your modified comment from nsCSSParser seems like it'd probably work here:

    // 'mask-clip' and 'mask-origin' use the same keyword table

::: layout/style/nsCSSParser.cpp:12026
(Diff revision 5)
>                     "consume tokens?");
>          if (result == CSSParseResult::NotFound) {
>            // When exactly one <box> value is set, it is used for both
>            // 'background-origin' and 'background-clip'.
>            // See assertions above showing these values are compatible.
>            aState.mClip->mValue = aState.mOrigin->mValue;

This comment ("see assertions above showing these values are compatible") needs updating, because you've redirected one of the assertions to not apply for background-origin/background-clip.

(Probably also worth a sanity-check to be sure we're not violating any dependencies here by using different keyword tables now.  I suspect we might be OK here, because every valid "background-origin" value is also a valid "background-clip" value, and we're copying in that direction here (background-origin --> background-clip).  I haven't poked around this code enough to be 100% sure, though.)

::: layout/style/nsCSSProps.cpp:893
(Diff revision 5)
>  };
>  
>  static_assert(NS_STYLE_IMAGELAYER_CLIP_BORDER == NS_STYLE_IMAGELAYER_ORIGIN_BORDER &&
>                NS_STYLE_IMAGELAYER_CLIP_PADDING == NS_STYLE_IMAGELAYER_ORIGIN_PADDING &&
>                NS_STYLE_IMAGELAYER_CLIP_CONTENT == NS_STYLE_IMAGELAYER_ORIGIN_CONTENT,
> -              "bg-clip and bg-origin style constants must agree");
> +              "Except bg-clip:text, all the other bg-clip and bg-origin style constants must agree");

Two nits:

 (1) The string "bg-clip:text" here looks too much like actual CSS to be using this "bg" abbreviation.  Please use the actual property-name -- s/bg/background/.
 
 (2) This line is now too long (101 columns in this patch, and a good bit longer after you expand 'bg'). Please wrap this line to <= 80 columns.

::: layout/style/nsCSSProps.cpp:904
(Diff revision 5)
>    { eCSSKeyword_UNKNOWN, -1 }
>  };
>  
> +KTableEntry nsCSSProps::kBackgroundClipKTable[] = {
> +  { eCSSKeyword_border_box, NS_STYLE_IMAGELAYER_ORIGIN_BORDER },
> +  { eCSSKeyword_padding_box, NS_STYLE_IMAGELAYER_ORIGIN_PADDING },

You need s/ORIGIN/CLIP/ for all of these constants, right?

(The assertion a little ways up from this proves that the underlying integer values are technically the same, so technically it shouldn't matter.  But as long as we have the CLIP constants, we should be using them here, I'd expect. The only reason that current mozilla-central uses ORIGIN constants for background-clip's table is because the table is shared between background-clip & background-origin.)

::: modules/libpref/init/all.js:2404
(Diff revision 5)
>  // Are "-webkit-{min|max}-device-pixel-ratio" media queries supported?
>  // (Note: this pref has no effect if the master 'layout.css.prefixes.webkit'
>  // pref is set to false.)
>  pref("layout.css.prefixes.device-pixel-ratio-webkit", false);
>  
> +pref("layout.css.background-clip.text", false);

A few things:
 (1) Please rename this new pref to "layout.css.background-clip-text.enabled". We use this pattern for pref-controlled property-values pretty consistently, e.g.:
  layout.css.display-contents.enabled
  layout.css.box-decoration-break.enabled
  layout.css.outline-style-auto.enabled
  
(We don't follow this "foo.enabled" pattern for prefix-related prefs -- layout.css.prefixes.XYZ -- I'm guessing you may have been looking at those when you named this.  But those are actually an anti-pattern -- it'd probably be better if we *did* follow this ".enabled" pattern for those there as well, because bug 1208744 makes it harder to add sub-prefs without breaking stuff -- see bug 1237720 comment 2 if you're curious.)

 (2) Please insert this line a bit lower down, somewhere alongside the other pref-controlled CSS property-values that I quoted above (e.g. layout.css.display-contents.enabled). Right now, your patch is inserting this in the midst of some definitevely webkit-prefixing-specific features (in the range between "layout.css.prefixes.webkit" & "layout.css.unprefixing-service.enabled").  And while this new feature is webkit-prefix-related in terms of how it's used on the web, I think it's better if we don't think of it that way from an implementation perspective, per comment 49.
Comment on attachment 8729553 [details]
MozReview Request: Bug 759568 - Part 2. Render background-clip:text;

https://reviewboard.mozilla.org/r/39487/#review38877

::: layout/base/nsDisplayList.h:274
(Diff revision 5)
>    bool IsForImageVisibility() { return mMode == IMAGE_VISIBILITY; }
> +  /**
> +   * @return true if the display list is being built for creating the glyph
> +   * path from text items. While painting the display list, all text display
> +   * items should only create glyph paths in target context, instead of
> +   * drawing text into.

s/into./into it./

::: layout/base/nsDisplayList.cpp:501
(Diff revision 5)
> +  // 1. Ask every text frame objects in aFrame puts glyph paths into aContext.
> +  // 2. Then, clip aContext.

What will happen if we're unable to get glyph paths for the text? I'd expect this to be the case for bitmap-only fonts such as Apple Color Emoji on OS X, or Noto Color Emoji on Linux systems.

In that situation, do we end up clipping everything, or nothing? Do we paint the glyphs as normal? We should at least consider what's the appropriate result. (What does Safari do with -webkit-background-clip:text if the text involved is all emoji?)

::: layout/base/nsDisplayList.cpp:522
(Diff revision 5)
> +  gfxContext* target = aContext->ThebesContext();
> +  gfxContextMatrixAutoSaveRestore save(target);
> +  target->SetMatrix(target->CurrentMatrix().Translate(aFillRect.TopLeft()));
> +  target->NewPath();

Please don't use `target` for a gfxContext variable; it sounds too much like a DrawTarget. Better to call it `ctx` or something like that.

I wonder, though, if we can in fact use a DrawTarget instead of gfxContext? There was a bunch of work done to migrate away from gfxContext in favor of DrawTarget usage; in that spirit, can you do

    DrawTarget* target = aContext->GetDrawTarget();

and then use DrawTarget methods directly here?

::: layout/base/nsDisplayList.cpp:2915
(Diff revision 5)
> +    case NS_STYLE_IMAGELAYER_CLIP_TEXT:
>        clipRect = nsRect(aItem->ToReferenceFrame(), frame->GetSize());

Is this OK, if the shapes of the glyphs extend beyond the frame's rect? There can be substantial "ink overflow" with some fonts; do you need to take that into account for the clipRect here?

::: layout/base/nsDisplayList.cpp:3033
(Diff revision 5)
>    uint32_t flags = aBuilder->GetBackgroundPaintFlags();
>    CheckForBorderItem(this, flags);
>  
> +  nsRect borderBox = nsRect(ToReferenceFrame(), mFrame->GetSize());
> +  gfxContext* ctx = aCtx->ThebesContext();
> +  gfxContextAutoSaveRestore save(ctx);

This adds a gfxContext::Save/Restore pair every time, but normally it's not needed so it is just extra overhead.

Please do the Save/Restore only if actually using ClipBackgroundByText.

Also, can we instead use DrawTarget methods to just push/pop the clip here, rather than save/restore the whole gfxContext state? (I'm assuming that would be cheaper.)

::: layout/base/nsDisplayList.cpp:3349
(Diff revision 5)
>    gfxContext* ctx = aCtx->ThebesContext();
>  
>    gfxRect bounds =
>      nsLayoutUtils::RectToGfxRect(borderBox, mFrame->PresContext()->AppUnitsPerDevPixel());
>  
> +  gfxContextAutoSaveRestore save(ctx);

Again, move the save/restore inside the conditional, so that we don't pay the extra cost on other paint operations.

::: layout/generic/nsFrame.cpp:1655
(Diff revision 5)
> +  if (aBuilder->IsForGenerateGlyphPath())
> +    return;

nit: style guide says this should have braces. (For bonus points, you could add them to the similar case immediately above, while you're here.)
Attachment #8729553 - Flags: review?(jfkthame)
Comment on attachment 8729554 [details]
MozReview Request: Bug 759568 - Part 3. Render text-selection beneath background image;

https://reviewboard.mozilla.org/r/39489/#review38875

::: layout/generic/nsTextFrame.cpp:6476
(Diff revision 5)
>    }
>  }
> +static bool
> +ShouldDrawSelection(const nsIFrame* aFrame,
> +                    const nsTextFrame::PaintTextParams& aParams)

nit: please add a blank line before the new function

::: layout/generic/nsTextFrame.cpp:6573
(Diff revision 5)
> +  nscolor foregroundColor = textPaintStyle.GetTextColor();
> +  if (aOpacity != 1.0f) {
> +    gfx::Color gfxColor = gfx::Color::FromABGR(foregroundColor);
> +    gfxColor.a *= aOpacity;
> +    foregroundColor = gfxColor.ToABGR();
> +  }

Why is this chunk moved? I can't see any reason it needs to happen earlier; the following if() block doesn't use the foregroundColor value being computed here.
Attachment #8729554 - Flags: review?(jfkthame)
Assignee

Comment 67

3 years ago
Posted image applo_emoji.png
Assignee

Comment 68

3 years ago
https://reviewboard.mozilla.org/r/39487/#review38877

> What will happen if we're unable to get glyph paths for the text? I'd expect this to be the case for bitmap-only fonts such as Apple Color Emoji on OS X, or Noto Color Emoji on Linux systems.
> 
> In that situation, do we end up clipping everything, or nothing? Do we paint the glyphs as normal? We should at least consider what's the appropriate result. (What does Safari do with -webkit-background-clip:text if the text involved is all emoji?)

Thanks for catching up this issue. We end up clipping everthing, while we can still paint the shape of non-bitmap fonts correlty.

Here is the screenshots of apple emoji background-clip-text redering result on three browsers
https://bug759568.bmoattachments.org/attachment.cgi?id=8735362

Only google chrome can make correct path for emoji. I "guess" they use mask in this case. I am fixing this problem.
Assignee

Comment 69

3 years ago
https://reviewboard.mozilla.org/r/39485/#review38805

> This comment ("see assertions above showing these values are compatible") needs updating, because you've redirected one of the assertions to not apply for background-origin/background-clip.
> 
> (Probably also worth a sanity-check to be sure we're not violating any dependencies here by using different keyword tables now.  I suspect we might be OK here, because every valid "background-origin" value is also a valid "background-clip" value, and we're copying in that direction here (background-origin --> background-clip).  I haven't poked around this code enough to be 100% sure, though.)

But I think this comment is valid because of this static assertion
  line 12002: "static_assert(NS_STYLE_IMAGELAYER_CLIP_BORDER =="
That comment still has a strong relation with this static_assert. How do you think?
(In reply to C.J. Ku[:cjku] from comment #69)
> https://reviewboard.mozilla.org/r/39485/#review38805
> 
> > This comment ("see assertions above[...]") needs updating
>
> But I think this comment is valid because of this static assertion

Yes -- my point was, it looks like there's only *one* static assertion that's related to this comment now.  So, the comment probably needs s/assertions/assertion/ ("See assertion above").
Assignee

Comment 71

3 years ago
Hi Jonathan,
From my understanding, Apple Color Emoji are PNG images, as a result, we can't get path from this font.
I have two proposals:
1. Keep using path clip and ignore bitmap fonts.
2. Use mask. Paint all glyph onto a mask layer and blend that mask with background-image.

The 1 solution should have better performance, while the second one is more robust. May I have your opinion here?
Flags: needinfo?(jfkthame)
So the question here, I guess, is whether an emoji glyph is a "shape" like any other glyph, that can be filled with a background image (or color), or whether its "picture" nature overrides this and remains visible. Or in other words, whether the Chrome or Safari result shown in attachment 8735362 [details] is preferable.

I was initially going to favor Chrome's result (which we could get using a mask), but on second thoughts I don't like it. Text is still text, even when it's being filled with a background color, gradient, or even image, and the meaning of the characters should not be thrown away just because a designer decides to fill headlines with a gradient. For "normal" glyphs, the identity of the glyph is clear solely from its shape, and how that shape is filled is unimportant.

Using the mask approach would mean that the two headlines
  <h1>I'm feeling &#x1f604;!</h1>
  <h1>I'm feeling &#x1f622;!</h1>
when filled with a gradient using background-clip:text would suddenly become indistinguishable; all meaning is lost. ISTM that's a pretty bad outcome.

So I think I'd prefer us to follow Safari's behavior here, and render color-bitmap glyphs as their normal images.
Flags: needinfo?(jfkthame)
I think I agree with jfkthame that Safari's behavior is more sensible here. But that's actually not part of this issue. It is that we should not apply color to color-bitmap glyphs at all (including the alpha channel). So I think the current patch is fine in terms of this, and we probably should file another bug for that behavior.
Assignee

Comment 74

3 years ago
Agree.
If background-clip:text is a standard for "drawing background image in text shape", chrome's behavior is preferred.
If we think background-clip:text as a standard of "drawing colorful font", we should choose safari's behavior.

The remaining question(for me) is: how can I know that we are going to use a color-bitmap glyph?
1. By unicode: just paint all emoji characters. 
2. By path: draw that character if we get a empty path from it.
(In reply to C.J. Ku[:cjku] from comment #74)
> The remaining question(for me) is: how can I know that we are going to use a
> color-bitmap glyph?
> 1. By unicode: just paint all emoji characters. 
> 2. By path: draw that character if we get a empty path from it.

Some version of (2), I think. There are emoji fonts that include valid monochrome outlines -- e.g. Microsoft's Segoe UI Emoji, when used on a system that doesn't support the COLR/CPAL tables to produce multi-layered color rendering -- and with these, I'd expect background-clip:text to work just as it does with any other monochrome characters. And there are characters that can have both "text-style" and "emoji-style" presentations for the same Unicode value, with the choice being made by one of (a) variation selectors in the text; (b) styling information such as OpenType font features; or (c) selection of different fonts.

If we get an empty path for a character, that could mean that it's simply a whitespace character, but "drawing" that character should be harmless. Still, it would feel more correct if we could distinguish the result "the glyph has no concept of 'path' that can be retrieved" (such as a PNG bitmap) from "the glyph in principle has a 'path', which happens to be empty" (a whitespace glyph in a TrueType or Type1 font). I wonder -- without having checked the code -- whether the functions we're using to get glyph paths return an error of some kind for the bitmap case, so we can recognize it? Or could we enhance them to support this distinction?
Assignee

Comment 76

3 years ago
https://reviewboard.mozilla.org/r/39487/#review38877

> Is this OK, if the shapes of the glyphs extend beyond the frame's rect? There can be substantial "ink overflow" with some fonts; do you need to take that into account for the clipRect here?

I think it should be fine. We are actually painting background in text-shape, so clip out everything outside BG-border boundary should be correct. Generally, web designer will increase padding to prevent this symptom happen.
Assignee

Comment 77

3 years ago
(In reply to Jonathan Kew (:jfkthame) [PTO this week] from comment #75)
> (In reply to C.J. Ku[:cjku] from comment #74)
> checked the code -- whether the functions we're using to get glyph paths
> return an error of some kind for the bitmap case, so we can recognize it? Or
> could we enhance them to support this distinction?
There is no error, but path->isEmpty() return true.
I don't think we need to do anything special in this bug for color glyphs. We should just fix bug 1260575.
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

https://reviewboard.mozilla.org/r/39485/#review41157

::: layout/base/nsLayoutUtils.cpp:148
(Diff revision 5)
>  #define GRID_ENABLED_PREF_NAME "layout.css.grid.enabled"
>  #define GRID_TEMPLATE_SUBGRID_ENABLED_PREF_NAME "layout.css.grid-template-subgrid-value.enabled"
>  #define DISPLAY_CONTENTS_ENABLED_PREF_NAME "layout.css.display-contents.enabled"
>  #define TEXT_ALIGN_UNSAFE_ENABLED_PREF_NAME "layout.css.text-align-unsafe-value.enabled"
>  #define FLOAT_LOGICAL_VALUES_ENABLED_PREF_NAME "layout.css.float-logical-values.enabled"
> +#define BG_CLIP_TEXT_ENABLED_PREF_NAME "layout.css.background-clip.text"

Please make the pref name be layout.css.background-clip-text.enabled

::: layout/style/Declaration.cpp:298
(Diff revision 5)
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> +#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> +      MOZ_ASSERT_IF(aTable == nsStyleImageLayers::kMaskLayerTable,
> +                    nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::origin]] ==
> +                      aTable[nsStyleImageLayers::origin]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> +                    nsCSSProps::kImageLayerOriginKTable);
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> +      MOZ_ASSERT_IF(aTable == nsStyleImageLayers::kMaskLayerTable,
> +                    nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::clip]] ==
> +                      aTable[nsStyleImageLayers::clip]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> +                    nsCSSProps::kImageLayerOriginKTable);
> +#endif

Putting these assertions behind the MOZ_ENABLE_MASK_AS_SHORTHAND ifdef doesn't seem right.  There's an invariant these are trying to check, and you're making it no longer check that invariant.

I think you should fix the assertions to continue to assert what they need to assert -- which is that you should check that all of the values in the origin table have the identical value (same constant maps to same keyword -- and probably at same position in table) in the clip table.

This is what we need to assert to make sure that the code below (which checks clip values for equality with origin values) makes sense.

(Same for the other set of assertions.)

::: layout/style/nsCSSProps.cpp:906
(Diff revision 5)
>  
> +KTableEntry nsCSSProps::kBackgroundClipKTable[] = {
> +  { eCSSKeyword_border_box, NS_STYLE_IMAGELAYER_ORIGIN_BORDER },
> +  { eCSSKeyword_padding_box, NS_STYLE_IMAGELAYER_ORIGIN_PADDING },
> +  { eCSSKeyword_content_box, NS_STYLE_IMAGELAYER_ORIGIN_CONTENT },
> +  // The next entry is controlled by the layout.css.background-clip.text.

Include the word "pref" or "preference" in this comment (and also fix the pref name)

::: modules/libpref/init/all.js:2404
(Diff revision 5)
>  // Are "-webkit-{min|max}-device-pixel-ratio" media queries supported?
>  // (Note: this pref has no effect if the master 'layout.css.prefixes.webkit'
>  // pref is set to false.)
>  pref("layout.css.prefixes.device-pixel-ratio-webkit", false);
>  
> +pref("layout.css.background-clip.text", false);

Please also file a bug on enabling the feature, and add the bug number here.
Attachment #8729552 - Flags: review?(dbaron)
Comment on attachment 8732328 [details]
MozReview Request: Bug 759568 - Part 4. mochitest for background-clip:text;

https://reviewboard.mozilla.org/r/41099/#review41171

::: layout/style/test/property_database.js:7382
(Diff revision 3)
>                        "open '#'", "'#' filled", "dot '#'", "'#' circle", "1", "1 open", "open 1" ]
>    };
>  }
>  
> +if (IsCSSPropertyPrefEnabled("layout.css.background-clip.text")) {
> +  gCSSProperties["background-clip"].other_values.concat([ "text", "content-box, text", "text, border-box", "text, text" ]);

This is not doing anything, since the result of concat is ignored.

::: layout/style/test/property_database.js:7386
(Diff revision 3)
> +if (IsCSSPropertyPrefEnabled("layout.css.background-clip.text")) {
> +  gCSSProperties["background-clip"].other_values.concat([ "text", "content-box, text", "text, border-box", "text, text" ]);
> +  gCSSProperties["background"].other_values.push("url(404.png) green padding-box text");
> +  gCSSProperties["background"].other_values.push("content-box text url(404.png) blue");
> +} else {
> +  gCSSProperties["background-clip"].invalid_values.concat([ "text", "content-box, text", "text, border-box", "text, text" ]);

same here (result of concat is ignored)
Attachment #8732328 - Flags: review?(dbaron)
https://reviewboard.mozilla.org/r/41099/#review41175

Also, please fix the spelling of "mochitest" in the commit message.

::: layout/style/test/property_database.js:7382
(Diff revision 3)
>                        "open '#'", "'#' filled", "dot '#'", "'#' circle", "1", "1 open", "open 1" ]
>    };
>  }
>  
> +if (IsCSSPropertyPrefEnabled("layout.css.background-clip.text")) {
> +  gCSSProperties["background-clip"].other_values.concat([ "text", "content-box, text", "text, border-box", "text, text" ]);

(And an additional note here:  in general I'd expect you to look at the test output to see that the tests that you've written have run as you expect.  You didn't do that here.)
https://reviewboard.mozilla.org/r/41099/#review41171

> This is not doing anything, since the result of concat is ignored.

It seems push method should be used here. It supports pushing multiple items.
Comment on attachment 8733945 [details]
MozReview Request: Bug 759568 - Part 5. reftest for background-clip:text;

https://reviewboard.mozilla.org/r/42039/#review41177

::: layout/reftests/backgrounds/background-clip-text-1-ref.html:4
(Diff revision 2)
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <title>ackground-clip: text reference</title>

missing "b"

::: layout/reftests/backgrounds/background-clip-text-1-ref.html:10
(Diff revision 2)
> +          <linearGradient id="grad1" x1="0%" y1="0%" x2="100%" y2="0%">
> +            <stop offset="0%" style="stop-color:green;stop-opacity:1" />
> +            <stop offset="100%" style="stop-color:green;stop-opacity:1" />
> +          </linearGradient>
> +        </defs>
> +        <text x="0" y="200" font-size="100px" fill="url(#grad1)" font-family="serif">TEXT clip</text>

Why can't this just use fill="green" and skip the gradient?
Attachment #8733945 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/42039/#review41177

> Why can't this just use fill="green" and skip the gradient?

IIRC, using fill="green" would trigger an optimization to just render the text fill directly rather than generating a path then filling it. These two different code paths would generate slight different result, probably due to subpixel things.

I guess it's worth a comment here to describe, though.
Assignee

Comment 85

3 years ago
https://reviewboard.mozilla.org/r/39485/#review41157

> Putting these assertions behind the MOZ_ENABLE_MASK_AS_SHORTHAND ifdef doesn't seem right.  There's an invariant these are trying to check, and you're making it no longer check that invariant.
> 
> I think you should fix the assertions to continue to assert what they need to assert -- which is that you should check that all of the values in the origin table have the identical value (same constant maps to same keyword -- and probably at same position in table) in the clip table.
> 
> This is what we need to assert to make sure that the code below (which checks clip values for equality with origin values) makes sense.
> 
> (Same for the other set of assertions.)

OK. And I think the static_asset bellow these MOZ_ASSERTs is not needed, since we already have it in nsCSSProps.cpp
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp#890
Assignee

Updated

3 years ago
Blocks: 1263516
Assignee

Comment 86

3 years ago
https://reviewboard.mozilla.org/r/39487/#review38877

> Thanks for catching up this issue. We end up clipping everthing, while we can still paint the shape of non-bitmap fonts correlty.
> 
> Here is the screenshots of apple emoji background-clip-text redering result on three browsers
> https://bug759568.bmoattachments.org/attachment.cgi?id=8735362
> 
> Only google chrome can make correct path for emoji. I "guess" they use mask in this case. I am fixing this problem.

Lets fix it in bug 1260575

> Please don't use `target` for a gfxContext variable; it sounds too much like a DrawTarget. Better to call it `ctx` or something like that.
> 
> I wonder, though, if we can in fact use a DrawTarget instead of gfxContext? There was a bunch of work done to migrate away from gfxContext in favor of DrawTarget usage; in that spirit, can you do
> 
>     DrawTarget* target = aContext->GetDrawTarget();
> 
> and then use DrawTarget methods directly here?

rename target to ctx: will do
Uisng DrawTarget directly: It may not suitable here. Path and path builder is actaully carried in gfxContext(not DrawTarget), so we need matrix information in gfxContext correct.

> This adds a gfxContext::Save/Restore pair every time, but normally it's not needed so it is just extra overhead.
> 
> Please do the Save/Restore only if actually using ClipBackgroundByText.
> 
> Also, can we instead use DrawTarget methods to just push/pop the clip here, rather than save/restore the whole gfxContext state? (I'm assuming that would be cheaper.)

Save/Restore only if actually using ClipBackgroundByText: will do.
Using DrawTarget push/pop directly: the same, since we use path and path builder(gfxContext attributes) in ClipBackgroundByText, save/restore may be more suitable.
Assignee

Comment 87

3 years ago
https://reviewboard.mozilla.org/r/39489/#review38875

> Why is this chunk moved? I can't see any reason it needs to happen earlier; the following if() block doesn't use the foregroundColor value being computed here.

I should move it to original place. The reason of moving up this chunk has disappeared.
Assignee

Comment 88

3 years ago
https://reviewboard.mozilla.org/r/42039/#review41177

> IIRC, using fill="green" would trigger an optimization to just render the text fill directly rather than generating a path then filling it. These two different code paths would generate slight different result, probably due to subpixel things.
> 
> I guess it's worth a comment here to describe, though.

Xidorn is right.

Go deeper into code
https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGTextFrame.cpp#5275
I need SVGTextFrame::ShouldRenderAsPath return true, so the SVGTextFrame::PaintSVG will generate a path and fill this path up, just like what background-text:clip did.
Assignee

Comment 89

3 years ago
https://reviewboard.mozilla.org/r/39485/#review41157

> Please also file a bug on enabling the feature, and add the bug number here.

Done. Bug 1263516.
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
No longer blocks: 1259345
Comment on attachment 8729553 [details]
MozReview Request: Bug 759568 - Part 2. Render background-clip:text;

https://reviewboard.mozilla.org/r/39487/#review42045

Looks good, thanks. Just a couple small cleanup issues noted.

::: layout/base/nsDisplayList.cpp:531
(Diff revision 6)
> +
> +  RefPtr<LayerManager> layerManager =
> +    list.PaintRoot(&builder, aContext, nsDisplayList::PAINT_DEFAULT);
> +
> +  ctx->Clip();
> +  list.DeleteAll();

Do we need to explicitly call this? `list` is going out of scope here, so its destructor should take care of cleaning up, shouldn't it?

::: layout/base/nsDisplayList.cpp:3358
(Diff revision 6)
>    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1148418#c21 for why this
>    // results in a precision induced rounding issue that makes the rect one
>    // pixel shorter in rare cases. Disabled in favor of the old code for now.
>    // Note that the pref layout.css.devPixelsPerPx needs to be set to 1 to
>    // reproduce the bug.

Given that this code is currently disabled, I guess it's OK to leave it untouched for now. But please add a TODO comment pointing out that it does not include support for `background-clip:text`; that will need to be fixed if/when we switch to this new code path.

::: layout/base/nsDisplayList.cpp:3376
(Diff revision 6)
>    gfxContext* ctx = aCtx->ThebesContext();
>  
>    gfxRect bounds =
>      nsLayoutUtils::RectToGfxRect(borderBox, mFrame->PresContext()->AppUnitsPerDevPixel());
>  
> +

no need to add an extra blank line here
Attachment #8729553 - Flags: review?(jfkthame) → review+
Comment on attachment 8729554 [details]
MozReview Request: Bug 759568 - Part 3. Render text-selection beneath background image;

https://reviewboard.mozilla.org/r/39489/#review42049

::: layout/generic/nsTextFrame.cpp:6517
(Diff revision 6)
> +  NS_FOR_VISIBLE_IMAGE_LAYERS_BACK_TO_FRONT(i, layers) {
> +     if (layers.mLayers[i].mClip == NS_STYLE_IMAGELAYER_CLIP_TEXT) {
> +       return false;
> +    }

The indentation of the if-statement here looks one space off.
Attachment #8729554 - Flags: review?(jfkthame) → review+
Attachment #8733946 - Flags: review?(jfkthame) → review+
Comment on attachment 8733946 [details]
MozReview Request: Bug 759568 - Part 6. Remove unused nsDisplayList::mVisibleRect;

https://reviewboard.mozilla.org/r/42041/#review42051
Attachment #8729552 - Flags: review?(dholbert) → review+
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

https://reviewboard.mozilla.org/r/39485/#review42061

r=me with nits addressed:

::: layout/base/nsLayoutUtils.cpp:397
(Diff revision 6)
>    nsCSSProps::kClearKTable[sIndexOfInlineEndInClearTable].mKeyword =
>      isFloatLogicalValuesEnabled ? eCSSKeyword_inline_end : eCSSKeyword_UNKNOWN;
>  }
>  
> +static void
> +BackgroundClipTextEnabledPrefChangeCallback(const char* aPrefName,

Please include an explanatory comment above this function, like we have for all the other similar functions above it.

(You can copypaste the comment for e.g. DisplayContentsEnabledPrefChangeCallback, and just update the pasted comment to change the pref name, the keyword table, and the value's name.)

::: layout/style/Declaration.cpp:299
(Diff revision 6)
>                 "should not have inherit/initial within list");
>  
>      if (clip->mValue.GetIntValue() != NS_STYLE_IMAGELAYER_CLIP_BORDER ||
>          origin->mValue.GetIntValue() != NS_STYLE_IMAGELAYER_ORIGIN_PADDING) {
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::origin]] ==
> +#ifdef DEBUG
> +      for (int i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1 ; i++) {

Two nits on the "for" statement:

(1) s/int/size_t/
   (Gecko C++ code rarely uses bare "int" values at all, including as a loop index. Sometimes I've seen int32_t, but that's somewhat problematic because it could conceivably overflow before you finish iterating, for large enough input.  Anyway, size_t is the correct type for an array-index, and it should work here, since the index is monotonically increasing from 0.)
   
 (2) Drop the stray space character between "-1" and the semicolon.

::: layout/style/Declaration.cpp:303
(Diff revision 6)
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::origin]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::clip]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> +#ifdef DEBUG
> +      for (int i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1 ; i++) {
> +        // Ensure the keyword positions in origin and clip table are the same.
> +        MOZ_ASSERT(nsCSSProps::kImageLayerOriginKTable[i].mKeyword ==
> +                   nsCSSProps::kBackgroundClipKTable[i].mKeyword);
> +        // Ensure the keyword values in origin and clip table are the same.

The two new comments here ("Ensure the...") aren't quite correct. The keyword positions in the two tables are not in fact "the same", because kBackgroundClipKTable now has an entry that's not present in kOriginKTable.

Please reword the comments to clarify this. E.g. something like the following would be more correct documentation:
"For each keyword & value in kOriginKTable, ensure that kBackgroundKTable has a matching entry at the same position."

::: layout/style/nsCSSParser.cpp:12065
(Diff revision 6)
>          // The spec allows a second box value (for background-clip),
>          // immediately following the first one (for background-origin).
>  
> -        // 'background-clip' and 'background-origin' use the same keyword table
> -        MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                     aTable[nsStyleImageLayers::origin]] ==
> +#ifdef DEBUG
> +        for (int i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1 ; i++) {
> +          // Ensure the keyword positions in origin and clip table are the same.

(The above nits on the for-loop & the comment-rewording apply here as well.)

::: layout/style/nsCSSProps.cpp:916
(Diff revision 6)
> +};
> +
> +static_assert(ArrayLength(nsCSSProps::kImageLayerOriginKTable) ==
> +              ArrayLength(nsCSSProps::kBackgroundClipKTable) - 1,
> +              "background-clip has one more extra value, which is text, compare"
> +              "to background-origin");

(1) "one more extra" is redundant -- just say "one more" or "one extra"
(2) "compare to" --> "compared to" (add a "d")

::: modules/libpref/init/all.js:2427
(Diff revision 6)
>  // Is support for background-blend-mode enabled?
>  pref("layout.css.background-blend-mode.enabled", true);
>  
> +// Keep this disabled on for now. See bug 1263516.
> +// Is support for background-clip:text enabled?
> +pref("layout.css.background-clip-text.enabled", false);

Drop the "Keep this disabled for now" comment. That implies there's some important reason that we can't enable this, which is described in the linked bug. (But really, the bug is just about enabling the pref, and I expect we should be able to enable it pretty soon.)

If you want to reference the bug number here (not necessary but might be nice), I'd say to just fold it into the documentation one-liner, e.g.:
// Is support for background-clip:text enabled? (bug 1263516)
Assignee

Comment 100

3 years ago
https://reviewboard.mozilla.org/r/39487/#review42045

> Do we need to explicitly call this? `list` is going out of scope here, so its destructor should take care of cleaning up, shouldn't it?

If you do not explicit call this it, will get a warning message
  ~nsDisplayList() {
    if (mSentinel.mAbove) {
      NS_WARNING("Nonempty list left over?"); << warning message, not a big deal, but make me nervous
    }
    DeleteAll();
  }

nsLayoutUtils explicit calls nsDisplayList::DeleteAll, so do I, here is an example:
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3064
Assignee

Updated

3 years ago
Attachment #8729552 - Flags: review?(cam)
Attachment #8732328 - Flags: review?(cam)
Comment on attachment 8729552 [details]
MozReview Request: Bug 759568 - Part 1. Parse background-clip:text;

https://reviewboard.mozilla.org/r/39485/#review42191

::: layout/style/Declaration.cpp:298
(Diff revision 6)
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::origin]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> -      MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                   aTable[nsStyleImageLayers::clip]] ==
> -                 nsCSSProps::kImageLayerOriginKTable);
> +#ifdef DEBUG
> +      for (int i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1 ; i++) {
> +        // Ensure the keyword positions in origin and clip table are the same.
> +        MOZ_ASSERT(nsCSSProps::kImageLayerOriginKTable[i].mKeyword ==
> +                   nsCSSProps::kBackgroundClipKTable[i].mKeyword);
> +        // Ensure the keyword values in origin and clip table are the same.
> +        MOZ_ASSERT(nsCSSProps::kImageLayerOriginKTable[i].mValue ==
> +                   nsCSSProps::kBackgroundClipKTable[i].mValue);
> +      }
> +#endif

Despite Daniel's comments on this change (which I agree with), I feel it is not useful to have these assertions (or the existing static_asserts) here at all:

* The old assertions check that the *-origin and *-clip properties used the same keyword table, but the following code did not rely on that fact.

* The new assertions check that kImageLayerOriginKTable contains the same properties and values as kBackgroundClipKTable (ignoring extra entries in kBackgroundClipKTable), but the following code does not rely on that fact.

* The static assertions check that the integer style constant values for 'border', 'padding' and 'content' are the same for both *-clip and *-origin properties, but again the following code does not rely on that fact.

I can't see what these assertions protect ourselves against, so in my opinion we can just remove them.

::: layout/style/nsCSSParser.cpp:12063
(Diff revision 6)
> -        // 'background-clip' and 'background-origin' use the same keyword table
> -        MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                     aTable[nsStyleImageLayers::origin]] ==
> -                   nsCSSProps::kImageLayerOriginKTable);
> -        MOZ_ASSERT(nsCSSProps::kKeywordTableTable[
> -                     aTable[nsStyleImageLayers::clip]] ==
> -                   nsCSSProps::kImageLayerOriginKTable);
> +#ifdef DEBUG
> +        for (int i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1 ; i++) {
> +          // Ensure the keyword positions in origin and clip table are the same.
> +          MOZ_ASSERT(nsCSSProps::kImageLayerOriginKTable[i].mKeyword ==
> +                     nsCSSProps::kBackgroundClipKTable[i].mKeyword);
> +          // Ensure the keyword values in origin and clip table are the same.
> +          MOZ_ASSERT(nsCSSProps::kImageLayerOriginKTable[i].mValue ==
> +                     nsCSSProps::kBackgroundClipKTable[i].mValue);
> +        }
> +#endif
>          static_assert(NS_STYLE_IMAGELAYER_CLIP_BORDER ==
>                        NS_STYLE_IMAGELAYER_ORIGIN_BORDER &&
>                        NS_STYLE_IMAGELAYER_CLIP_PADDING ==
>                        NS_STYLE_IMAGELAYER_ORIGIN_PADDING &&
>                        NS_STYLE_IMAGELAYER_CLIP_CONTENT ==
>                        NS_STYLE_IMAGELAYER_ORIGIN_CONTENT,
>                        "bg-clip and bg-origin style constants must agree");

The assertions here are useful, because of the |aState.mClip->mValue = aState.mOrigin->mValue| assignment further down, so do apply Daniel's comments to this change.

Also, I think you should static_assert that the length of kImageLayerOriginKTable is >= the length of kBackgroundClipKTable, so that we won't ever read past the end of kBackgroundClipKTable.

::: layout/style/nsCSSProps.cpp:893
(Diff revision 6)
> -              "bg-clip and bg-origin style constants must agree");
> +              "Except background-clip:text, all the other background-clip and "
> +              "background-origin style constants must agree");

Maybe mention mask-* in the comment as well, e.g.:

Except for background-clip:text, all {background,mask}-clip and {background,mask}-origin style constants must agree.

::: layout/style/nsCSSProps.cpp:916
(Diff revision 6)
> +};
> +
> +static_assert(ArrayLength(nsCSSProps::kImageLayerOriginKTable) ==
> +              ArrayLength(nsCSSProps::kBackgroundClipKTable) - 1,
> +              "background-clip has one more extra value, which is text, compare"
> +              "to background-origin");

If you decide to mention mask-* properties above, make this: "... compared to {background,mask}-origin"
Attachment #8729552 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #101)
> not useful to have these assertions (or the existing static_asserts) here at
> all:

I retract this comment, since we are indeed relying on this (in the |clip->mValue != origin->mValue| check), as Daniel pointed out.
Comment on attachment 8732328 [details]
MozReview Request: Bug 759568 - Part 4. mochitest for background-clip:text;

https://reviewboard.mozilla.org/r/41099/#review42197

::: layout/style/test/property_database.js:7398
(Diff revision 4)
>      invalid_values: [ "rubbish", "dot rubbish", "rubbish dot", "open rubbish", "rubbish open", "open filled", "dot circle",
>                        "open '#'", "'#' filled", "dot '#'", "'#' circle", "1", "1 open", "open 1" ]
>    };
>  }
>  
> +var bg_clip_text_values = [

We should try to avoid adding new global variables to property_database.js, since they will be visible in any test that uses it.  But we have bug 1211332 to fix this for the whole file, so I wouldn't worry about it unless you want to.  If you do keep these variables, please give them camelCaseNames, which is the prevailing style for variable names here.

::: layout/style/test/property_database.js:7399
(Diff revision 4)
>                        "open '#'", "'#' filled", "dot '#'", "'#' circle", "1", "1 open", "open 1" ]
>    };
>  }
>  
> +var bg_clip_text_values = [
> +  "text",

Is "text" really a valid value for the 'background' shorthand when the pref is enabled?  From my reading of your nsCSSParser changes, we will try (and fail) to parse it as a 'background-origin' value.

::: layout/style/test/property_database.js:7411
(Diff revision 4)
> +  gCSSProperties["background-clip"].other_values =
> +    gCSSProperties["background-clip"].other_values.concat(bg_clip_text_values);

You can write this shorter:

gCSSProperties["background-clip"].other_values.push(...backgroundClipTextValues);
Attachment #8732328 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #103)
> > +var bg_clip_text_values = [
> > +  "text",
> 
> Is "text" really a valid value for the 'background' shorthand when the pref
> is enabled?  From my reading of your nsCSSParser changes, we will try (and
> fail) to parse it as a 'background-origin' value.

That we parse the 'background' shorthand like this is a bug.  We should allow the two <box> values to be separated by other values.  That's bug 1188074.  Once we fix that we will need to decide what it means if you write:

  background: text border-box;

since the Backgrounds and Borders spec says that the first <box> value is the background-origin value.
(In reply to Cameron McCormack (:heycam) from comment #103)
> ::: layout/style/test/property_database.js:7399
> (Diff revision 4)
> >                        "open '#'", "'#' filled", "dot '#'", "'#' circle", "1", "1 open", "open 1" ]
> >    };
> >  }
> >  
> > +var bg_clip_text_values = [
> > +  "text",
> 
> Is "text" really a valid value for the 'background' shorthand when the pref
> is enabled?  From my reading of your nsCSSParser changes, we will try (and
> fail) to parse it as a 'background-origin' value.

CJ pointed out to me on IRC that I misread this; we're adding bg_text_values, not bg_clip_text_values, to the other_values array for 'background'.

Just one request, then: please add "text" as an invalid_value for 'background'.
Comment on attachment 8732328 [details]
MozReview Request: Bug 759568 - Part 4. mochitest for background-clip:text;

https://reviewboard.mozilla.org/r/41099/#review42209
Attachment #8732328 - Flags: review+
Backed out for failures in test_all_shorthand.html and more

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b99d656ac9ce0ad8fc6a3654e469e14cd0a7f7c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=25618364&repo=mozilla-inbound

08:11:05     INFO -  191 INFO TEST-START | layout/style/test/test_all_shorthand.html
08:11:05     INFO -  192 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_all_shorthand.html | uncaught exception - SyntaxError: missing ] after element list at
08:11:05     INFO -      simpletestOnerror@SimpleTest/SimpleTest.js:1563:11
08:11:05     INFO -  193 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_all_shorthand.html | uncaught exception - ReferenceError: gCSSProperties is not defined at runTest@http://mochi.test:8888/tests/layout/style/test/test_all_shorthand.html:36:7
08:11:05     INFO -      simpletestOnerror@SimpleTest/SimpleTest.js:1563:11

There is a comma missing at https://hg.mozilla.org/integration/mozilla-inbound/rev/82179839a88cad7631d2431da8302b25329a5b07#l2.12
Flags: needinfo?(cku)
This also caused Windows 8 x64 opt R(R) to fail.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=25625315&repo=mozilla-inbound


REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/background-clip-text-1a.html | image comparison (==), max difference: 1, number of differing pixels: 44
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/background-clip-text-1b.html | image comparison (==), max difference: 1, number of differing pixels: 44
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/background-clip-text-1c.html | image comparison (==), max difference: 1, number of differing pixels: 44
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/background-clip-text-1d.html | image comparison (==), max difference: 1, number of differing pixels: 44
The reftest failure in comment 110 is extremely subtle ("max difference: 1" -- imperceptible mismatches on various pixels scattered around the edges of letters).

Sounds like a fine scenario for a "fuzzy-if()" annotation. If this is windows-specific, "fuzzy-if(winWidget,1,44)" should do.

(Alternately, the harness supports more targeted d2d-specific/windows-version-specific queries that you could use if you like -- though I'm not sure it's worth investing too much time in finding the perfect query for a subtle fuzzy annotation like this.)
Assignee

Comment 112

3 years ago
It takes a long time to wait for win64 try result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ef2fecd83c
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku] from comment #113)
> Bug 759568 - Part 1. Parse background-clip:text; r=dholbert r=heycom
> Bug 759568 - Part 4. mochitest for background-clip:text; r=heycom

(That should be "heycam" with an "a", but not a big deal)
(in any case: hooray / well done! :) Great to see this landed.)
sorry had to back this out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25775070&repo=mozilla-inbound
Flags: needinfo?(cku)
Assignee

Comment 118

3 years ago
Can not parse text attribute after merge inbound. checking!
Flags: needinfo?(cku)
Assignee

Comment 119

3 years ago
bug 1264238 reorder macros in nsCSSPropList.h, rebase on it, what I changed in background-clip macro be moved into background-origin...
Summary: Implement -webkit-background-clip: text; → Implement -webkit-background-clip: text (as background-clip: text)
Depends on: 1265071
No longer blocks: 658467
Depends on: 1265154
Duplicate of this bug: 1265745

Comment 123

3 years ago
Not supported in @keyframes.

Demo: http://jsbin.com/kitivasura/edit?html,css,output
(In reply to percyley from comment #123)
> Not supported in @keyframes.
> 
> Demo: http://jsbin.com/kitivasura/edit?html,css,output

Looks like Safari doesn't animate clipped gradient bg either, but Chrome is happy to.
I've documented this at https://developer.mozilla.org/en-US/docs/Web/CSS/background-clip.

Please let me know if this covers it!

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #125)
> I've documented this at
> https://developer.mozilla.org/en-US/docs/Web/CSS/background-clip.
> 
> Please let me know if this covers it!
> 
> Sebastian

Looks pretty good to me Sebastian, thanks for the work! I've made a couple of small edits.
Assignee

Comment 127

3 years ago
(In reply to percyley from comment #123)
> Not supported in @keyframes.
> 
> Demo: http://jsbin.com/kitivasura/edit?html,css,output
By removing  -webkit-background-clip: text, you will find there is no gradient animation on background-image. I am not familiar with animation, but I think it should be a bug.

Comment 128

3 years ago
(In reply to C.J. Ku[:cjku] from comment #127)
> (In reply to percyley from comment #123)
> > Not supported in @keyframes.
> > 
> > Demo: http://jsbin.com/kitivasura/edit?html,css,output
> By removing  -webkit-background-clip: text, you will find there is no
> gradient animation on background-image. I am not familiar with animation,
> but I think it should be a bug.

Who can repair the gradient animation bug? I'm going to submit a new bug.
(In reply to percyley from comment #128)
> (In reply to C.J. Ku[:cjku] from comment #127)
> > (In reply to percyley from comment #123)
> > > Not supported in @keyframes.
> > > 
> > > Demo: http://jsbin.com/kitivasura/edit?html,css,output
> > By removing  -webkit-background-clip: text, you will find there is no
> > gradient animation on background-image. I am not familiar with animation,
> > but I think it should be a bug.
> 
> Who can repair the gradient animation bug? I'm going to submit a new bug.

I don't think that is something we are going to fix soonish, because:
* that needs nontrivial work;
* no browser correctly supports that;
* we add background-clip: text only for compatibility, and don't think it is something web should rely on.

The original usecase should definitely have some standard way to achieve, which csswg should work on. However, doing so with background-clip is considered a hack, and should be deprecated.

Comment 130

3 years ago
@Xidorn Quan This is not background-clip: text bug. Thanks. We must be support https://drafts.csswg.org/css-images/#interpolating-gradients
OK. We are in background-clip: text bug, so your comment confused me. It seems you were talking aboug bug 536540.

Comment 132

3 years ago
I added a test case in bug 536540.
No longer blocks: 653670

Updated

2 years ago
Depends on: 1395791

Updated

3 months ago
Regressions: 1543117
No longer regressions: 1543117
You need to log in before you can comment on or make changes to this bug.