Are you OK with changing this in Gecko/IE?

Should this be only for CSSStyleSheet#insertRule or also CSSGroupingRule#insertRule?

> so maybe it's on WebKit-specific paths

I read through https://code.google.com/p/chromium/issues/detail?id=319695 (again; when I got to the end it became clear I'd done that before) and it looks like the instances there were all on WebKit-specific paths.  Specifically, all of the reports seemed to be from a single jQuery plugin that uses dynamically-inserted CSS animation rules, but only if style.WebkitAnimationName tests not undefined (conveniently using a slower codepath in other browsers, yay.  :( ).

_If_ Chrome is planning to ever remove style.WebkitAnimationName, that would automatically fix the issue with this library.  I don't think it makes much sense to talk about use counters until that happens.  I did mention this in https://code.google.com/p/chromium/issues/detail?id=319695#c34 which got summarily ignored.

Past that, I'm pretty loath to make this argument non-optional because then people would be more likely to use it without the second argument (implying 0), and the behavior of Chrome and other browsers differs wildly when the second argument is 0.  Specifically, because Chrome doesn't represent @namespace and @charset rules in the CSSOM at all, as far as I can tell, so insertion at index 0 would work in Chrome in stylesheets that use those but not @import rules, while they would fail in Firefox and IE (or at least fail in the @namespace case in IE; IE seems to not represent @charset in the CSSOM).

This interaction with @-rules means that insertRule at 0 is a huge footgun.  From an API design standpoint, I would be much happier if we had an appendRule on CSSStyleSheet, because that would actually be safe to call, unlike insertRule(..., 0).

Back to the original question, though, I guess I'd be ok with doing it if browsers first align on the cases in which insertRule() throws.  Otherwise it feels like we're promoting a pattern which throws in some but not all browsers in very non-obvious ways...

I'm sorry, I didn't actually read all of https://code.google.com/p/chromium/issues/detail?id=319695 myself before filing this bug.

Can this single jQuery plugin really be behind 0.1% of page views? Maybe, I guess getting rid of WebkitAnimationName is a fair way to find out.

Sadly, the use of -webkit-animation-name is very high:
https://www.chromestatus.com/metrics/css/timeline/popularity/172

And unprefixed CSS animations have not been shipped yet :/

Experimental support was added in https://codereview.chromium.org/22925002/ so I'll ask Alexis about the status.

> Can this single jQuery plugin really be behind 0.1% of page views?

Could a use counter be added for that?  Hard to tell, I guess, since it might end up with random filenames.

> Sadly, the use of -webkit-animation-name is very high:

I expect it would be possible to drop WebkitAnimationName without dropping -webkit-animation-name if needed.  But the fact that unprefixed hasn't shipped yet is a bigger issue, agree.

