Closed Bug 1264905 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: u459114, Assigned: dholbert)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1259345
Depends on: 1264949
Blocks: 658467
Depends on: 1265715
Blocks: 1238527
Depends on: 1267128
Depends on: 1267697
Jeremy is handling bug 1267128, still need a couple of days.
Attached 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)
Attached 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)
Attached patch fixSplinter Review
Attachment #8747983 - Attachment is obsolete: true
Attachment #8747983 - Flags: review?(cku)
Attachment #8747984 - Flags: review?(cku)
Attachment #8747982 - Attachment description: fix v1 → (ignore, forgot to qref)
Attachment #8747983 - Attachment description: fix v1 → (ignore, forgot to qref)
Attachment #8747984 - Flags: review?(cku) → review+
backgorund-clip:text is not correct in nightly build, please hold on. I am checking now
(Thanks -- I'm already holding off on landing, until bug 1267128 is addressed [and any other blockers that open up in the meantime].)
Depends on: 1269971
Depends on: 1270795
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)
(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.
>  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).
https://hg.mozilla.org/mozilla-central/rev/bf9501152947
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1273068
Depends on: 1273365
(Forgot to clear needinfo when landing; clearing now.)
Flags: needinfo?(dholbert)
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?
(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+
Blocks: 1277092
(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.