1385478 - Consider caching the nsGkAtoms::required attribute in a flag

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[(no longer active)]

Similar to bug 1375599, the lookup for this attribute from the code below shows up in profiles of bug 1346723.

https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/html/input/SingleLineTextInputTypes.cpp#61

Can we use a similar technique to cache this one in NS_EVENT_STATE_REQUIRED similarly?

Comment 1

[(no longer active)]

Boris, can you please share your thoughts on the feasibility of this?  Thanks!

Comment 2

[Boris Zbarsky [:bzbarsky]]

This seems pretty reasonable, yes.  Should be a good bit simpler than "disabled" actually, because it doesn't have any of the non-local behavior there: it just depends on the one element's attributes.  There's the caveat of needing to update the state on "type" attribute changes on <input>, but that's it.

We could presumably add an IsRequired() method too, and use this in the code linked above.  We should also look into other getters of this attr and see if they can be switched to checking the state flag...

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This seems pretty reasonable, yes.  Should be a good bit simpler than
> "disabled" actually, because it doesn't have any of the non-local behavior
> there: it just depends on the one element's attributes.  There's the caveat
> of needing to update the state on "type" attribute changes on <input>, but
> that's it.

Awesome!  Jessica, are you interested in this one as well?

> We could presumably add an IsRequired() method too, and use this in the code
> linked above.  We should also look into other getters of this attr and see
> if they can be switched to checking the state flag...

Yes, good idea.  There is also other places in the code where we check for it which we could possibly switch over, see https://searchfox.org/mozilla-central/search?q=nsGkAtoms%3A%3Arequired&path=.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Sure, I can work on this.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add IsRequired() helper function.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: change IsRequired() to look at the cached required state.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8892270 [details] [diff] [review]
Part 1: add IsRequired() helper function.

>+  HTMLInputElement* input = HTMLInputElement::FromContentOrNull(element);

How about we change nsIRadioGroupContainer::AddToRadioGroup to take HTMLInputElement*?  That would simplify a bunch of code around this stuff...

Followup is fine (in fact preferable) for this part.

In the meantime, we just asserted element is not null, so just FromContent(), please.  This applies to both nsDocument and HTMLFormElement.

>@@ -7214,34 +7214,34 @@ HTMLInputElement::UpdateValueMissingValidityStateForRadio

Can we assert that DoesRequiredApply() in here, please?

>+++ b/dom/html/HTMLInputElement.h
>+  bool IsRequired() const

This needs some comments about how it differs from Required().  For example, if DoesRequiredApply() is false, right?

>+++ b/dom/html/HTMLSelectElement.h
>+++ b/dom/html/HTMLTextAreaElement.h

Do these need an IsRequired(), or could we just make Required() check the state once the second patch is done for these elements?  If we really do need an IsRequired(), please document why.

r=me with the above fixed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8892271 [details] [diff] [review]
Part 2: change IsRequired() to look at the cached required state.

> to update or clear our required states when input type changes, since we we may

s/we we/we/

> viceversa.

"vice versa".