(In reply to Boris Zbarsky from comment #4)
> > Can this single jQuery plugin really be behind 0.1% of page views?
> 
> Could a use counter be added for that?  Hard to tell, I guess, since it
> might end up with random filenames.

That would be tricky. One way would be to see if WebkitAnimationName has already been accessed in the same script execution, but we don't have any existing use counters like that. It would be useful in many cases, though.

> > Sadly, the use of -webkit-animation-name is very high:
> 
> I expect it would be possible to drop WebkitAnimationName without dropping
> -webkit-animation-name if needed.  But the fact that unprefixed hasn't
> shipped yet is a bigger issue, agree.

Yeah. I'll wait to hear back from Alexis, if unprefixing CSS Animations is in reach it seems best to wait for that.

OK.

Note that it would still be good if UAs would align insertRule behavior, because it's still a nasty footgun...

Is there a tracking bug for that?

Note per Simon's request I added a use counter for CSSCharsetRule.encoding a while ago:
https://codereview.chromium.org/299443014
https://www.chromestatus.com/metrics/feature/timeline/popularity/426

Blink doesn't have CSSNamespaceRule at all, so kicking @namespace and @charset out of CSSOM and implementations seems feasible.

Are there other facets of insertRule interop that I'm missing?

BTW, it seems plausible that making insertRule(rule) with no index append instead of prepend would work, if that would be less risky for others to adopt.

> Is there a tracking bug for that?

I don't know.

> so kicking @namespace and @charset out of CSSOM and implementations seems
> feasible.

I'm 99% sure there is stuff depending on existence of @namespace rules, at least (e.g. editors built on top of browser engines).  Not web content, but not things we really want to break nevertheless.  At least in my opinion.

> Are there other facets of insertRule interop that I'm missing?

I don't know.  There's clearly no set of decent exhaustive tests for this that has been run cross-browser...

> it seems plausible that making insertRule(rule) with no index append
> instead of prepend would work

Maybe.  What should insertRule(rule, undefined) do?  Right now it prepends in all UAs, right?

I mean, we could make that work differently from insertRule(rule) with the second argument omitted, but that's not all that desirable; we should only do that as a last resort.  Especially if we're only doing it as a workaround for WebkitAnimationName existing, since the long-term goal is for that to not exist.

(In reply to Boris Zbarsky from comment #8)
> > Is there a tracking bug for that?
> 
> I don't know.

Simon, do you know which spec and browser bugs are relevant to this?

> > so kicking @namespace and @charset out of CSSOM and implementations seems
> > feasible.
> 
> I'm 99% sure there is stuff depending on existence of @namespace rules, at
> least (e.g. editors built on top of browser engines).  Not web content, but
> not things we really want to break nevertheless.  At least in my opinion.

Here's a quick test:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3314

It looks like WebKit/Blink are the only to not represent it. Blink bug:
https://code.google.com/p/chromium/issues/detail?id=389549

Gecko doesn't have the CSSNamespaceRule interface, but instead CSSNameSpaceRule...

Simon, do you care whether CSSNamespaceRule lives or dies? Is it in the same bucket as CSSCharsetRule or does it actually make sense?

> > it seems plausible that making insertRule(rule) with no index append
> > instead of prepend would work
> 
> Maybe.  What should insertRule(rule, undefined) do?  Right now it prepends
> in all UAs, right?

Per WebIDL trailing undefined arguments are treated as missing arguments, so I don't think this will be a problem.

> Per WebIDL trailing undefined arguments are treated as missing arguments

Right, so if we make insertRule(rule) append, that would make insertRule(rule, undefined) also append unless we took some special pains to avoid that (involving overloads that check the arg count).  Which is not what it does in any browser right now.

(In reply to Philip Jägenstedt from comment #9)
> Simon, do you know which spec and browser bugs are relevant to this?

I'm aware of this thread which is somewhat related
http://lists.w3.org/Archives/Public/www-style/2014Mar/0441.html

> Simon, do you care whether CSSNamespaceRule lives or dies? Is it in the same
> bucket as CSSCharsetRule or does it actually make sense?

I think it's not in the same bucket as @charset. If you want to build a stylesheet with namespaces you have to use @namespace. Not supporting it in CSSOM but supporting it in syntax means you can't use the API but you can use <style>.textContent, which seems silly.

(In reply to Boris Zbarsky from comment #10)
> > Per WebIDL trailing undefined arguments are treated as missing arguments
> 
> Right, so if we make insertRule(rule) append, that would make
> insertRule(rule, undefined) also append unless we took some special pains to
> avoid that (involving overloads that check the arg count).  Which is not
> what it does in any browser right now.

Oh, right you are.

OK, so I guess this is what needs to happen in Blink:

1. Support CSSNamespaceRule.
2. Unprefix CSS Animations.
3. See what happens to the CSSStyleSheetInsertRuleOptionalArg counter.

If any browser has a compat problem because of the optional argument before that, I'm of course in favor of changing the spec.

Also, we should figure out what to do with CSSCharsetRule:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27422

Something interesting came up while I was poking at the non-standard CSSStyleSheet.addRule that IE introduced and WebKit copied.

In IE it has two mandatory arguments while the index argument is optional. If left out, it default to appending, not prepending. It would make sense to do the same for insertRule, in case we end up having to standardize addRule:
https://www.chromestatus.com/metrics/feature/timeline/popularity/220

Again, that requires treating "missing" and "undefined" differently, yes?

I think it would be spec'd as addRule(DOMString selector, DOMString style, optional unsigned long index) with no default value for index. undefined vs. missing argument wouldn't be an issue because nobody has shipped addRule with a non-optional index argument, I assume.

It's not a given that addRule will have to be standardized, making insertRule append would just be a precaution, and it's not crazy independent of addRule either.

> because nobody has shipped addRule with a non-optional index argument

I think you have this backwards.

What matters is precisely that the index argument is optional in UAs that ship this API and that those UAs don't actually implement the Web IDL spec around optional arguments and undefined last I checked, so in those browsers this call:

  addRule(foo, bar);

will append while this call:

  addRule(foo, bar, undefined);

will do the same thing as this call:

  addRule(foo, bar, 0);

Please do correct me if I'm wrong on this, by the way.

If I am not wrong, then the proposal that we make insertRule behave like addRule precisely corresponds to specifying that it needs to treat missing and undefined differently, no?

You're right, until WebIDL's overload resolution algorithm is implemented correctly addRule(foo, bar, undefined) would behave like addRule(foo, bar, 0).

In Blink, "Treat undefined as missing for optional arguments with defaults" has landed:
https://code.google.com/p/chromium/issues/detail?id=335871

That isn't relevant to addRule case, but that's the bug to follow if you're interested.

I'm not worried about compat issues around insertRule(rule, undefined), and it seems like Blink would be at greater risk, since inserting at 0 can throw an exception.

> I'm not worried about compat issues

I am, if browsers start shipping different behavior.  If everyone aligns on undefined == missing == append, I'm less worried.

(In reply to Boris Zbarsky from comment #19)
> > I'm not worried about compat issues
> 
> I am, if browsers start shipping different behavior.  If everyone aligns on
> undefined == missing == append, I'm less worried.

I take it Gecko already implements WebIDL's overload resolution algorithm?

By the time the things in comment #13 have happened maybe it will have happened in Blink and the point will be moot. We'll see.

> I take it Gecko already implements WebIDL's overload resolution algorithm?

Yes, and has since Firefox 28.

That's great! Off-topically, did you get many regressions from content where f(x, undefined) changed behavior?

The only cases where that causes a problem is when the missing case is treated different from a "falsey" case or from explicit "undefined".

We found two instances of that in specs, leading to <https://www.w3.org/Bugs/Public/show_bug.cgi?id=25686> and <https://www.w3.org/Bugs/Public/show_bug.cgi?id=23565>, which both resulted in spec changes (the former to explicitly treat undefined and missing differently, the latter to change the missing behavior to be the falsey behavior.

There was also an issue in window.open() for the feature string; it used to become "undefined" and now becomes "".  The behavior of both is not specified, so it's hard to tell what the compat behavior is; it depends on what the UA does with those two values.

Alexis found the tracking bug for unprefixing CSS Animations in Blink:
https://code.google.com/p/chromium/issues/detail?id=154771

(In reply to Boris Zbarsky from comment #23)
> The only cases where that causes a problem is when the missing case is
> treated different from a "falsey" case or from explicit "undefined".
> 
> We found two instances of that in specs, leading to
> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=25686> and
> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=23565>, which both resulted
> in spec changes (the former to explicitly treat undefined and missing
> differently, the latter to change the missing behavior to be the falsey
> behavior.

Thanks! Both alert() and cloneNode() seem to be in order in Blink.

> There was also an issue in window.open() for the feature string; it used to
> become "undefined" and now becomes "".  The behavior of both is not
> specified, so it's hard to tell what the compat behavior is; it depends on
> what the UA does with those two values.

That's unfortunate :(

(In reply to Boris Zbarsky from comment #23)
> There was also an issue in window.open() for the feature string; it used to
> become "undefined" and now becomes "".  The behavior of both is not
> specified, so it's hard to tell what the compat behavior is; it depends on
> what the UA does with those two values.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25685 seems related.

There's also scrollIntoView which is bending over backwards for undefined. I don't recall if someone tried shipping without the special-case and found it broke sites or not, though.

http://dev.w3.org/csswg/cssom-view/#dom-element-scrollintoview
http://lists.w3.org/Archives/Public/www-style/2014Jul/0510.html

Also XMLHttpRequest#open async argument.

I think there were more things but I don't recall.

> There's also scrollIntoView which is bending over backwards for undefined.

Ah, yes, sorry.  When we ran into the cloneNode issue I just looked for the "boolean foo = true" pattern, and scrollIntoView was the other example I found.  So I proactively didn't change the behavior there and hence we never had bug reports about it, which is why I didn't notice it when writing comment 23.  :(

Gecko has never shipped a behavior change to scrollIntoView, so I can't guarantee that undefined is ever passed to it in the wild.

Usage of this doesn't seem to be going down, at least not far enough to start throwing exceptions:
https://www.chromestatus.com/metrics/feature/timeline/popularity/198

https://compat.spec.whatwg.org/#css-properties now has -webkit-animation-name and Gecko supports element.style.WebkitAnimationName so I think it'd be best to just make this argument optional now to minimize risk for all.

So which specific behavior are you proposing?  Has Blink fixed its CSSOM bugs wrt namespace rules and such?  Or are you proposing the append behavior?  With or without explicit undefined acting differently?

I was thinking just "optional unsigned long index = 0" with undefined meaning missing as for all optional arguments, and no other changes. In other words, prepend.

CSSNamespaceRule was added to Blink in https://bugs.chromium.org/p/chromium/issues/detail?id=389549 and CSSCharsetRule was removed from Gecko in https://bugzilla.mozilla.org/show_bug.cgi?id=1148694, were there other things that might go wrong if Gecko makes the argument non-optional to sort out?

No, that seems like it should make it safe to make the argument optional, in the sense of everyone ending up with the same behavior as a result.  That's great!

https://github.com/w3c/csswg-drafts/commit/7949d41a2d86107f8ad4624c055b4b0c9c28ad0d

Are there web platform tests for the change?  Bug reports on UAs?

Issue to write tests:
https://github.com/w3c/web-platform-tests/issues/5524

Have not yet filed bugs for browsers.