Closed
Bug 1385478
Opened 7 years ago
Closed 7 years ago
Consider caching the nsGkAtoms::required attribute in a flag
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
| Tracking | Status | |
|---|---|---|
| firefox57 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: jessica)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
|
(deleted),
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Boris, can you please share your thoughts on the feasibility of this? Thanks!
Flags: needinfo?(bzbarsky)
|
||
Comment 2•7 years ago
|
||
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•7 years 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=.
|
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8892270 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Comment 6•7 years ago
|
||
Attachment #8892271 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8892271 -
Attachment description: Part 2: cache required state. → Part 2: change IsRequired() to look at the cached required state.
|
|
||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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-
|
|
||
Comment 9•7 years ago
|
||
> 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•7 years ago
|
||
Btw, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=751406 on Chrome failing that testcase too...
|
|
||
Comment 11•7 years ago
|
||
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.
|
|
Assignee | |
Comment 12•7 years ago
|
||
(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.
|
|
Assignee | |
Comment 13•7 years ago
|
||
(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.
|
|
Assignee | |
Comment 14•7 years ago
|
||
(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.
|
|
Assignee | |
Comment 15•7 years ago
|
||
Address review comment 7 and carry r+.
Attachment #8892270 -
Attachment is obsolete: true
Attachment #8892952 -
Flags: review+
|
|
Assignee | |
Comment 16•7 years ago
|
||
Attachment #8892271 -
Attachment is obsolete: true
Attachment #8892955 -
Flags: review?(bzbarsky)
|
|
||
Comment 17•7 years ago
|
||
> 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•7 years ago
|
||
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+
|
|
Assignee | |
Comment 19•7 years ago
|
||
(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. :)
|
|
Assignee | |
Comment 20•7 years ago
|
||
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+
|
|
Assignee | |
Comment 21•7 years ago
|
||
Addressed review comment 18, carrying r+.
Attachment #8892955 -
Attachment is obsolete: true
Attachment #8893263 -
Flags: review+
|
|
Assignee | |
Comment 22•7 years ago
|
||
Keywords: checkin-needed
|
|
Assignee | |
Comment 23•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/00412d7d8f38
https://hg.mozilla.org/mozilla-central/rev/2aa4fe1bb445
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•