Skip to content
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

Merged
merged 4 commits into from
May 29, 2020
Merged

Add <link disabled> #4519

merged 4 commits into from
May 29, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 10, 2019

Copy link
Member Author

@annevk annevk left a 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 Show resolved Hide resolved
source Outdated Show resolved Hide resolved
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>
Copy link
Member Author

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.

Copy link
Contributor

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.

@sideshowbarker sideshowbarker added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Apr 13, 2019
@sideshowbarker
Copy link
Contributor

heads-up @whatwg/documentation

@chrisdavidmills
Copy link

Potential documentation need recorded at https://trello.com/c/6YAMGL3B/132-html-link-element

Tests: ...

Fixes #3840, maybe.
@annevk annevk force-pushed the annevk/attr-link-disabled branch from 79c033d to de4720d Compare April 2, 2020 10:30
annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 2, 2020
@annevk
Copy link
Member Author

annevk commented Apr 20, 2020

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.

@domenic
Copy link
Member

domenic commented Apr 20, 2020

I'm sad to ask, but is there multi-implementer interest here?

@annevk
Copy link
Member Author

annevk commented Apr 20, 2020

Is your question specifically about #3840 (comment)? It would be good to hear from @mfreed7 and @cdumez, yeah.

@domenic
Copy link
Member

domenic commented Apr 20, 2020

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.

@emilio
Copy link
Contributor

emilio commented Apr 22, 2020

cc @lilles, is fixing this in Chromium something you'd consider? Current alternate sheets are pretty broken there :)

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest do not merge yet Pull request must not be merged per rationale in comment labels May 27, 2020
@domenic domenic removed do not merge yet Pull request must not be merged per rationale in comment needs implementer interest Moving the issue forward requires implementers to express interest labels May 28, 2020
Copy link
Member

@domenic domenic left a 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="">&lt;link disabled rel="alternate stylesheet" href="css/pooh"></code></pre>
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@annevk annevk merged commit 9e77887 into master May 29, 2020
@annevk annevk deleted the annevk/attr-link-disabled branch May 29, 2020 06:17
annevk added a commit to web-platform-tests/wpt that referenced this pull request May 29, 2020
@annevk
Copy link
Member Author

annevk commented May 29, 2020

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.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 1, 2020
…estonly

Automatic update from web-platform-tests
<link disabled> is now standardized

For whatwg/html#4519.
--

wpt-commits: ee42c2ebaba97d3bb6b4676de3712f62a6eea1d9
wpt-pr: 22645
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 2, 2020
…estonly

Automatic update from web-platform-tests
<link disabled> is now standardized

For whatwg/html#4519.
--

wpt-commits: ee42c2ebaba97d3bb6b4676de3712f62a6eea1d9
wpt-pr: 22645
pull bot pushed a commit to Yannic/chromium that referenced this pull request Jun 3, 2020
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}
@sideshowbarker
Copy link
Contributor

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.

@mfreed7
Copy link
Contributor

mfreed7 commented Jun 4, 2020

@sideshowbarker, the documentation looks pretty good, except this part:

Changing the value of this property instead simply enables and disables the stylesheet form being applied to the document.
This differs from StyleSheet's disabled property; changing it to true removes the stylesheet from the document's document.styleSheets list, ...

The difference/change is that now, changing HTMLLinkElement.disabled to true will actually remove the stylesheet from document.styleSheets.

@sideshowbarker
Copy link
Contributor

@mfreed7 OK, thanks — so I’ve replaced the text cited above with the following single sentence:

Setting the disabled property in the DOM causes the stylesheet to be removed from the document's document.styleSheets list.

Lemme know if further changes are needed.

@mfreed7
Copy link
Contributor

mfreed7 commented Jun 5, 2020

Sounds succinct and correct - thanks!

domenic pushed a commit that referenced this pull request Apr 11, 2022
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 #1081. That issue was closed in #4519, though it did not spec anything <style> related.
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
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.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation
Development

Successfully merging this pull request may close these issues.

<link rel="stylesheet" disabled>
6 participants