-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add <link disabled> #4519
Add <link disabled> #4519
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilio here's a sketch, but I've mainly identified the places that need changing and did not thoroughly think through what needs to happen in each place.
source
Outdated
@@ -24936,7 +24947,8 @@ document.body.appendChild(wbr);</code></pre> | |||
<dd><p>null</p></dd> | |||
|
|||
<dt><span data-x="concept-css-style-sheet-disabeld-flag">disabled flag</span></dt> | |||
<dd><p>Left at its default value.</p></dd> | |||
<dd><p>Unset if <var>element</var>'s <span>explicitly enabled</span> is true, and left at its | |||
default value otherwise.</p></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also doesn't seem entirely correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I need to add a new flag to CSSOM instead.
heads-up @whatwg/documentation |
Potential documentation need recorded at https://trello.com/c/6YAMGL3B/132-html-link-element |
Tests: ... Fixes #3840, maybe.
79c033d
to
de4720d
Compare
I filed w3c/csswg-drafts#4978 as a follow-up here. If the current PR looks good I suggest we merge it next week somewhere to give some time for final concerns. |
I'm sad to ask, but is there multi-implementer interest here? |
Is your question specifically about #3840 (comment)? It would be good to hear from @mfreed7 and @cdumez, yeah. |
It was more general, but it sounds like you have done the work of narrowing it down to specific divergences, which is even better. In particular it sounds like for 001/002, if we wanted to go with the majority we'd have some different (probably worse) behavior than what is in this PR. But if we want to go with what's in this PR, then we should get at least one of Chrome and Safari to agree to change. |
cc @lilles, is fixing this in Chromium something you'd consider? Current alternate sheets are pretty broken there :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with potential improvement
<p>Removing the <code data-x="attr-link-disabled">disabled</code> attribute dynamically will | ||
fetch and apply the style sheet:</p> | ||
|
||
<pre><code class="html" data-x=""><link disabled rel="alternate stylesheet" href="css/pooh"></code></pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example should probably include script that removes the disabled attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean adding <script>document.querySelector("link").removeAttribute("disabled")</script>
? Seems a bit redundant to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly. Right now the intro text for the example seems pretty disconnected from the example itself; it talks about removing but the code under discussion does nothing of the sort.
Filed https://bugs.webkit.org/show_bug.cgi?id=212515 on WebKit. Chrome already has an intent to ship out so I think that's covered. |
…estonly Automatic update from web-platform-tests <link disabled> is now standardized For whatwg/html#4519. -- wpt-commits: ee42c2ebaba97d3bb6b4676de3712f62a6eea1d9 wpt-pr: 22645
…estonly Automatic update from web-platform-tests <link disabled> is now standardized For whatwg/html#4519. -- wpt-commits: ee42c2ebaba97d3bb6b4676de3712f62a6eea1d9 wpt-pr: 22645
Previously, Chromium had intermittent behavior with respect to the "disabled" attribute: - <link id=foo rel="stylesheet" disabled> would not show up in document.styleSheets. - foo.disabled=false; foo.disabled=true; would cause it to appear (and remain) in document.styleSheets. - <link rel="alternate stylesheet"> cannot be enabled, except by disabling and re-enabling it. - When disabled, link.ownerNode was not null The above issues are being resolved with this CL. See the spec discussion and PR here: whatwg/html#3840 whatwg/html#4519 And the ChromeStatus and I2S here: https://chromestatus.com/feature/5110851973414912 https://groups.google.com/a/chromium.org/d/msg/blink-dev/3izM5vVo98U/iDLjz_TwAgAJ= WPT tests are located here: https://wpt.fyi/results/css/cssom?label=master&label=experimental&aligned&q=htmllinkelement-disabled and they show that Firefox already implements the new behavior, while WebKit does not. With this CL, all 7 tests are now green. Bug: 695984, 1087043 Change-Id: Ic6d2c8e901a72885b9f4806c616adb95e019cf09 Cq-Do-Not-Cancel-Tryjobs: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216701 Reviewed-by: Chris Harrelson <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Commit-Queue: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/master@{#774658}
As far as the doc need, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#attr-disabled already has documentation for this. But it’d be great to have it reviewed. If anybody has changes, you can edit that article directly, or else let me know and I’d be happy to make any changes needed. |
@sideshowbarker, the documentation looks pretty good, except this part:
The difference/change is that now, changing HTMLLinkElement.disabled to true will actually remove the stylesheet from document.styleSheets. |
@mfreed7 OK, thanks — so I’ve replaced the text cited above with the following single sentence:
Lemme know if further changes are needed. |
Sounds succinct and correct - thanks! |
All browsers have implemented this for a long time. (However, unlike HTMLLinkElement, no browsers implement a disabled="" content attribute for <style>.) Note that this was originally tracked by whatwg#1081. That issue was closed in whatwg#4519, though it did not spec anything <style> related.
Previously, Chromium had intermittent behavior with respect to the "disabled" attribute: - <link id=foo rel="stylesheet" disabled> would not show up in document.styleSheets. - foo.disabled=false; foo.disabled=true; would cause it to appear (and remain) in document.styleSheets. - <link rel="alternate stylesheet"> cannot be enabled, except by disabling and re-enabling it. - When disabled, link.ownerNode was not null The above issues are being resolved with this CL. See the spec discussion and PR here: whatwg/html#3840 whatwg/html#4519 And the ChromeStatus and I2S here: https://chromestatus.com/feature/5110851973414912 https://groups.google.com/a/chromium.org/d/msg/blink-dev/3izM5vVo98U/iDLjz_TwAgAJ= WPT tests are located here: https://wpt.fyi/results/css/cssom?label=master&label=experimental&aligned&q=htmllinkelement-disabled and they show that Firefox already implements the new behavior, while WebKit does not. With this CL, all 7 tests are now green. Bug: 695984, 1087043 Change-Id: Ic6d2c8e901a72885b9f4806c616adb95e019cf09 Cq-Do-Not-Cancel-Tryjobs: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216701 Reviewed-by: Chris Harrelson <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Commit-Queue: Mason Freed <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#774658} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 0ebaa77c4ffda85933b6a822218e1c4b439efbe9
Tests: web-platform-tests/wpt#22645
Fixes #3840, maybe.
/custom-elements.html ( diff )
/indices.html ( diff )
/links.html ( diff )
/semantics.html ( diff )