Consider caching the nsGkAtoms::required attribute in a flag

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Ehsan, Assigned: jessica)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
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?
(Reporter)

Comment 1

a year ago
Boris, can you please share your thoughts on the feasibility of this?  Thanks!
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 3

a year ago
(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=.
Sure, I can work on this.
Assignee: nobody → jjong
Created attachment 8892270 [details] [diff] [review]
Part 1: add IsRequired() helper function.
Attachment #8892270 - Flags: review?(bzbarsky)
Created attachment 8892271 [details] [diff] [review]
Part 2: change IsRequired() to look at the cached required state.
Attachment #8892271 - Flags: review?(bzbarsky)
(Assignee)

Updated

a year ago
Attachment #8892271 - Attachment description: Part 2: cache required state. → Part 2: change IsRequired() to look at the cached required state.
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.
Attachment #8892270 - Flags: review?(bzbarsky) → review+
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.
Attachment #8892271 - Flags: review?(bzbarsky) → review-
> 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.
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.
(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.
(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.
(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.
Created attachment 8892952 [details] [diff] [review]
Part 1: Use IsRequired/Required() to get required state.

Address review comment 7 and carry r+.
Attachment #8892270 - Attachment is obsolete: true
Attachment #8892952 - Flags: review+
Created attachment 8892955 [details] [diff] [review]
Part 2: change Required/IsRequired() to look at the cached required state, v2.
Attachment #8892271 - Attachment is obsolete: true
Attachment #8892955 - Flags: review?(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 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!
Attachment #8892955 - Flags: review?(bzbarsky) → review+
(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. :)
Created attachment 8893262 [details] [diff] [review]
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+.
Attachment #8892952 - Attachment is obsolete: true
Attachment #8893262 - Flags: review+
Created attachment 8893263 [details] [diff] [review]
Part 2: change Required/IsRequired() to look at the cached required state, v3.

Addressed review comment 18, carrying r+.
Attachment #8892955 - Attachment is obsolete: true
Attachment #8893263 - Flags: review+
(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

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00412d7d8f38
https://hg.mozilla.org/mozilla-central/rev/2aa4fe1bb445
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.