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

Implement correct :disabled support for fieldsets and nested form controls #125

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

camchenry
Copy link
Contributor

With the current implementation of the :disabled selector, there are two main issues:

  1. We are not considering nested fieldsets, only checking the first ancestor fieldset. The HTML spec specifies that it should be any descendant:

    A form control is disabled if any of the following are true: [...] the element is a descendant of a fieldset element whose disabled attribute is specified, and is not a descendant of that fieldset element's first legend element child, if any.

  2. We are not correctly considering fieldsets that do not have a legend. Currently, the check for if the legend element contains the element doesn't handle if the first legend element doesn't exist in the fieldset. Specifically, (n=s.ancestor("fieldset",e))&&(n=s.first("legend",n))&&!n.contains(e)) returns false because n=s.first("legend",n) evaluates to null which is falsey, so the whole AND expression evaluates to false. Rather, if there is no legend element, we do not need to consider if the element is contained within the legend element and can solely focus on whether it is disabled or a descendant of a disabled fieldset.

Since the original test/w3c/html/semantics/selectors/pseudo-classes/disabled.html web-platform-test was added, it has been updated with additional test-cases. So I have added those here to ensure this works correctly.

// F is true if any of the fieldset elements in the ancestry chain has the disabled attribute specified
// L is true if the first legend element of the fieldset contains the element
'var x=0,N=[],F=false,L=false;' +
'if(!(/^(optgroup|option)$/i.test(e.localName))){' +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit messy, but optgroup has different definitions for disabled, as the HTML spec notes in the disabled attribute documentation:

The disabled attribute for option elements and the disabled attribute for optgroup elements are defined separately.

We shouldn't take into account fieldset/legend for optgroup/option, so we skip over code related to that here.

@dperini
Copy link
Owner

dperini commented Aug 30, 2024

@camchenry
very well this is fantastic, this is real help for me.
Thank you so much for the help catching the problem and code to fix it.
It will be merged immediately after a small review once tested against wpt.

@camchenry
Copy link
Contributor Author

@dperini Thanks, let me know if there is anything I can do to help with this PR. I did test it out in Chrome 127 on macOS and it looked okay, but I didn't try any other browsers so far.

@dperini
Copy link
Owner

dperini commented Sep 27, 2024

@camchenry
it is better if I apply your fix as is, without further delays since there have been enough testing.
Optimizations can be applied later if they still make this change to pass wpt requirements.

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.

2 participants