Closed Bug 1285425 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mozilla, Assigned: wisniewskit)

References

()

Details

(Keywords: css3, html5, Whiteboard: [parity-chrome][parity-safari][parity-webkit])

Attachments

(2 files, 3 obsolete files)

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
Blocks: 344614
Has STR: --- → yes
Keywords: css3, html5
Whiteboard: [parity-chrome][parity-safari][parity-webkit]
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).
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8778078 - Flags: review?(bzbarsky)
Also, here's a link to a try-run (which seems fine): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf818907ba0&selectedJob=25127343
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.
Attachment #8778078 - Flags: review?(bzbarsky) → review-
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
Attachment #8778078 - Attachment is obsolete: true
Attachment #8778369 - Flags: review?(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.  ;)
Attachment #8778369 - Flags: review?(bzbarsky) → review-
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.
Attachment #8778369 - Attachment is obsolete: true
Attachment #8779480 - Flags: review?(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?
Flags: needinfo?(wisniewskit)
>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.
Attachment #8779480 - Attachment is obsolete: true
Attachment #8779480 - Flags: review?(bzbarsky)
Flags: needinfo?(wisniewskit)
Attachment #8779499 - Flags: review?(bzbarsky)
Comment on attachment 8779499 [details] [diff] [review]
1285425-set_forms_to_valid_upon_construction.diff

r=me
Attachment #8779499 - Flags: review?(bzbarsky) → review+
Thanks, requesting check-in.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1398505493
Set forms to valid in their constructor. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac1398505493
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.