New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 690609 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit 17 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 674593


Show other hotlists

Hotlists containing this issue:
Non-Standard-IDL


Sign in to add a comment

Remove Document#selectedStylesheetSet/preferredStylesheetSet

Project Member Reported by lunalu@chromium.org, Feb 9 2017

Issue description

Currently in Blink Document.idl, we have:
attribute DOMString? selectedStylesheetSet;
readonly attribute DOMString? preferredStylesheetSet;

But in the spec, it has:
[SameObject] readonly attribute StyleSheetList styleSheets;

Should we do something to match the spec, either update the spec or change our implementation? 
 
Labels: Needs-Feedback
Can you please link the name and version of spec that you're referring to?

Comment 2 by lunalu@chromium.org, Feb 13 2017

https://drafts.csswg.org/cssom/#extensions-to-the-document-interface

This link is right above the "stylesheet" attributes in Blink Document.idl (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.idl?rcl=28ff32abc83ae8939f56702151b3d982859d56b5&l=145). If the link is no longer correct, please update the link. 

Thanks 
Cc: foolip@chromium.org

Comment 4 by meade@chromium.org, Apr 4 2017

Labels: -Needs-Feedback
Status: Available (was: Untriaged)
Labels: Hotlist-Interop Update-Quarterly
CL to add use counters for the currently shipped attributes:
https://chromium-review.googlesource.com/c/chromium/src/+/745322
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/708cfbd0e177072433ccf56307f0982101502af5

commit 708cfbd0e177072433ccf56307f0982101502af5
Author: Eric Willigers <ericwilligers@chromium.org>
Date: Tue Oct 31 08:02:23 2017

Document: Use counters for stylesheetSet attributes

We add use counters for the following attributes:
attribute DOMString? selectedStylesheetSet;
readonly attribute DOMString? preferredStylesheetSet;

BUG= 690600 , 690609 

Change-Id: I0edf7fc6ec5e3e9295d36f18ad5853ffabfa6c1e
Reviewed-on: https://chromium-review.googlesource.com/745322
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512775}
[add] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/LayoutTests/fast/dom/document-preferredStylesheetSet-use-counter.html
[add] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/LayoutTests/fast/dom/document-selectedStylesheetSet-use-counter-001.html
[add] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/LayoutTests/fast/dom/document-selectedStylesheetSet-use-counter-002.html
[modify] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/Source/core/dom/Document.idl
[modify] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/third_party/WebKit/public/platform/web_feature.mojom
[modify] https://crrev.com/708cfbd0e177072433ccf56307f0982101502af5/tools/metrics/histograms/enums.xml

Comment 9 by shend@chromium.org, Nov 27 2017

Labels: BlockedBug
Marking as BlockedBug as we need to wait for use counter data

Comment 10 by shend@chromium.org, Nov 27 2017

NextAction: 2018-04-01
I'm guessing we'll get enough data around April 2018.
Labels: -Update-Quarterly
The NextAction date has arrived: 2018-04-01
For the getters, it looks like we're at about 0.1% to 0.17% of page loads. For the setter of selectedStyleSheet, it bounces between 0% and probably one or two page loads.
cnardi@, what's your takeaway from those numbers? Will both the getter and setter be affected by matching the spec, and is there a way to estimate how much of that 0.17% of the getter might result in real breakage?
So the setter seems like something we could drop for sure, since (virtually) nothing uses it.

However, the getters are a more complicated situation it seems. The parts of the spec that our code claims to implement were removed in https://github.com/w3c/csswg-drafts/commit/e5353d078e686136fdb28c7de37ffd375cd6d79f, citing https://www.w3.org/Bugs/Public/show_bug.cgi?id=29471 and https://lists.w3.org/Archives/Public/www-style/2013Aug/0640.html. This notes that the parts of that removed section which were implemented by Blink/WebKit are non-standard in that they are preferredStylesheetSet and selectedStylesheetSet vs preferredStyleSheetSet and selectedStyleSheetSet (note the difference in the casing of stylesheet). It also seems like nothing has changed since that change, as Firefox still implements all of those methods per https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#296.

