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

Edge 17: fieldset disabled attribute not working #2978

Merged
merged 3 commits into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@BartG95
Contributor

BartG95 commented Oct 12, 2018

The disabled attribute on a fieldset has no effect in Edge 17.

This is also reflected in https://www.w3schools.com/TAgs/att_fieldset_disabled.asp

Edge 17: fieldset disabled attribute not working
The disabled attribute on a fieldset has no effect in Edge 17.

This is also reflected in https://www.w3schools.com/TAgs/att_fieldset_disabled.asp

@Elchi3 Elchi3 requested a review from wbamberg Oct 15, 2018

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 24, 2018

Thanks for the PR, and sorry to be slow reviewing. I don't have Edge installed here but just tested it on Edge 17 using BrowserStack and it seemed to work fine (specifically, the live example here worked https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset#Disabled_fieldset).

Do you have any other references than w3schools about it not working in Edge? Caniuse says that IE has problems with it, but says nothing about problems in Edge.

Update fieldset.json
It does works as intended with one fieldset, but it does not work with nested fieldsets.
@BartG95

This comment has been minimized.

Contributor

BartG95 commented Oct 24, 2018

You are right when it comes down to one fieldset. But with nested fieldsets it fails. See https://jsfiddle.net/4djphrq2/ for an example.

I reflected this in 0012646.

@wbamberg

Thanks for digging into this, looks great. I pushed a very minor editorial update to the note ("E.g." -> "For example") just to avoid you having to go through the re-review cycle again.

@wbamberg wbamberg merged commit 41160be into mdn:master Oct 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment