Input elements mishandle notifications on type change that affects validity

RESOLVED FIXED in Firefox 58

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

53 Branch
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Created attachment 8892762 [details]
First testcase

This is a horrible bug that got introduced in bug 598833 and apparently we didn't have the test coverage to catch it.

Simple testcases attached.  Since bug 1385478 is going to make those testcases work for slightly unrelated reasons, I'll try to write some others that still fail even with those fixes.

The problem is that HTMLInputElement::HandleTypeChange does this:

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

The "done later if needed" bit went away in bug 598833 in favor of an UpdateState() call in the "later" place.  But UpdateAllValidityStates was also changed to UpdateState(), in the case when validity changed, and in this case it doesn't notify that update.  Which means the later UpdateState doesn't pick up any state changes due to the type change, because they already happened.  So we don't notify on them.
Attachment #8892762 - Attachment description: bar.html → First testcase
Created attachment 8892764 [details]
Testcase using :placeholder-shown instead of :required
Created attachment 8892765 [details]
Testcase using :placeholder-shown that passes for reasons I don't understand
How urgent is this?
Flags: needinfo?(bzbarsky)
Priority: -- → P2
I was planning to patch it in the next few days, fwiw.
Flags: needinfo?(bzbarsky)
So the obvious fix is not enough here.  That's because in my placeholder-shown testcase HTMLInputElement::HandleTypeChange also calls HTMLInputElement::SetValueInternal (because we're switching to VALUE_MODE_VALUE), which calls nsTextEditorState::SetValue, which calls nsTextEditorState::SetValue, which calls HTMLInputElement::OnValueChanged, which calls HTMLInputElement::UpdateAllValidityStates.  And in this case aNotify is false because nsTextEditorState::SetValue has a null mRootNode, so doesn't bother to notify.

And even if we didn't call UpdateAllValidityStates from OnValueChanged, we'd still call UpdateState(aNotify) (because we have a placeholder and are now a type to which placeholder applies) which would fail in the same way.

Ehsan, is there a reason we skip notifying from nsTextEditorState::SetValue if !mRootNode?
Created attachment 8910380 [details]
Testcase not involving type changes that fails due to that !mRootNode business
Er, actually needinfo to Ehsan for comment 6.
Flags: needinfo?(ehsan)
Created attachment 8910393 [details] [diff] [review]
What I have so far; doesn't help the testcase given the value change thing
Created attachment 8910394 [details]
Testcase that _is_ helped by the WIP patch
Spun off comment 6 into bug 1401657.
Flags: needinfo?(ehsan)
Created attachment 8910406 [details] [diff] [review]
Fix handling of type changes that affect validity state to properly notify state changes

MozReview-Commit-ID: Khhzi1HyCpt
Attachment #8910406 - Flags: review?(jjong)
Attachment #8910393 - Attachment is obsolete: true
Attachment #8910380 - Attachment is obsolete: true
Comment on attachment 8910406 [details] [diff] [review]
Fix handling of type changes that affect validity state to properly notify state changes

Review of attachment 8910406 [details] [diff] [review]:
-----------------------------------------------------------------

Although the case of "changing from an input type that placeholder applies to an input type that placeholder does not apply" (attachment 8892765 [details]) was working already before this patch, can we also add a test for it? Thanks.
Attachment #8910406 - Flags: review?(jjong) → review+
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #3)
> Created attachment 8892765 [details]
> Testcase using :placeholder-shown that passes for reasons I don't understand

I think this works because UpdateAllValidityStates() doesn't actually call UpdateState() in this case since the validity hasn't changed.
> I think this works because UpdateAllValidityStates() doesn't actually call UpdateState() in this case

Right you are!  If I make the input in attachment 8892765 [details] required, the testcase fails.  Thank you!  I'll add that to the tests, as well as the test you asked for in comment 13.

Comment 16

10 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db561b3fd265
Fix handling of type changes that affect validity state to properly notify state changes.  r=jessica
https://hg.mozilla.org/mozilla-central/rev/db561b3fd265
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.