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 HTMLLinkElement as eligible .disabled element #1791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gossi
Copy link

@gossi gossi commented Dec 20, 2022

When trying to write tests for a HTMLLinkElement and check for .disabled state, instead of asserting, qunit-dom throw an error.

There is a little gotcha for disabled on <link>.

  • As per declarative API (<link disabled>), this is experimental, deprecated and advised against its usage (and also does not work in browsers)
  • As per imperative API (HTMLLinkElement.disabled), this is according to MDN allow and works in browsers

So, this PR adds HTMLLinkElement as eligible element to check this for 😃

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 20, 2022

but... why? 😂

is there a legitimate use case for something like this? 😅

@gossi
Copy link
Author

gossi commented Dec 20, 2022

In fact yes, there is 😅

When each <link> represents a theme and only one can be active, then make the others .disabled in which case the browser don't use that CSS. That's the state when CSS is loaded but not in use (which is for what I'm writing the test for).

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 20, 2022

and why not remove the link elements instead? 🤔

@gossi
Copy link
Author

gossi commented Dec 20, 2022

that's implementation details of my particular library.

The point is, the provided API (isDisabled() and isNotDisabled()) is here to check for a disabled property on any * extends HTMLElement that supports it. However, this is subjectively constrained by qunit-dom and doesn't match reality. I figured HTMLLinkElement is missing from that list and I wouldn't be surprised if there are more.

I see this more of a bugfix, so the API keeps holding up to its promise :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants