Let layout.css.background-clip-text.enabled ride the trains

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: u459114, Assigned: dholbert)

Tracking

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

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
After the patch in bug 1263516 landed, layout.css.background-clip-text.enabled defaults to "true" on Nightly/Aurora, and "false" on Beta/Release.

Fixing all blockers of this one before turning on layout.css.background-clip-text.enabled on all builds.
(Reporter)

Updated

3 years ago
Blocks: 1259345
(Reporter)

Updated

3 years ago
Depends on: 1264949
Blocks: 658467
(Reporter)

Updated

3 years ago
Depends on: 1265715
Blocks: 1238527
(Reporter)

Updated

3 years ago
Depends on: 1267128
Depends on: 1267697
(Reporter)

Comment 1

3 years ago
Jeremy is handling bug 1267128, still need a couple of days.
(Assignee)

Comment 2

3 years ago
Posted patch (ignore, forgot to qref) (obsolete) — Splinter Review
I'll jump in to steal this bug, to post the (trivial) pref-flip patch.

We should land this ASAP once the last blocker (bug 1267128) is fixed, as long as no other blockers come up (and we should backport this ASAP after that blocker is backported, assuming it gets backport approval).
Assignee: cku → dholbert
Status: NEW → ASSIGNED
Attachment #8747982 - Flags: review?(cku)
(Assignee)

Comment 3

3 years ago
Posted patch (ignore, forgot to qref) (obsolete) — Splinter Review
(Sorry, forgot to qref in the actual file change :))
Attachment #8747982 - Attachment is obsolete: true
Attachment #8747982 - Flags: review?(cku)
Attachment #8747983 - Flags: review?(cku)
(Assignee)

Comment 4

3 years ago
Posted patch fixSplinter Review
Attachment #8747983 - Attachment is obsolete: true
Attachment #8747983 - Flags: review?(cku)
Attachment #8747984 - Flags: review?(cku)
(Assignee)

Updated

3 years ago
Attachment #8747982 - Attachment description: fix v1 → (ignore, forgot to qref)
(Assignee)

Updated

3 years ago
Attachment #8747983 - Attachment description: fix v1 → (ignore, forgot to qref)
(Reporter)

Updated

3 years ago
Attachment #8747984 - Flags: review?(cku) → review+
(Reporter)

Comment 5

3 years ago
backgorund-clip:text is not correct in nightly build, please hold on. I am checking now
(Assignee)

Comment 6

3 years ago
(Thanks -- I'm already holding off on landing, until bug 1267128 is addressed [and any other blockers that open up in the meantime].)
(Assignee)

Updated

3 years ago
Depends on: 1269971
(Reporter)

Updated

3 years ago
Depends on: 1270795
(Reporter)

Comment 7

3 years ago
Hi dholbert,
Do we still need uplift the patches in bug 1269971 into FF 48? I notice that Bloomberg[1] had changed webkkit-text-stroke color from transparent to a solid color:
-webkit-text-stroke: 0.09375rem #FA1E64
So that we can render that page correctly even without the fix in 1269971.

And I think it's time to overall turn on bg-clip:text in FF 49. :)

[1] http://www.bloomberg.com/news/features/2016-02-15/missing-malaysia-jet-mh370-weeks-away-from-keeping-secrets-forever
Flags: needinfo?(dholbert)
(Assignee)

Comment 8

3 years ago
(In reply to C.J. Ku[:cjku] from comment #7)
> Do we still need uplift the patches in bug 1269971 into FF 48?

That's seems probably too risky -- better not to uplift those.

> I notice that
> Bloomberg[1] had changed webkkit-text-stroke color from transparent to a
> solid color:
> -webkit-text-stroke: 0.09375rem #FA1E64
> So that we can render that page correctly even without the fix in 1269971.

I'm not seeing us render that page correctly, in current Nightly (which doesn't quite have bug 1269971's fix).

If I load that Bloomberg page & scroll down a page or two, the pullquotes still look like "ghosts" and are pretty much unreadable. (They look like bug 1248644's attachment 8719912 [details], sort of.)

So: I tend to think we should hold off on enabling "background-clip:text" & webkit prefix support in Firefox 48 (so that we don't ship bug 1248644 as a regression in Firefox 48) -- I think we should let it ride the trains in 49 instead.

> And I think it's time to overall turn on bg-clip:text in FF 49. :)

Once we've verified that bug 1269971 has fixed bug 1248644 (in the nightly tomorrow or the next day), I think I agree!  At that point we can take this bug's patch and bug 1269971's patch.

I'll leave needinfo=me to land those patches after bug 1269971 has made it into a Nightly.
(Assignee)

Comment 9

3 years ago
>  At that point we can take this bug's patch and bug 1269971's patch.

Sorry, I meant "this bug's patch and bug 1259345's patch (the patches to enable prefs unconditionally).

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf9501152947
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Reporter)

Updated

3 years ago
Depends on: 1273068
Depends on: 1273365
(Assignee)

Comment 12

3 years ago
(Forgot to clear needinfo when landing; clearing now.)
Flags: needinfo?(dholbert)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8747984 [details] [diff] [review]
fix

I'm requesting approval to uplift to Aurora48, so that this can be part of a short trial on Beta48 to get broader testing. See bug 1259345 comment 16 and bug 1269971 comment 93 for more.

Approval Request Comment
[Feature/regressing bug #]: This specific feature is "background-clip:text" (a new CSS feature), but it's part of our broader effort to add support for webkit prefixed CSS features & extensions, bug 1259345.  (This is a webkit extension that some sites accidentally depend on, in Firefox builds that recognize webkit-prefixed CSS.)

[User impact if declined]:
 - Zero impact for Aurora48 users, whether this is backported or not. (This is already enabled for them.)
 - "If declined", when 48 goes from Aurora --> beta, our beta48 users won't have this feature default-enabled (or the general webkit-prefixes feature, since it requires this feature to be enabled). That'd mean we won't get any early beta feedback on these features.

[Describe test coverage new/current, TreeHerder]: We have automated mochitest + reftest coverage for this feature (parts 4 and 5 on bug 759568 which provided the implementation).

[Risks and why]:
 - As in bug 1259345, there's zero risk to Firefox 48 release users, since I intend to back out this patch before Firefox 48 hits release.
 - During the Beta 48 trial (see bug 1259345 comment 16), there's a small risk that users will run into regressions from this feature interacting with (or implicitly depending on) other features. Those risks will be mitigated when we turn this off - and in the meantime, we'll hopefully get valuable feedback & bugs filed, sooner than if we waited for this to ride the trains to beta in Firefox 49.

[String/UUID change made/needed]: None.
Attachment #8747984 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

3 years ago
(Sylvestre, let me know if you have any questions about this, or if there's any thing I can do to facilitate approvals here & on the related bugs.)
Flags: needinfo?(sledru)
Comment on attachment 8747984 [details] [diff] [review]
fix

OK, let's try that too :)
Should we ride some release notes for this?
Flags: needinfo?(sledru)
Attachment #8747984 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
Blocks: 1277092
(Assignee)

Comment 17

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Should we ride some release notes for this?

I think my release note on the general webkit-prefix bug (bug 1259345 comment 23) is basically sufficient to cover this as well.
No longer blocks: 658467
Depends on: 1313757
You need to log in before you can comment on or make changes to this bug.