>+++ b/dom/html/HTMLInputElement.cpp
>@@ -1448,16 +1448,23 @@ HTMLInputElement::AfterSetAttr(int32_t a
>+        UpdateRequiredState(aValue ? true : false, aNotify);

You could just pass "aValue" as the first arg.  If you want to emphasize the conversion to boolean, pass !!aValue.

>@@ -4992,16 +4999,25 @@ HTMLInputElement::HandleTypeChange(uint8
>+    UpdateRequiredState(isRequired, false);

Why false and not aNotify?  That doesn't look right to me.

>+    RemoveStatesSilently(REQUIRED_STATES);

Likewise, doing a silent state change here does not seem right.

If this passed tests, that means we're missing test coverage.  Here's a testcase that passes without this patch but fails with it, afaict:

  <!DOCTYPE html>
  <style>
    span { color: red; }
    :required + span { color: green }
  </style>
  <input type="hidden" required><span>This should be green</span>
  <script>
    onload = function() {
      document.querySelector("input").type = "";
    }
  </script>

>+HTMLInputElement::UpdateRequiredState(bool aIsRequired, bool aNotify)

Assert in this method that DoesRequiredApply(), please.

>+   * Check our required state to decide whether our required flag should be
>+   * toggled.

It doesn't really check anything.  The caller checks.  Maybe:

  * Update our required/optional flags to match the given aRequired boolean.

?

>+++ b/dom/html/HTMLSelectElement.cpp
>+      UpdateRequiredState(aValue ? true : false, aNotify);

Again, aValue or !!aValue.

It looks to me like UpdateRequiredState in this file is identical to the one in HTMLInputElement and only depends on Element members.  Can we hoist it to a common superclass?  For the assert I asked for, we could add an #ifdef DEBUG block that does HTMLInputElement::FromContent(this) and then if non-null asserts DoesRequiredApply.

>+++ b/dom/html/HTMLTextAreaElement.cpp
>+        UpdateRequiredState(aValue ? true : false, aNotify);

aValue or !!aValue.

>+HTMLTextAreaElement::UpdateRequiredState(bool aIsRequired, bool aNotify)

Again, would be best to share this code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Here's a testcase that passes without this patch but fails with it, afaict

Actually, it looks like that fails without the patch too, in a current nightly.  I wonder why...  In any case, I expect that fixing that lack of notification in the patch will in fact make the test pass.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Btw, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=751406 on Chrome failing that testcase too...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, I know why that testcase and this variant:

  <!DOCTYPE html>
  <style>
    span { color: red; }
    :required + span { color: green }
  </style>
  <input type="hidden" required><span>This should be green</span>
  <script>
    onload = function() {
      document.querySelector("input").type = "";
    }
  </script>

fail.  It's because HTMLInputElement::HandleTypeChange has this bit:

  // Do not notify, it will be done after if needed.
  UpdateAllValidityStates(false);

which is a horrible bug.  Looks like the "will be done after" went away in bug 598833 and ever since then this has been broken.  I filed bug 1386530 on this.

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Oh, I know why that testcase and this variant:
> 
>   <!DOCTYPE html>
>   <style>
>     span { color: red; }
>     :required + span { color: green }
>   </style>
>   <input type="hidden" required><span>This should be green</span>
>   <script>
>     onload = function() {
>       document.querySelector("input").type = "";
>     }
>   </script>
> 
> fail.  It's because HTMLInputElement::HandleTypeChange has this bit:
> 
>   // Do not notify, it will be done after if needed.
>   UpdateAllValidityStates(false);
> 
> which is a horrible bug.  Looks like the "will be done after" went away in
> bug 598833 and ever since then this has been broken.  I filed bug 1386530 on
> this.

Right. Actually, I took the "false" from this code, without verifying :(
I'll fix bug 1386530 once this bug is landed.

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8892270 [details] [diff] [review]
> Part 1: add IsRequired() helper function.
> 
> >+  HTMLInputElement* input = HTMLInputElement::FromContentOrNull(element);
> 
> How about we change nsIRadioGroupContainer::AddToRadioGroup to take
> HTMLInputElement*?  That would simplify a bunch of code around this stuff...
> 
> Followup is fine (in fact preferable) for this part.
> 
> In the meantime, we just asserted element is not null, so just
> FromContent(), please.  This applies to both nsDocument and HTMLFormElement.

Sure, will file a followup and use FromContent() here for now.

> 
> >@@ -7214,34 +7214,34 @@ HTMLInputElement::UpdateValueMissingValidityStateForRadio
> 
> Can we assert that DoesRequiredApply() in here, please?

Will do.

> 
> >+++ b/dom/html/HTMLInputElement.h
> >+  bool IsRequired() const
> 
> This needs some comments about how it differs from Required().  For example,
> if DoesRequiredApply() is false, right?

In this part, IsRequired() is sill the same as Required(), they both check the required attribute. They will differ in Part 2 though, I can add comments there.

> 
> >+++ b/dom/html/HTMLSelectElement.h
> >+++ b/dom/html/HTMLTextAreaElement.h
> 
> Do these need an IsRequired(), or could we just make Required() check the
> state once the second patch is done for these elements?  If we really do
> need an IsRequired(), please document why.

Yeah, I think we can just use Required().

> 
> r=me with the above fixed.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #7)
> > 
> > >@@ -7214,34 +7214,34 @@ HTMLInputElement::UpdateValueMissingValidityStateForRadio
> > 
> > Can we assert that DoesRequiredApply() in here, please?
> 
> Will do.
> 

I will just assert that mType is NS_FORM_INPUT_RADIO.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Use IsRequired/Required() to get required state.

Address review comment 7 and carry r+.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: change Required/IsRequired() to look at the cached required state, v2.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I'll fix bug 1386530 once this bug is landed.

Thank you!  I'm happy to fix it too, since I caused it, but if you want to do it, I have other stuff I can work on, for sure.  Just let me know!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8892955 [details] [diff] [review]
Part 2: change Required/IsRequired() to look at the cached required state, v2.

+  MOZ_ASSERT((type & NS_FORM_INPUT_ELEMENT ||

I'd prefer we had parens around the "&" expression, since those have weird precedence... so something like:

  MOZ_ASSERT((type & NS_FORM_INPUT_ELEMENT) ||
             type == NS_FORM_SELECT ||
             type == NS_FORM_TEXTAREA,

(no parens around the general MOZ_ASSERT condition).

r=me.  Thank you for doing this!

Comment 19

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> > I'll fix bug 1386530 once this bug is landed.
> 
> Thank you!  I'm happy to fix it too, since I caused it, but if you want to
> do it, I have other stuff I can work on, for sure.  Just let me know!

Oh, sorry, I didn't notice it was assigned already. I'll leave it to you then. :)

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Use IsRequired/Required() to get required state, v2.

I forgot to change to use IsRequired() in RemoveFromRadioGroup(), so adding this part. Since it's a straightforward change, carrying r+.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: change Required/IsRequired() to look at the cached required state, v3.

Addressed review comment 18, carrying r+.

Comment 22

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=784dd932ae74f58501c89cd8ec432eb2bf570789&group_state=expanded

Comment 23

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #7)
> > Comment on attachment 8892270 [details] [diff] [review]
> > Part 1: add IsRequired() helper function.
> > 
> > >+  HTMLInputElement* input = HTMLInputElement::FromContentOrNull(element);
> > 
> > How about we change nsIRadioGroupContainer::AddToRadioGroup to take
> > HTMLInputElement*?  That would simplify a bunch of code around this stuff...

Filed bug 1386969 for this.

> > 
> > Followup is fine (in fact preferable) for this part.
> > 
> > In the meantime, we just asserted element is not null, so just
> > FromContent(), please.  This applies to both nsDocument and HTMLFormElement.
> 
> Sure, will file a followup and use FromContent() here for now.
>

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00412d7d8f38
Part 1: Use IsRequired/Required() to get the current required state. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa4fe1bb445
Part 2: Change Required/IsRequired() to look at NS_EVENT_STATE_REQUIRED instead. r=bz

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/00412d7d8f38
https://hg.mozilla.org/mozilla-central/rev/2aa4fe1bb445