jimmywarting / FormData

HTML5 `FormData` polyfill for Browsers and nodejs
MIT License
360 stars 102 forks source link

Exclude Disabled Fieldset Descendent Controls #110

Closed kevinslodic closed 4 years ago

kevinslodic commented 4 years ago

According to the W3C spec, a disabled <fieldset> element should cause all descendent form controls, excluding those of the first <legend> element, to also become disabled.

Looking through the source that iterates over the form's elements, I didn't see a check for the above. To have parity with the native implementation of FormData, this sounds like a good addition.

Happy to help contribute myself if you don't have the bandwidth.

jimmywarting commented 4 years ago

crap, You are right. Missed it. That means iterating over all fields makes it slightly more annoying since we probably have to start looking at the DOM tree :[

Happy to help contribute myself if you don't have the bandwidth.

That would be nice, just contributing with an idea of how to filter out fields inside a disabled fieldset would be 👍

jimmywarting commented 4 years ago

hmm, maybe could use matches + :disabled

elm.disabled || elm.matches(':disabled')
jimmywarting commented 4 years ago

https://jsfiddle.net/pqc315uh/

jimmywarting commented 4 years ago

any other suggestions or alternative way? must deal with prefixed versions of matches otherwise

kevinslodic commented 4 years ago

I don't think looking at elm.matches(':disabled') will work since the input itself doesn't become disabled, it just inherently acts disabled based on the <fieldset>; I think that's a different problem altogether, though.

I stand corrected above; interesting that it triggers :disabled, but not the DOM property.

What about keeping a reference to the disabled <fieldset> elements and simply checking if the element is a descendent of that node?

// since this primarily is driven from <= IE 11's lack of support, can't use Array.from without adding a polyfill
const disabledFieldsets = Array.prototype.slice.call(form.querySelectorAll('fieldset:disabled'));

each(form.elements, elm => {
  if (!elm.name || elm.disabled || elm.type === 'submit' || elm.type === 'button' || disabledFieldsets.some(f => f.contains(elm)) return
});
jimmywarting commented 4 years ago

Are you sure elm.matches(':disabled') would not work? i like it better... and it seems to work

otherwise an alternetive would be elm.matches('fieldset[disabled] *')

kevinslodic commented 4 years ago

No, you're right; I had spoken too soon (I updated my comment above to reflect). I had figured it wouldn't work based on elm.disabled still reporting as false, so poorly assuming it wouldn't match the :disabled pseudo-class.

Agreed--it's a much cleaner implementation!

kevinslodic commented 4 years ago

I think elm.matches('fieldset[disabled] *') would work best; I ran elm.matches(':disabled') in an IE 11 VM and it didn't seem to register correctly (screenshot below):

ie11-matches
jimmywarting commented 4 years ago

fixed, and publish to npm. it's funny how no new features are added or that there is no breaking changes. my patch version is on 20 now :P never had it that high on any application

I'm amazed that ppl still are able to find tiny bugs that have gone unnoticed by so many users upon till this day. I would think more ppl would stop using this since most browser have proper support now days

kevinslodic commented 4 years ago

Cool--thanks for the effort! Unfortunately, IE 11 still has just enough presence that it's too hard to ignore.