1569314 - Incorrect constraint validation for select element with selected disabled option

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Florian Schoppmann]

Attachment: select-element-constraint-validation.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Safari/605.1.15

Steps to reproduce:

Consider the following HTML:

<select required>
<option value="value" selected disabled>
Initially Disabled Option
</option>
</select>

Actual results:

Firefox considers the <select> element as invalid (as reported by checkValidity() or shown by the CSS :invalid pseudo-class).

A reproducible test case is attached.

Expected results:

According to the specification (https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element:attr-select-required-4), calling checkValidity() on the <select> element should return true. Chrome and Safari work correctly.

Comment 1

[Florian Schoppmann]

The following might justify a separate bug report, but it is closely related, and the attached test case already shows it, too:

Firefox does not update validity of the <select> element if the disabled property of the <option> element is changed. A workaround currently would be to temporarily set required property to false, and then back to true.

Comment 2

[Virginia (please send need info request to Brindusa Tot)]

Hi Florian,

Thank you for providing us with the test page. Can you please tell us the steps so we can reproduce the behavior reported on our end?

Also, please let us know if you see this happening in the latest version of Firefox Nightly, you can download it from here: https://nightly.mozilla.org/.

If there is any other info you can provide to help us figure out how we can can reproduce the issue please share them.

Thanks for your contribution.

Comment 3

[Florian Schoppmann]

Hi Virginia,

Sure, the detailed steps are:

1. If you go to my attached test case (https://bugzilla.mozilla.org/attachment.cgi?id=9081024), you'll see a <select> element with red color. The red color comes from the :invalid pseudo-class defined in the CSS (see the source).

   The problem is:
   The <select> should not be considered invalid (as reported by checkValidity() or shown by the CSS :invalid pseudo-class). Indeed, Chrome and Safari do not consider it invalid (because an option with a non-empty value is selected).

2. Now if you uncheck the checkbox labeled "Disabled", JavaScript code sets the disabled property of the <option> element within the <select> to false. Consequently, the red color (i.e., :invalid pseudo-class) should disappear. But it doesn't. If you then click on the button labeled "Set required to false and back to true", JavaScript code sets the required property of the <select> element to false and back to true. The red color finally disappears (as it should have before).

   In other words:
   Even if 1. was a deliberate design choice, Firefox should update validity if the disabled property of the <option> is changed. (Validity is correctly updated if the required property of the <select> is changed.)

Regarding the nightly build:

Also, please let us know if you see this happening in the latest version of Firefox Nightly, you can download it from here: https://nightly.mozilla.org/.

Yes, it still does. Version 70.0a1 (2019-08-02).

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 5

[Virginia (please send need info request to Brindusa Tot)]

Hi Florian,

Thank you for the detailed explanation! I was able to replicate the behavior you reported.

I'll mark this ticket as new. A product and component have already been set but feel free to change them if you consider they are not the correct ones.

Regards,

Comment 6

[Virginia (please send need info request to Brindusa Tot)]

Also reproducible on Windows 10
Nightly 70.0a1 (2019-08-14) (64-bit)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I think this check is what's causing this.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

The check goes all the way back to bug 596511 so not a regression. The check should just be removed, most likely, and a test should be added if there's none.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Sorry, I wanted to mentor this, not self-assign it. It is an easy bug to fix, if somebody wants to submit their first patch. I can fix it too of course but I'd prefer if somebody would peek this up :)

Comment 10

[Samarth Tambad]

Hey! Could I take up this bug?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yes, please go ahead! Let me know if you have any question :)

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 13

[Nour Saffour]

Hi,
Can I get this bug assigned to me please :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Sure!

Comment 15

[Nour Saffour]

Attachment: Bug 1569314 select element no longer marked as invalid

Removed check from HTMLSelectElement::IsValueMissing which was invalidating the select element

Comment 16

[Nour Saffour]

Hi Emilio,

I have removed the line that you mentioned earlier and built nightly.
The select is no longer in red which I think is the behavior that we wanted to achieve.

Do you think I should make some tests for this bug?

Comment 17

[Nour Saffour]

I'm running the mochitests now to see if everything goes well.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nour Saffour from comment #16)

Do you think I should make some tests for this bug?

Yes, do you have try access? I'd run both mochitests and web-platform-tests if so.

Comment 19

[Nour Saffour]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

(In reply to Nour Saffour from comment #16)

Do you think I should make some tests for this bug?

Yes, do you have try access? I'd run both mochitests and web-platform-tests if so.

Yes I have try access, I got it from my previous bug.

I have submitted the patch to the try server and selected the mochitest and web-platform-tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78697efae5281d3bed51716d348983320827df7

Comment 20

[Nour Saffour]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

(In reply to Nour Saffour from comment #16)

Do you think I should make some tests for this bug?

Yes, do you have try access? I'd run both mochitests and web-platform-tests if so.

Hi Emilio,

I have ran the tests on the try server and got the following results:
https://treeherder.mozilla.org/testview.html?repo=try&revision=b78697efae5281d3bed51716d348983320827df7

I'm not sure how to interpret it, could you guide me through this process?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

If you see https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78697efae5281d3bed51716d348983320827df7&selectedJob=273645232 you'll see those tests being marked with a [Tier 2] flag. That means that they do not block landing, and those failures seem completely unrelated to your stuff.

But the downside is that there's no existing tests for this, so we need to write one.

Do you know how to write web platformt tests? You can write a test similar to this, but even simpler, since you don't need all the dynamic element creation / cloning stuff: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/forms/the-textarea-element/textarea-validity-clone.html

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:nsaffour, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[Nour Saffour]

Sorry for the delay.
I'm having a busy week. Will add the tests and submit it in the next week.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 25

[NicolasPacheco]

Hey Emilio. I would like to take this bug. Should I just write the test?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Yeah that'd be awesome! Let me know if you need help with that.

Comment 27

[NicolasPacheco]

Yes, is there any info on how to write a test? or should I use https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/forms/the-textarea-element/textarea-validity-clone.html ? (the link you provided in comment 21)

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Yeah, that test is a decent example to crib from, but there are WPT docs here: http://web-platform-tests.org/writing-tests/testharness.html

Comment 29

[Loek]

Hi Emilio,
I noticed that there has been no activity in the past month. Is that long enough for it to be considered unassigned again? If so, I would like to work on this bug.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Yes, please, feel free to submit a patch or ask me any question you might have. The bug is unassigned, see comment 24.

Comment 31

[Loek]

Okay I have removed the check Emilio mentioned in comment 7 which seems to fix it, and then I started looking at the test file in testing/web-platform/tests/html/semantics/forms/the-select-element/select-validity.html to add a test for this test case (this is my first bug so I was looking at the existing tests to get familiar with it).

However when looking at the existing tests I noticed that in line 40 it is stated that "A disabled first placeholder option should invalidate the select.". Doesn't this contradict this bug?

If it doesn't contradict or the statement in the test file is wrong (meaning I should still write a new test for this bug) then I have another question: Should I add a test(function{ }) to select-validity.html for the extra test case, or should I create a new test .html file?

Comment 32

[Emilio Cobos Álvarez (:emilio)]

No, I don't think it contradicts that statement, because the test-case in comment 0 has a non empty option. So yes, a new test needs to be added, and a test(function() {}) in that test seems great (though a new test would also be acceptable).

Comment 33

[Loek]

Thanks for your quick reply! I'm running into a new problem when trying to test the test file. I'm running the command
./mach test testing/web-platform/tests/html/semantics/forms/the-select-element/select-validity.html
and it gives me a syntax error on the usage of metaclass at line44 in item.py.
I suspect this is a problem with python3 code being executed by python2.
Am I using the wrong command, or can I not run these tests individually? What is the best way to run the test if I'm only interested in running this test?

Comment 34

[Emilio Cobos Álvarez (:emilio)]

./mach wpt html/semantics/forms/the-select-element/select-validity.html should work. James, is the error above known?

Comment 35

[James Graham [:jgraham]]

Yeah, mach test is somewhat broken at the moment since it's still using Python 2 but the harnesses are a mix of Python 2-only, Python 3-only and both Python 2 and 3. wpt in particular is Python 3-only so it's kind of the worst case scenario. There is ongoing work to move everything onto Py3; I think that most other testing mach commands already moved, even when the corresponding suite is still Py2 in CI, so perhaps it's possible to move mach test sooner rather than later. Bug 1638992 covers that (maybe it should block mach-busted?).

Comment 36

[Loek]

wpt works for me, thanks! I wrote a working test, so now I have reached the point where I did hg commit and have to submit it. I read through all guidelines I could find but since it is the first time I'm submitting a patch I have some questions:

1. To test on the try server I need to get level 1 commit access. Do I have to do that for this bug?
2. What name should I specify for you in r=<reviewer> in my commits?
3. I'm understanding that I have to submit my commit to Phabricator using the moz-phab submit command, but I'm not sure if that is the full command I should use and if I should do something else first. I did make sure the test runs successfully locally and the only hg diff is the two files I edited.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

(In reply to Loek from comment #36)

1. To test on the try server I need to get level 1 commit access. Do I have to do that for this bug?

No, I can push it for you.

2. What name should I specify for you in r=<reviewer> in my commits?

r=emilio :-)

3. I'm understanding that I have to submit my commit to Phabricator using the moz-phab submit command, but I'm not sure if that is the full command I should use and if I should do something else first. I did make sure the test runs successfully locally and the only hg diff is the two files I edited.

Yeah, ideally you don't even need the submit part even, as submit is the default action, and moz-phab should be able to detect the commits that are authored by you but not public yet.

You can read moz-phab submit --help for all the different options, for example moz-phab submit . --single should be slightly faster since it doesn't need to auto-detect the commits you wrote. But anyhow that's sort of more advanced usage, moz-phab alone or moz-phab submit should work.

Comment 38

[Loek]

Attachment: Bug 1569314 - Remove IsOptionDisabled check and add relevant test case. r=emilio

Comment 39

[Loek]

Okay thanks for your fast reply once again :). I submitted: https://phabricator.services.mozilla.com/D108005.

Comment 40

[Loek]

I noticed that there are 4 failures on the try server including one from the test I added on, if I'm understanding correctly, an android emulator. Am I supposed to look into this? If so, should I look at all 4 failures or only this failure?

Comment 41

[Emilio Cobos Álvarez (:emilio)]

That's the only thing related to your change, but looking closer my try run accidentally used artifact builds, so that explains the issue. Sorry :(

Comment 42

[Loek]

Attachment: Bug 1569314 - Change incorrect select validation test. r=emilio

Depends on D108005

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/028bce8cc4ac
Remove IsOptionDisabled check and add relevant test case. r=emilio
https://hg.mozilla.org/integration/autoland/rev/95a0692aa312
Change incorrect select validation test. r=emilio

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Thanks for the fix Loek!

Comment 45

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/028bce8cc4ac
https://hg.mozilla.org/mozilla-central/rev/95a0692aa312

Comment 46

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28076 for changes under testing/web-platform/tests

Comment 47

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot