1386530 - Input elements mishandle notifications on type change that affects validity

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Second testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase using :placeholder-shown instead of :required

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase using :placeholder-shown that passes for reasons I don't understand

Comment 4

[Andrew Overholt [:overholt]]

How urgent is this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I was planning to patch it in the next few days, fwiw.

Comment 6

[Boris Zbarsky [: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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase not involving type changes that fails due to that !mRootNode business

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, actually needinfo to Ehsan for comment 6.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: What I have so far; doesn't help the testcase given the value change thing

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that _is_ helped by the WIP patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Spun off comment 6 into bug 1401657.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix handling of type changes that affect validity state to properly notify state changes

MozReview-Commit-ID: Khhzi1HyCpt

Comment 13

[Jessica Jong [:jessica] (inactive)]

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.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/db561b3fd265