whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.11k stars 2.67k forks source link

<link rel="stylesheet" disabled> #3840

Closed emilio closed 4 years ago

emilio commented 6 years ago

Consider the following test-case:

<!doctype html>
<link rel="stylesheet" disabled href="data:text/css,html { background: green }">
<div id="log"></div>
<script>
window.onload = function() {
  log.innerHTML = document.styleSheets.length;
}
</script>

Current browser behavior is:

As far as I can tell, the specified behavior is Firefox's. But that being said, Firefox supports doing link.disabled = true and toggling the disabled flag on the stylesheet, even though it's not in the webidl.

Should the disabled attribute affect whether the sheet is enabled or not? If not, could we get that clarified?

Should disabled be in HTMLLinkElements IDL? 'disabled' in HTMLLinkElement.prototype returns true everywhere.

Should a disabled sheet appear in document.styleSheets? Blink and WebKit don't remove it if you toggle the attribute, which looks definitely like a bug to me.

I think I'd expect either Firefox's behavior (disabled attribute has no effect, disabled via the IDL tweaks the disabled flag of the sheet (effectively forwards setting and getting to the sheet), or Edge's behavior, where the disabled attribute presumably reflects sheet.disabled (though note I haven't checked this in practice, chances are that link.sheet.disabled = true doesn't affect link.hasAttribute("disabled")) and stuff like that.

cc @bzbarsky @annevk @fremycompany @lilles @ericwilligers

emilio commented 6 years ago

I guess this changed in https://github.com/whatwg/html/commit/cc5fa75c35354f7438327bbd816a2adf054a9379...

emilio commented 6 years ago

I think it is fine to leave this unchanged spec-wise at least until there's more compat data. https://bugs.chromium.org/p/chromium/issues/detail?id=695984 was already on file to remove the attribute from Blink.

I can try to remove the WebIDL from Firefox on Nightly or something and see if there's fallout as well.

domenic commented 6 years ago

This area was last investigated in https://github.com/whatwg/html/issues/1081, FWIW.

emilio commented 6 years ago

cc @foolip, do you think it'd be reasonable to remove the disabled attribute from Blink?

The use counters look somewhat high: https://www.chromestatus.com/metrics/feature/timeline/popularity/809

But Blink's behavior is clearly bogus and not something we'd want to implement, in the sense that it's not on document.styleSheets list if the attribute is initially specified in the markup, but it is if it's dynamically toggled back and forth.

Alternatively, we should decide whether disabled links appear on document.styleSheets, whether they trigger loads (they don't seem to in Blink), etc. I'm happy to do that too but it seems fairly more work.

emilio commented 6 years ago

Also it's really weird that link.disabled = true; would do something completely different from link.sheet.disabled = true...

upsuper commented 6 years ago

I would slightly hope that we don't add it, but if Blink is unwilling to remove it and it'd be required by webcompat, I guess it's fine to add... We should probably sync this carefully with the existing disabled state in CSSOM.

emilio commented 6 years ago

I think if we spec it it may be worth to prevent it from triggering loads and appearing in document.styleSheets. At least that's the only useful thing I would expect it to be used for...

That'd be consistent with current Blink / WebKit's behavior when there's no dynamic fiddling: link.sheet == null, document.styleSheets doesn't contain it, plus it prevents the weird synchronization problem altogether.

Though I'd too prefer not having to do it as well, it'd be somewhat confusing either way.

foolip commented 6 years ago

I've noticed this from time to time but never known quite what to make of it, or what the compat constraints are. In https://bugs.chromium.org/p/chromium/issues/detail?id=695984&desc=2#c13 I wrote:

Concrete next step for this issue: Investigate in more detail what these IDL attributes do in each engine. One oddity to look at is the fact that HTMLLinkElement#disabled is a reflected attribute in Blink, but HTMLStyleElement#disabled and SVGStyleElement#disabled are bare IDL attributes.

The oddity here is that reflecting the content attribute doesn't mix with forwarding to the element's sheet, and Blink is inconsistent in what it does. If the attributes are reflected, then it will be possible for them to be out of sync with the real disabled state of the sheets, unless setting link.sheet.disabled should also add/remove the disabled content attribute on the link element, which would be quite unusual. (Mentioned by @emilio above re Edge's behavior too.)

There are also more related use counters:

All of them have fairly high usage, but that doesn't say too much about the web compat constraints.

Concrete options that come to mind:

In both cases, what to do if this.sheet is null when the content attribute is added, does it prevent loading, or does it cause the sheet to be disabled when created?

bzbarsky commented 6 years ago

Should disabled be in HTMLLinkElements IDL?

Yes. Afaik sites use it, and @foolip's claim about use counters agrees. I don't understand why it was ever removed from the spec. :(

I did some quick testing, and setting .disabled in Blink on an existing HTMLLinkElement in the DOM that already has a sheet does not drop the sheet but does set the sheet's disabled.

My personal, no-hysteresis, preference is that the content attribute does nothing and the DOM attribute forwards to the underlying sheet, if any... I admit that I'm biased, since that's what Gecko implements.

One comment about the above use counter data. https://www.chromestatus.com/metrics/feature/timeline/popularity/809 includes all the sites setting .disabled, due to the reflection that Blink does, as far as I can tell. So it tells us nothing about whether people are actually using the content attribute, right? Or am I totally misreading the code?

bzbarsky commented 6 years ago

I guess https://github.com/whatwg/html/issues/1081#issuecomment-216161576 talks a bit about extracting signal from a combination of that use counter and the V8HTMLLinkElement_Disabled_AttributeSetter use counter...

foolip commented 6 years ago

One comment about the above use counter data. https://www.chromestatus.com/metrics/feature/timeline/popularity/809 includes all the sites setting .disabled, due to the reflection that Blink does, as far as I can tell. So it tells us nothing about whether people are actually using the content attribute, right? Or am I totally misreading the code?

No, you're right about that, when https://www.chromestatus.com/metrics/feature/timeline/popularity/811 is triggered, https://www.chromestatus.com/metrics/feature/timeline/popularity/809 will be too. They're in a very similar range, so most likely the content attribute counter is dominated by the IDL attribute being said. (It'd be consistent with the data that pages are doing both, but less plausible.)

I'll update the use counter to measure the parser case separately.

foolip commented 6 years ago

Sent https://chromium-review.googlesource.com/c/chromium/src/+/1148203, but honestly I'm not too optimistic about this alone telling us whether the content attribute is needed for real-world compat.

I did a quick httparchive search for the regexp <link[^>]{1,100} disabled and that only returned 74 matches:

page,url
http://www.hacontent.com/,http://www.aon.com/human-capital-consulting/default.jsp
http://www.waxfilm.net/,http://xxxx.re/
http://www.manufaktura.com/,http://www.manufaktura.com/
http://www.trl.org/,http://www.trl.org/
http://www.isiment.com/,http://www.isiment.com/
http://www.aon.com/,http://www.aon.com/default.jsp
http://www.skiphop.com/,http://www.skiphop.com/
http://www.skiphop.com/,http://www.skiphop.com/?callback=located&_=1517679261400
http://www.skiphop.com/,http://www.skiphop.com/home?id=skiphop&_=1517679261400&callback=located
http://www.ksepb.gov.tw/,http://www.ksepb.gov.tw/
http://www.ordistricts.nic.in/,http://www.ordistricts.nic.in/
http://www.maarefquran.org/,http://www.maarefquran.org/
http://www.mosconsv.ru/,http://www.mosconsv.ru/
http://www.otsuka-plus1.com/,http://www.otsuka-plus1.com/
http://www.apkpro.ru/,http://www.apkpro.ru/
http://www.dxbsky.com/,http://www.dxbsky.com/
http://www.supadipa.com/,http://www.supadipa.com/
http://www.learncodeonline.in/,http://www.learncodeonline.in/
http://www.clien.net/,http://www.clien.net/
http://www.funai.gov.br/,http://www.funai.gov.br/
http://www.oshkosh.com/,http://www.oshkosh.com/
http://www.nomade.kr/,http://www.nomade.kr/
http://www.rev.cn/,http://www.rev.cn/
http://www.rev.cn/,http://www.rev.cn/js/header.js
http://www.shasm.gov.cn/,http://www.shasm.gov.cn/JScript.js
http://www.xn--80aaaac8algcbgbck3fl0q.xn--p1ai/,http://www.xn--80aaaac8algcbgbck3fl0q.xn--p1ai/
http://www.scorecardrewards.com/,https://www.scorecardrewards.com/assets/hinda-0.0.1.11756.js
http://www.gate.io/,http://www.gate.io/
http://www.gate.io/,https://gate.io/
http://www.cpcb.nic.in/,http://www.cpcb.nic.in/
http://www.aon.fr/,http://www.aon.fr/france/default.jsp
http://www.dajs.gov.cn/,http://www.dajs.gov.cn/
http://www.chilicel.com/,http://www.chilicel.com/
http://www.steamspy.com/,http://www.steamspy.com/
http://www.hainanairlines.com/,http://www.hainanairlines.com/distil_identify_cookie.html?httpReferrer=%2F&uid=8FD9C2E9-ED92-3AD1-97CC-88143764D73C
http://www.myrunningman.com/,http://www.myrunningman.com/
http://www.sanwa-p.co.jp/,https://www.sanwa-p.co.jp/js/jquery.font.js
http://www.teenarea.biz/,http://www.teenarea.biz/
http://www.wlnupdates.com/,http://www.wlnupdates.com/
http://www.yourtotalrewards.com/,http://www.aon.com/human-capital-consulting/default.jsp
http://www.cbenni.com/,http://www.cbenni.com/
http://www.mphro.com/,http://www.aon.com/human-capital-consulting/default.jsp
http://www.kkesh.med.sa/,http://www.kkesh.med.sa/
http://www.connected2.me/,http://www.connected2.me/
http://www.microfocus.net/,http://www.microfocus.net/
http://www.ficpa.org/,http://www.ficpa.org/
http://www.supeera.com/,http://www.supeera.com/
http://www.pesc.ru/,http://www.pesc.ru/
http://www.synchroarts.com/,http://www.synchroarts.com/
http://www.dxbpp.gov.ae/,http://www.dxbpp.gov.ae/
http://www.vodafonefreezone.com/,http://www.vodafonefreezone.com/
http://www.tmbam.com/,https://www.tmbam.com/home/th/inc/setFont.js
http://www.jnu.ac.in/,http://www.jnu.ac.in/
http://www.bemlindia.com/,http://www.bemlindia.com/
http://www.fcijobportalmah.com/,http://fcijobportalmah.com/FCI01/js/FontSizes.js
http://www.aon.it/,http://www.aon.com/italy
http://www.mkb.hu/,http://www.mkb.hu/
http://www.carters.com/,http://www.carters.com/
http://www.szhr.com.cn/,http://www.szhr.com.cn/hrmall/index-center.html
http://www.szhr.com.cn/,http://www.szhr.com.cn/hrmall/index_ad_item_hr.html
http://www.alsok.co.jp/,http://www.alsok.co.jp/
http://www.wum.edu.pl/,http://www.wum.edu.pl/themes/frameworkNOWY/kontrast.js?V
http://www.wum.edu.pl/,http://www.wum.edu.pl/modules/styleswitcher/styleswitcher.js
http://www.cartersoshkosh.ca/,http://www.cartersoshkosh.ca/
http://www.cartersoshkosh.ca/,http://www.cartersoshkosh.ca/?callback=located&_=1517713421387
http://www.cartersoshkosh.ca/,http://www.cartersoshkosh.ca/on/demandware.store/Sites-CartersCA-Site/en_CA/Default-Start?callback=located&_=1517713421387
http://www.hewitt.com/,http://www.aon.com/human-capital-consulting/default.jsp
http://www.jeld-wen.com/,http://www.jeld-wen.com/
http://www.dotajackpots.com/,http://www.dotajackpots.com/
http://www.cvag.de/,http://www.cvag.de/
http://www.triptogether.com/,http://www.triptogether.com/
http://www.oddistricts.nic.in/,http://www.oddistricts.nic.in/
http://www.previdencia.gov.br/,http://www.previdencia.gov.br/
http://www.bjft.gov.cn/,http://www.bjft.gov.cn/top_lan.html

That's not a lot, but there is some stuff in there that looks like deliberate use. On https://www.ksepb.gov.tw/ the disabled attribute is everywhere, and then there's some scripts that poke at the IDL attributes too. Unfortunately, this page doesn't load in Firefox at all due to an invalid cert, so I can't check if it's broken or not.

Spending a couple of hours on this list and comparing the result in a Chromium build with or without the disabled content attribute supported should give a good idea about the risks.

Before I go spend a bunch of time on that I'd like to hear if the Edge or WebKit team have thoughts on this. @patrickkettner @cdumez, can you find contacts?

foolip commented 6 years ago

Looking a bit more, there's a layer between the disabled content attribute and underlying sheet which would explain the differences in document.styleSheets: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/link_style.cc?l=204&rcl=9a429d1f18c5b2713ab8bad9f795217efe0d7bcd

This dates back to 2002/2003 when Dave Hyatt exclaimed that "This disabling stuff is complicated!" https://trac.webkit.org/changeset/2318/webkit https://trac.webkit.org/changeset/4010/webkit

Because of the reflection, this stuff is in play when setting the disabled IDL attribute on HTMLLinkElement as well.

emilio commented 5 years ago

Here's a concrete proposal: Make <link rel="stylesheet" disabled> work by not loading the stylesheet, and by not adding the stylesheet to document.styleSheets, instead of forwarding to the stylesheet's disabled attribute.

This is a nice way of preventing stylesheet loads, and is the sanest thing I could think of, since <style>'s disabled getters and setters do the same in every browser.

@foolip does that sound reasonable? It means that using the getter and setter produces the same result rendering-wise, and consistent results regardless of whether the attribute was set from the parser or not.

emilio commented 5 years ago

Ugh, the only reason that people use disabled is because it's the only way to enable an alternate stylesheet in Blink / WebKit... ;_;

emilio commented 5 years ago

So, ok, a bit more of an elaborated answer (I'll use WebKit and Blink interchangeably here).

We (Gecko) get compat issues from not supporting <link rel="stylesheet" disabled>. The Blink model for this is quite weird as described above.

This attribute is mostly used in along with <link rel="alternate stylesheet">, as a work-around to enable alternate stylesheets in WebKit. In WebKit you cannot enable an alternate sheet unless it's gotten explicitly disabled before. This is because WebKit doesn't model alternate stylesheets like the spec says (in terms of the disabled state), and in order to get to apply the stylesheet properly.

For example, there's no other way to get the stylesheet as follows to apply in WebKit other than changing the rel attribute or toggling link.disabled back and forth:

<link rel="alternate stylesheet" href="data:text/css,html { background: green }">

So for convenience, and in order to be able to toggle alternate stylesheets across browsers, people do the following:

<link rel="alternate stylesheet" href="data:text/css,html { background: green }" disabled>

So that they can enable the stylesheet using link.disabled = false, and disable it using link.disabled = true in both Gecko / Edge and WebKit.

There are a whole lot of extra issues in WebKit implementation, like the stylesheet inconsistently appearing in document.styleSheets.

Proposed model

The model I proposed above tries to get the benefits of the current disabled implementation in WebKit (avoids the stylesheet load when the attribute is present), while trying to make it saner. However as-is it would break the "enabling alternate stylesheet" case. So I propose adopting that model with a slightly different change to make it compatible with that use-case:

So with that, the proposal is:

Does that sound reasonable to everybody?

// cc @lilles @rniwa

bzbarsky commented 5 years ago

"reasonable" is a high bar, but if WebKit/Blink are not going to fix their bugs around alternate stylesheets this sounds like the next-best option to me...

emilio commented 5 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1281135 has a patch implementing that on Gecko, fwiw.

rniwa commented 5 years ago

I agree we should do something to improve interoperability here.

@emilio : When you say "WebKit" in https://github.com/whatwg/html/issues/3840#issuecomment-481034206 you mean both WebKit and Blink? If so, changing the behavior of WebKit and Blink to match the spec might pose a compatibility risk.

In particular, removing & adding an item from document.styleSheets seems risky to me because some people might relying on finding a given stylesheet with a given index value even after disabling one.

@smfr @hober

emilio commented 5 years ago

@emilio : When you say "WebKit" in #3840 (comment) you mean both WebKit and Blink? If so, changing the behavior of WebKit and Blink to match the spec might pose a compatibility risk.

Yes.

In particular, removing & adding an item from document.styleSheets seems risky to me because some people might relying on finding a given stylesheet with a given index value even after disabling one.

Right now the WebKit / Blink behavior doesn't really make much sense. If you use <link rel="stylesheet" disabled> it doesn't appear in document.styleSheets, if you unset and set the attribute again, then it magically appears. That doesn't make sense to me.

I think it's also a bug that in WebKit / Blink you cannot enable an alternate stylesheet without the disabled attribute: <link rel="alternate stylesheet" href="foo.css"> cannot be enabled, unless you add the disabled attribute and then remove it.

I think what I propose is a good way to get interop with a somewhat consistent model. But I'm all ears if you have better ideas. Given the poor interop situation here I think we have some leeway...

a2sheppy commented 5 years ago

With a patch into Firefox for Firefox 68 to implement this proposal, I am looking at this from a documentation perspective. The spec change has not yet landed, and reading the review comments suggests there will be some tweaks made, although I don't think they'll affect developer docs at all. But as a general policy, we don't update the compatibility database and add items to the interface member database until the change is actually merged to the master branch of the spec.

Do we have any idea of a timeframe in which that might be likely to happen here? Is this something that will happen in a matter of days or weeks?

cc @emilio and @annevk

Zibri commented 4 years ago

also,

<style disabled="true">
...
</style>

would be really nice.

annevk commented 4 years ago

@a2sheppy apologies for the delay here, there's still a couple things to sort out, but I hope to make some progress again on this.

@Zibri that would be a new feature that's probably best filed as a new issue.

Looking at the tests, currently Chrome/Safari fail half of 001 and all of 002. Chrome also fails 007. The tests are located at css/cssom/HTMLLinkElement-disabled* in WPT. In particular Chrome/Safari don't null out ownerNode for a disabled style sheet. And Chrome does something weird around disabled attribute reflection.

@emilio https://drafts.csswg.org/cssom/#css-style-sheets doesn't currently say that owner node is supposed to be null when the style sheet is disabled.

@tkent-google do you agree 007 is a bug in Chrome?

emilio commented 4 years ago

@emilio https://drafts.csswg.org/cssom/#css-style-sheets doesn't currently say that owner node is supposed to be null when the style sheet is disabled.

Yeah but this makes a disabled link not be the same as a disabled sheet.

tkent-google commented 4 years ago

@tkent-google do you agree 007 is a bug in Chrome?

I delegate to @mfreed7. I'm not responsible for HTML any longer.

annevk commented 4 years ago

@emilio there's no text for returning null when the owner is disabled either. But if that's done as part of adding the flag that's needed for #4519 I guess that's fine.

emilio commented 4 years ago

@annevk when the owner is disabled there's no stylesheet associated to the owner so there's no stylesheet to return null from anywhere.

mfreed7 commented 4 years ago

Sorry for the delay responding to this one. Generally, this sounds like a reasonable suggestion, and improving interop (and sanity) here sounds like a good thing. I'm a bit concerned about compat, given the longstanding weird behavior in Webkit/Blink. I understand that the interop issues might mean the compat issues aren't so bad, but probably not zero either.

Does the current Gecko behavior match https://github.com/whatwg/html/pull/4519?

emilio commented 4 years ago

Yes, Gecko behavior is tested in these WPTs. As you may note, Safari and Chrome are pretty close to green in those tests.

mfreed7 commented 4 years ago

Thanks. I have a patch ready that implements the new behavior in Chromium, which I'll try to land soon in M85 and see if there are any reported compat bugs.

Thanks for writing the tests for this - made implementing it easy.

domenic commented 4 years ago

Hooraaay!

Shall we hold off on landing #4519 until Chromium gets some compat data? Or should we land it, since the current spec doesn't have <link disabled> at all, and consider if we need to change the behavior to Chromium/WebKit-style as a followup?

lilles commented 4 years ago

Yay! Thanks for doing this Mason!

mfreed7 commented 4 years ago

Shall we hold off on landing #4519 until Chromium gets some compat data? Or should we land it, since the current spec doesn't have <link disabled> at all, and consider if we need to change the behavior to Chromium/WebKit-style as a followup?

Either way is ok by me, but I suppose I'd prefer that it lands. As you mentioned, there's no spec and mixed implementations at the moment, so it seems better to land something and then tweak it if necessary. But clearly, it's up to you.