1285425 - :valid CSS pseudo-class doesn't match valid <form>s

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Chris Rebert]

Attachment: Testcase demonstrating the bug

Relevant specification:
https://html.spec.whatwg.org/multipage/scripting.html#selector-valid

Relevant quote:
>  The :valid pseudo-class must match any element falling into one of the following categories:
> [...]
> * <form> elements that are not the form owner of any elements that themselves are candidates for constraint validation but do not satisfy their constraints
> [...]

Steps to reproduce:
1. Open the attached testcase in Firefox.

Expected result:
I should see a green rectangle next to the text input.
This rectangle is the <form>, and since it contains no invalid form controls,
it should match the `form:valid` selector in the CSS, and thus become green.

Actual result:
I see a red rectangle next to the text input,
indicating that the <form> doesn't match the :valid CSS pseudo-class.

Other browsers:
* Chrome: Works
* Safari: Works
* Edge: Broken

See also:
https://github.com/Fyrd/caniuse/issues/2384

Comment 1

[Thomas Wisniewski]

Attachment: 1285425-have_forms_always_UpdateState_on_constrainted_element_add_or_remove.diff

Here's a patch that passes the attached test-case. It seems that forms simply weren't updating their state in all cases when constrained elements were added to/removed from them, so they weren't being redrawn/styled appropriately, despite having the correct validity state.

I updated a web platform test to include this test-case, including a similar one for fieldsets (just to be thorough).

Comment 2

[Thomas Wisniewski]

Also, here's a link to a try-run (which seems fine): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf818907ba0&selectedJob=25127343

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8778078 [details] [diff] [review]
1285425-have_forms_always_UpdateState_on_constrainted_element_add_or_remove.diff

I don't think this is right.

In particular, consider this testcase:

  <style>
  form {
    background-color: red;
  }
  form:valid {
    background-color: green;
  }
  </style>
  <form>text</form>

This should show a green background, but shows red even with this patch.

It seems to me that the state maintenance in AddElement/RemoveElement is correct.  The problem is that our state before any elements have been added is not :valid, but of course it should be.

I think what should happen here is that the HTMLFormElement constructor should  AddStatesSilently(NS_EVENT_STATE_VALID) with a comment explaining why.  See what the NS_EVENT_STATE_VALID constructor does, for example.

Comment 4

[Thomas Wisniewski]

Attachment: 1285425-set_forms_to_enabled_and_valid_upon_construction.diff

Ah, yes that does make sense. Here's a new version of the patch that just modifies the form constructor to set their initial state to enabled and valid, which works for empty forms as well. I've tweaked the WPT as well to test that case, too.

Hopefully try works out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df376b9851c

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8778369 [details] [diff] [review]
1285425-set_forms_to_enabled_and_valid_upon_construction.diff

>+++ b/dom/html/HTMLFormElement.cpp
>+  AddStatesSilently(NS_EVENT_STATE_ENABLED | NS_EVENT_STATE_VALID);

Why do you need NS_EVENT_STATE_ENABLED in there?  Or more to the point, isn't that part just wrong, since it makes a random form match :enabled?  I suppose nothing tests that, which is not surprising.

In general, it doesn't make sense to AddStatesSilently for bits in the constructor if your IntrinsicState method doesn't add those bits to the return value.  In the case of HTMLFormElement, the only bits that HTMLFormElement::IntrinsicState can add are NS_EVENT_STATE_VALID and NS_EVENT_STATE_INVALID, so those are the only bits that make sense to pass to an AddStatesSilently call in the constructor.

>+++ b/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/valid-invalid.html
>+  function testStyles(type) {

If you get an exception while evaluating the header of this function (getting all those vars before the first test() call), that won't play very nice with the harness.  Luckily, you can have nested test() invocations, so I think it should be fine to invoke the function like so:

  test(testStyles.bind(undefined, "form"), ":valid/:invalid styling for <form>");

or so.

It may be worth having a helper function like this:

  function getBgColor(elem) {
    return getComputedStyle(elem).backgroundColor;
  }

but either way.

>+    test(function() {
>+      assert_equals(getComputedStyle(invalid).backgroundColor, expectedPassBGColor, "wrong background-color");
>+    }, 'invalid ' + type + ' correctly styled on page-load')

This expectation confused me for a bit until I realized that you're styling :valid and :invalid with the same background, so this test is just checking that one of those applies, but isn't checking which one.  Why not use different backgrounds for those two cases, have expectedValidBGColor and expectedInvalidBGColor (e.g. "green" and "lime") and use those in these asserts?  I think that would be a lot clearer.

Also, is there a reason this test, and the one for getComputedStyle(valid).backgroundColor on pageload is repeated twice with no intermediate mutations?  Looks to me like copy/paste error...

Also, ';' after the statement, and same for the other test() calls.  Let's not rely on ASI if we don't have to.  ;)

Comment 6

[Thomas Wisniewski]

Attachment: 1285425-set_forms_to_valid_upon_construction.diff

Here's an updated patch addressing your comments.


>Why do you need NS_EVENT_STATE_ENABLED in there?

Ah, right. I just copied that line from the fieldset constructor, and forgot to confirm whether :enabled applies to forms as well. Thanks for the catch! (Also, on the off-chance that it's a sign of a problem, there's no mention of "enabled" in fieldset's IntrinsicState method.)


>Why not use different backgrounds for those two cases, have expectedValidBGColor and expectedInvalidBGColor (e.g. "green" and "lime")

Done.


>Luckily, you can have nested test() invocations

Done, thanks for that tip!


>It may be worth having a helper function

Sure, why not. It makes things slightly more readable.


>Also, is there a reason this test, and the one for getComputedStyle(valid).backgroundColor on pageload is repeated twice with no intermediate mutations? Looks to me like copy/paste error...

Yes, that's all that was.


>Let's not rely on ASI

Agreed, fixed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Also, on the off-chance that it's a sign of a problem, there's no
> mention of "enabled" in fieldset's IntrinsicState method.

Yeah, true.  It's set in its superclass method, nsGenericHTMLFormElement::IntrinsicState.

The problem is that not all the subclasses of nsGenericHTMLFormElement default to the enabled state (because not all can be disabled).  So it's only set in the constructors of the ones which do.  Checking CanBeDisabled() in the nsGenericHTMLFormElement constructor is not possible because it's a virtual function and the subclass vtable isn't set up yet at that point....

We _could move the handling of the ENABLED flag into the CanBeDisabled subclasses, but that would involve some code duplication (though one less virtual call during intrinsic state calculation).

Anyway, the setup is in fact ok, but it was definitely worth checking!

> Done, thanks for that tip!

Um... the patch you attached isn't making use of that tip.  Nor is it doing the helper function thing.  Did you forget to commit or qrefresh or something?

Comment 8

[Thomas Wisniewski]

Attachment: 1285425-set_forms_to_valid_upon_construction.diff

>Did you forget to commit or qrefresh or something?

Odd... whatever happened that's not the version of the patch I intended for review. Here's a fresh copy.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779499 [details] [diff] [review]
1285425-set_forms_to_valid_upon_construction.diff

r=me

Comment 10

[Thomas Wisniewski]

Thanks, requesting check-in.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1398505493
Set forms to valid in their constructor. r=bz

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac1398505493