Closed Bug 1569314 Opened 5 years ago Closed 3 years ago

Incorrect constraint validation for select element with selected disabled option

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
68 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox70 --- wontfix
firefox88 --- fixed

People

(Reporter: mail, Assigned: lvdgugten, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++], [wptsync upstream])

Attachments

(4 files)

select-element-constraint-validation.html
1.71 KB, text/html
Details
Bug 1569314 select element no longer marked as invalid
47 bytes, text/x-phabricator-request
Details | Review
Bug 1569314 - Remove IsOptionDisabled check and add relevant test case. r=emilio
48 bytes, text/x-phabricator-request
Details | Review
Bug 1569314 - Change incorrect select validation test. r=emilio
48 bytes, text/x-phabricator-request
Details | Review

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.

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.

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.

Flags: needinfo?(mail)

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).

Flags: needinfo?(mail)

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

Component: Untriaged → Layout: Form Controls
Product: Firefox → Core

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,

Status: UNCONFIRMED → NEW
Ever confirmed: true

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

status-firefox70: --- → affected
Component: Layout: Form Controls → DOM: Core & HTML

I think this check is what's causing this.

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.

Assignee: nobody → emilio
Mentor: emilio
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [lang=c++]

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 :)

Assignee: emilio → nobody

Hey! Could I take up this bug?

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

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

Keywords: regression

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

Sure!

Assignee: nobody → nsaffour

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

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?

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

(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.

(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

Attachment #9104933 - Attachment description: select element no longer marked as invalid → Bug 1569314 select element no longer marked as invalid

(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?

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

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.

Flags: needinfo?(nsaffour)

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

Flags: needinfo?(nsaffour)

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

Assignee: nsaffour → nobody

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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)

Flags: needinfo?(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

Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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?

Flags: needinfo?(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).

Flags: needinfo?(emilio)

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?

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio) → needinfo?(james)

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?).

Flags: needinfo?(james)

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.
Flags: needinfo?(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.

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

r=emilio :-)

  1. 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.

Flags: needinfo?(emilio)
Assignee: nobody → lvdgugten
Status: NEW → ASSIGNED

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

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?

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 :(

Depends on D108005

Pushed by ealvarez@mozilla.com:
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

Thanks for the fix Loek!

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28076 for changes under testing/web-platform/tests
Whiteboard: [lang=c++] → [lang=c++], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: