27384 2014-11-20 22:38:57 +0000 CSSStyleSheet.insertRule's index argument should be optional with default 0 2017-04-10 14:46:41 +0000 1 1 1 Unclassified CSS CSSOM unspecified PC All RESOLVED FIXED P2 normal --- 1 philipj zcorpan bzbarsky philip travil public-css-bugzilla oldest_to_newest 115263 0 philipj 2014-11-20 22:38:57 +0000 http://dev.w3.org/csswg/cssom/#cssstylesheet It's optional with default 0 in WebKit and Blink. There was an attempt to make it non-optional but it didn't work out: https://code.google.com/p/chromium/issues/detail?id=319695 When the removal was reverted, a use counter and deprecation message was also added: https://www.chromestatus.com/metrics/feature/timeline/popularity/198 This has been in since M32 (reaching stable in January 2014) but there's no sign of decreasing usage. Gecko/IE throw an exception if the argument is missing, so maybe it's on WebKit-specific paths. Maybe there are some sites that would start working in Gecko/IE if this is spec'd and they make it optional. 115273 1 zcorpan 2014-11-21 09:43:01 +0000 Are you OK with changing this in Gecko/IE? Should this be only for CSSStyleSheet#insertRule or also CSSGroupingRule#insertRule? 115298 2 bzbarsky 2014-11-21 15:33:06 +0000 > 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... 115377 3 philipj 2014-11-24 11:52:20 +0000 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. 115381 4 bzbarsky 2014-11-24 14:27:24 +0000 > 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. 115382 5 philipj 2014-11-24 14:34:32 +0000 (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. 115385 6 bzbarsky 2014-11-24 14:42:04 +0000 OK. Note that it would still be good if UAs would align insertRule behavior, because it's still a nasty footgun... 115386 7 philipj 2014-11-24 14:51:29 +0000 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. 115391 8 bzbarsky 2014-11-24 15:17:22 +0000 > 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. 115396 9 philipj 2014-11-24 15:48:04 +0000 (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. 115397 10 bzbarsky 2014-11-24 16:06:06 +0000 > 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. 115429 11 zcorpan 2014-11-24 21:20:29 +0000 (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. 115441 12 philipj 2014-11-25 08:34:51 +0000 (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. 115442 13 philipj 2014-11-25 09:04:22 +0000 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 115508 14 philipj 2014-11-26 13:28:05 +0000 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 115522 15 bzbarsky 2014-11-26 15:18:51 +0000 Again, that requires treating "missing" and "undefined" differently, yes? 115527 16 philipj 2014-11-26 15:43:45 +0000 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. 115528 17 bzbarsky 2014-11-26 15:50:28 +0000 > 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? 115530 18 philipj 2014-11-26 16:07:19 +0000 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. 115531 19 bzbarsky 2014-11-26 16:19:46 +0000 > 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. 115535 20 philipj 2014-11-26 19:03:40 +0000 (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. 115537 21 bzbarsky 2014-11-26 19:08:54 +0000 > I take it Gecko already implements WebIDL's overload resolution algorithm? Yes, and has since Firefox 28. 115539 22 philipj 2014-11-26 19:13:05 +0000 That's great! Off-topically, did you get many regressions from content where f(x, undefined) changed behavior? 115546 23 bzbarsky 2014-11-26 19:22:39 +0000 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. 115561 24 philipj 2014-11-26 20:43:49 +0000 Alexis found the tracking bug for unprefixing CSS Animations in Blink: https://code.google.com/p/chromium/issues/detail?id=154771 115562 25 philipj 2014-11-26 20:55:31 +0000 (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 :( 115609 26 zcorpan 2014-11-27 10:04:36 +0000 (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. 115624 27 bzbarsky 2014-11-27 14:43:06 +0000 > 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. 127704 28 philip 2016-10-10 18:59:14 +0000 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. 128484 29 bzbarsky 2017-03-13 20:46:29 +0000 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? 128509 30 philip 2017-03-28 12:04:28 +0000 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? 128510 31 bzbarsky 2017-03-28 12:49:27 +0000 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! 128526 32 zcorpan 2017-04-10 14:43:19 +0000 https://github.com/w3c/csswg-drafts/commit/7949d41a2d86107f8ad4624c055b4b0c9c28ad0d 128527 33 bzbarsky 2017-04-10 14:45:01 +0000 Are there web platform tests for the change? Bug reports on UAs? 128528 34 zcorpan 2017-04-10 14:46:41 +0000 Issue to write tests: https://github.com/w3c/web-platform-tests/issues/5524 Have not yet filed bugs for browsers.