So to sum it up: we and WebKit still implement the non-standard preferredStylesheetSet and selectedStylesheetSet methods (but I'm not sure if the behavior of these non-standard methods aligns with the old spec). Firefox implements the standard methods which were removed in https://github.com/w3c/csswg-drafts/commit/e5353d078e686136fdb28c7de37ffd375cd6d79f. And Edge doesn't appear to implement any of these methods.
Firefox bug on removing these methods: https://bugzilla.mozilla.org/show_bug.cgi?id=1260720
(old) WebKit bug about the difference in the casing of "Stylesheet": https://bugs.webkit.org/show_bug.cgi?id=62407
Also note that some of my comments above would apply to  bug 690600  too, since I was making comments about both methods.
If you're looking into dropping all these, I could look into removing them from Firefox, I wasn't aware of that bug, but it allows me to clean up some related code :)
I think it makes sense to remove the two methods we implement, since usage seems to be fairly low. However, it would probably be good to verify that the breakage from removing them would be fairly minimal.
Summary: Remove Document#selectedStylesheetSet/preferredStylesheetSet (was: Make Document#selectedStylesheetSet match the spec)
Sent https://github.com/w3c/web-platform-tests/pull/10861 to create some failing tests.

selectedStylesheetSet also needs a use counter, and both need to be moved to the non-standard section of our document.idl.
We actually do have use counters for all of these attributes, the ones for selectedStylesheetSet are just separated out into getter/setter and thus not implemented in IDL.

https://www.chromestatus.com/metrics/feature/timeline/popularity/2211 - DocumentGetPreferredStylesheetSet
https://www.chromestatus.com/metrics/feature/timeline/popularity/2212 - DocumentGetSelectedStylesheetSet
https://www.chromestatus.com/metrics/feature/timeline/popularity/2213 - DocumentSetSelectedStylesheetSet

 Issue 690600  has been merged into this issue.
Ah, didn't realize that :)

I think that some httparchive analysis would be a reasonable next step, to figure out what the usage looks like. Some tips for that are here:
https://www.chromium.org/blink/platform-predictability/compat-tools
Philip, do you think you could help me with getting access to run httparchive queries on my Chromium account? Since I'm not a Googler, it doesn't let me, and I already ran one query this month on my personal account.
I don't think we have a way to hook up Chromium accounts with BigQuery billing, which is very unfortunate.

To move this along, I checked if the counters in #21 were hit on any site, and they were not. Used this query:
https://docs.google.com/document/d/1cpjWFoXBiuFYI4zb9I7wHs7uYZ0ntbOgLwH-mgqXdEM/edit#heading=h.3701s9nnsb27

I can get you a list of URLs with matches for selectedStylesheetSet and preferredStylesheetSet, would that help?
Yeah, that would probably help. Although if the counters weren't hit, there probably won't be that many matches for the attributes themselves. Thanks!
OK, there were actually incredibly few hits.

