1229609 - Remove layout.css.text-emphasis.enabled pref

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

I don't think we need to follow the general "enable the feature by default" then long later "remove the pref" path, because:
1. this feature is not that complicated so I don't think it would cause any major breakage which would require us to switch it off again;
2. the spec around this feature has been quite stable for a while: some other vendors have been shipping a compitable prefixed version of this feature, and I don't see any major blocker during implementing it.

Given this, I believe we can simply remove the pref to enable it directly.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

dbaron, what do you think?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

One correction: Blink is shipping a mostly compatible prefixed version (whose syntax of text-emphasis-position doesn't match the current spec), and WebKit is shipping a compatible unprefixed version.

Those impls haven't yet had the proper behavior for ruby with emphasis marks, though.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

dbaron, could you review this super simple patch?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

As we've missed Firefox 45 for enabling text-emphasis, let's wait for the next version to remove the pref.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d22bebea625

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8696528 [details]
MozReview Request: Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27359/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8696528 [details]
MozReview Request: Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

https://reviewboard.mozilla.org/r/27359/#review41941

I think we should wait at least one cycle with the pref enabled on Release (46) before removing the pref.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8696528 [details]
MozReview Request: Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27359/diff/2-3/

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8696528 [details]
MozReview Request: Bug 1229609 - Remove layout.css.text-emphasis.enabled pref.

https://reviewboard.mozilla.org/r/27359/#review52798

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03bd76ec05cb9b0765e79ab5077a61a533b40168
Bug 1229609 - Remove layout.css.text-emphasis.enabled pref. r=heycam

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03bd76ec05cb

Comment 13

[Jean-Yves Perrier [:teoli]]

I added a comment in:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-emphasis#Browser_compatibility
and in:
https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS