892929 - CSSFontFeatureValuesRule seems unaffected by the pref

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

window.CSSFontFeatureValuesRule should be undefined when layout.css.font-features.enabled is false, but it isn't.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is an xpidl interface, not a WebIDL one, so not defining it on Window is kinda hard...

We should just convert to WebIDL ASAP.  ;)

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

WebIDL doesn't seem backportable (which we might want), and on old bindings it should only take 5 lines or so in OldBindingConstructorEnabled, I think.

Comment 3

[bhavana bajaj [:bajaj]]

(In reply to :Ms2ger from comment #2)
> WebIDL doesn't seem backportable (which we might want), and on old bindings
> it should only take 5 lines or so in OldBindingConstructorEnabled, I think.

Is the behavior not working as expected as well or is it only a front end issue ? 

Also is this "window.CSSFontFeatureValuesRule" a new property given implementation of Bug 549861 ? Under what scenario is this pref generally used and how can a user be impacted ?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The pref is set to false in release builds: <http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#1778>. This property is new in bug 549861; exposing it when we turn off the actual implementation through the pref can break feature detection and probably breaks our experimental implementations policy.

Comment 5

[John Daggett (:jtd)]

There's a related bug to make tests not fail when the pref is disabled, bug 886691.  That's more about making the logic of shorthands work and fixing up tests that aren't sensitive to the pref but I thought I'd mention it here.

Comment 6

[John Daggett (:jtd)]

> Also is this "window.CSSFontFeatureValuesRule" a new property given
> implementation of Bug 549861 ? Under what scenario is this pref generally
> used and how can a user be impacted ?

Like other new CSS properties and rules behind prefs, this will generally be enabled in nightly/aurora builds but disabled in beta/release builds.  The pref will be removed once we make the decision that the spec is stable enough to unpref it.  I think that process is still a little undefined as we haven't done this yet for a lot of preffed-rather-than-prefixed properties.

Comment 7

[John Daggett (:jtd)]

Bouncing this over to bz as I don't really know the right way to go about hiding window.CSSFontFeatureValuesRule based on the pref state.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Per comment 2

This applies to today's Aurora as well.

John, can you deal with writing tests and shepherding this in?  I'm in and out a bit between now and the Aurora merge.  :(

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 782754 [details] [diff] [review]
Per comment 2

But this needs those tests.

Comment 10

[John Daggett (:jtd)]

Attachment: patch, test whether window.CSSFontFeatureRule is hidden when disabled

Comment 11

[John Daggett (:jtd)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> John, can you deal with writing tests and shepherding this in?  I'm in and
> out a bit between now and the Aurora merge.  :(

Yup, no problem.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 782956 [details] [diff] [review]
patch, test whether window.CSSFontFeatureRule is hidden when disabled

>+  var hasFFVRule = (window["CSSFontFeatureValuesRule"] != null);

  var hasFFVRule = "CSSFontFeatureValuesRule" in window;

and similar for hasStyleAlternates and hasCompStyleAlternates.

r=me with that

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks!

Comment 14

[John Daggett (:jtd)]

Pushed to inbound (including changes based on review comments):
https://hg.mozilla.org/integration/mozilla-inbound/rev/60dcd30de3fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b235393dd59

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60dcd30de3fb
https://hg.mozilla.org/mozilla-central/rev/9b235393dd59

Comment 16

[Kohei Yoshino [:kohei]]

Added to the site compatibility doc just in case:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The description in that doc doesn't appear accurate.  This change is substantially different from the one above it (about geolocation).  And this is also a new feature that's not turned on yet, so the effect of this bug should be to make it so that there is no change in Firefox 25.  I think it should be removed from the doc.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

But this does need to be backported to 24.

Comment 19

[Kohei Yoshino [:kohei]]

Thanks for the feedback, removed the reference from the compatibility doc.

Comment 20

[John Daggett (:jtd)]

Comment on attachment 782754 [details] [diff] [review]
Per comment 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 549861
User impact if declined: exposure of CSSFontFeatureValuesRule on window
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 21

[John Daggett (:jtd)]

Comment on attachment 782956 [details] [diff] [review]
patch, test whether window.CSSFontFeatureRule is hidden when disabled

[Approval Request Comment]
-- same as above --

Comment 22

[John Daggett (:jtd)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe11c9394b14
https://hg.mozilla.org/releases/mozilla-aurora/rev/26b9b6a2e8e3

Comment 23

[u279076]

Looks like this landed with tests. Any need for QA here?