selectedStylesheetSet (https://bigquery.cloud.google.com/savedquery/762219082167:235fd43fbe7647ea94ec7b14498e173b):
http://www.instructorscorner.org/
http://www.deltadentalcoversme.com/
http://www.wellsfargocommunity.com/

preferredStylesheetSet (https://bigquery.cloud.google.com/savedquery/762219082167:26eff7fb22a147408e113694ec2dd17b):
http://www.pokolenie-x.com/
http://www.instructorscorner.org/
http://www.wellsfargocommunity.com/
http://www.deltadentalcoversme.com/

Looking into these might tell us something interesting about the risks of removal. But this really is exceptionally few results, so I think the 0.17% must be either to accidentally hitting the counter, or a single large-ish site. Both are good news that mean lower risk than what 0.17% "normally" means.
Delta Dental and Wells Fargo are two major US companies, so it would make sense that the use counts were inflated.

However, it doesn't seem like these sites actually use either selectedStylesheetSet or preferredStylesheetSet. The only hit I could find in either site was at https://www.deltadentalcoversme.com/s/sfsites/auraFW/javascript/1bO4dJePbDnoI-_VdhdsEQ/aura_prod.js and https://www.wellsfargocommunity.com/s/sfsites/auraFW/javascript/1bO4dJePbDnoI-_VdhdsEQ/aura_prod.js, but they're only referenced in a dictionary mapping some values to attributes on different prototypes. https://www.instructorscorner.org/s/sfsites/auraFW/javascript/1bO4dJePbDnoI-_VdhdsEQ/aura_proddebug.js appears to be an un-minified version of the same file.

The last site (http://www.pokolenie-x.com/) uses https://getfirebug.com/firebug-lite.js, which is only for debugging, and appears to be unmaintained.

Because of this, I don't believe there should be any compatibility risk from removing both attributes. I may have missed some use of selectedStylesheetSet or preferredStylesheetSet in the aura files, but from my viewing it looks like they are only referenced and not used.
Thanks for checking it out. If you'd like to send an intent to deprecate and remove, I'll like to say LGTM :)
Owner: cnardi@chromium.org
Status: Started (was: Available)
Philip, do you want to just go ahead with immediate removal per https://groups.google.com/a/chromium.org/d/msg/blink-dev/w1Bv7YZxAco/91dOdEkiCAAJ, or go with the deprecation route you suggested a couple messages before?
I would prefer removal without deprecation in this case. If we're right, almost all of the deprecation warnings will be bogus.
Project Member

Comment 32 by bugdroid1@chromium.org, May 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62e388ac4d084b73649d99df72e651559be44077

commit 62e388ac4d084b73649d99df72e651559be44077
Author: Chris Nardi <cnardi@chromium.org>
Date: Thu May 10 17:37:08 2018

Remove Document#selectedStylesheetSet/preferredStylesheetSet

Document#selectedStylesheetSet/preferredStylesheetSet are non-standard
methods that are only implemented by WebKit and Blink. Their standard
versions are no longer in the spec. Remove them entirely from our
implementation.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/w1Bv7YZxAco

Bug:  690609 
Change-Id: I796a6bc0693934eb308a4fe59b52e4a9d1269e6f
Reviewed-on: https://chromium-review.googlesource.com/1049051
Commit-Queue: Chris Nardi <cnardi@chromium.org>
Reviewed-by: Philip J├Ągenstedt <foolip@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557569}
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/external/wpt/css/cssom/historical-expected.txt
[add] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/external/wpt/css/cssom/preferred-stylesheet-order.html
[add] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/external/wpt/css/cssom/preferred-stylesheet-reversed-order.html
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/external/wpt/shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/test-011.html
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/fast/css/link-disabled-attr-expected.txt
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/fast/css/link-disabled-attr.html
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/css/preferred-stylesheet-order-expected.txt
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/css/preferred-stylesheet-order.html
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/css/preferred-stylesheet-reversed-order-expected.txt
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/css/preferred-stylesheet-reversed-order.html
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/fast/dom/document-attribute-js-null-expected.txt
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/fast/dom/document-attribute-js-null.html
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/dom/document-preferredStylesheetSet-use-counter.html
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/dom/document-selectedStylesheetSet-use-counter-001.html
[delete] https://crrev.com/1b5126a68c8620c18c515d5aa1e7bc61394c99af/third_party/WebKit/LayoutTests/fast/dom/document-selectedStylesheetSet-use-counter-002.html
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/public/platform/web_feature.mojom
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/renderer/core/css/style_engine.cc
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/renderer/core/css/style_engine.h
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/62e388ac4d084b73649d99df72e651559be44077/third_party/blink/renderer/core/dom/document.idl

Labels: -BlockedBug
NextAction: ----
Status: Fixed (was: Started)

Sign in to add a comment