Bug 1528644 - Internal value changes shouldn't change validity state. r=masayuki

Even less so on reframe, where it's just unsound to do so. I had to give a value
to eSetValue_Internal, since otherwise I cannot check for its presence. I can
further special-case the reframe case if you prefer.

Differential Revision: https://phabricator.services.mozilla.com/D20133

--HG--
extra : moz-landing-system : lando

dom/html/HTMLInputElement.cpp

@@ -2604,8 +2604,10 @@ nsresult HTMLInputElement::SetValueInternal(const nsAString& aValue,
         // If the caller won't dispatch "input" event via
         // nsContentUtils::DispatchInputEvent(), we need to modify
         // validationMessage value here.
-        if (aFlags & (nsTextEditorState::eSetValue_Internal |
-                      nsTextEditorState::eSetValue_ByContent)) {
+        //
+        // FIXME(emilio): eSetValue_Internal is not supposed to change state,
+        // but maybe we could run this too?
+        if (aFlags & nsTextEditorState::eSetValue_ByContent) {
           MaybeUpdateAllValidityStates();
         }
       } else {
@@ -2640,8 +2642,7 @@ nsresult HTMLInputElement::SetValueInternal(const nsAString& aValue,
           }
         }
         if (mDoneCreating) {
-          OnValueChanged(/* aNotify = */ true,
-                         /* aWasInteractiveUserChange = */ false);
+          OnValueChanged(/* aNotify = */ true, ValueChangeKind::Internal);
         }
         // else DoneCreatingElement calls us again once mDoneCreating is true
       }
@@ -6753,8 +6754,10 @@ HTMLInputElement::InitializeKeyboardEventListeners() {
 }
 
 NS_IMETHODIMP_(void)
-HTMLInputElement::OnValueChanged(bool aNotify, bool aWasInteractiveUserChange) {
-  mLastValueChangeWasInteractive = aWasInteractiveUserChange;
+HTMLInputElement::OnValueChanged(bool aNotify, ValueChangeKind aKind) {
+  if (aKind != ValueChangeKind::Internal) {
+    mLastValueChangeWasInteractive = aKind == ValueChangeKind::UserInteraction;
+  }
 
   UpdateAllValidityStates(aNotify);
 

dom/html/HTMLInputElement.h

@@ -244,8 +244,7 @@ class HTMLInputElement final : public nsGenericHTMLFormElementWithState,
   NS_IMETHOD_(bool) GetPlaceholderVisibility() override;
   NS_IMETHOD_(bool) GetPreviewVisibility() override;
   NS_IMETHOD_(void) InitializeKeyboardEventListeners() override;
-  NS_IMETHOD_(void)
-  OnValueChanged(bool aNotify, bool aWasInteractiveUserChange) override;
+  NS_IMETHOD_(void) OnValueChanged(bool aNotify, ValueChangeKind) override;
   virtual void GetValueFromSetRangeText(nsAString& aValue) override;
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   virtual nsresult SetValueFromSetRangeText(const nsAString& aValue) override;

dom/html/HTMLTextAreaElement.cpp

@@ -1111,9 +1111,10 @@ HTMLTextAreaElement::InitializeKeyboardEventListeners() {
 }
 
 NS_IMETHODIMP_(void)
-HTMLTextAreaElement::OnValueChanged(bool aNotify,
-                                    bool aWasInteractiveUserChange) {
-  mLastValueChangeWasInteractive = aWasInteractiveUserChange;
+HTMLTextAreaElement::OnValueChanged(bool aNotify, ValueChangeKind aKind) {
+  if (aKind != ValueChangeKind::Internal) {
+    mLastValueChangeWasInteractive = aKind == ValueChangeKind::UserInteraction;
+  }
 
   // Update the validity state
   bool validBefore = IsValid();

dom/html/HTMLTextAreaElement.h

@@ -99,8 +99,7 @@ class HTMLTextAreaElement final : public nsGenericHTMLFormElementWithState,
   NS_IMETHOD_(void) EnablePreview() override;
   NS_IMETHOD_(bool) IsPreviewEnabled() override;
   NS_IMETHOD_(void) InitializeKeyboardEventListeners() override;
-  NS_IMETHOD_(void)
-  OnValueChanged(bool aNotify, bool aWasInteractiveUserChange) override;
+  NS_IMETHOD_(void) OnValueChanged(bool aNotify, ValueChangeKind) override;
   virtual void GetValueFromSetRangeText(nsAString& aValue) override;
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   virtual nsresult SetValueFromSetRangeText(const nsAString& aValue) override;

dom/html/TextInputListener.h

@@ -47,7 +47,7 @@ class TextInputListener final : public nsIDOMEventListener,
   /**
    * OnSelectionChange() is called when selection is changed in the editor.
    */
-  void OnSelectionChange(Selection& aSelection, int16_t aReason);
+  void OnSelectionChange(dom::Selection& aSelection, int16_t aReason);
 
   /**
    * Start to listen or end listening to selection change in the editor.

dom/html/nsITextControlElement.h

@@ -181,11 +181,17 @@ class nsITextControlElement : public nsISupports {
    */
   NS_IMETHOD_(bool) GetPreviewVisibility() = 0;
 
+  enum class ValueChangeKind {
+    Internal,
+    Script,
+    UserInteraction,
+  };
+
   /**
    * Callback called whenever the value is changed.
    */
   NS_IMETHOD_(void)
-  OnValueChanged(bool aNotify, bool aWasInteractiveUserChange) = 0;
+  OnValueChanged(bool aNotify, ValueChangeKind) = 0;
 
   /**
    * Helpers for value manipulation from SetRangeText.

dom/html/nsTextEditorState.cpp

@@ -51,6 +51,7 @@
 
 using namespace mozilla;
 using namespace mozilla::dom;
+using ValueChangeKind = nsITextControlElement::ValueChangeKind;
 
 inline nsresult SetEditorFlagsIfNecessary(EditorBase& aEditorBase,
                                           uint32_t aFlags) {
@@ -987,7 +988,7 @@ void TextInputListener::HandleValueChanged(nsTextControlFrame* aFrame) {
 
   if (!mSettingValue) {
     mTxtCtrlElement->OnValueChanged(/* aNotify = */ true,
-                                    /* aWasInteractiveUserChange = */ true);
+                                    ValueChangeKind::UserInteraction);
   }
 }
 
@@ -2456,11 +2457,15 @@ bool nsTextEditorState::SetValue(const nsAString& aValue,
     ValueWasChanged(!!mBoundFrame);
   }
 
+  // TODO(emilio): It seems wrong to pass ValueChangeKind::Script if
+  // BySetUserInput is in aFlags.
+  auto changeKind = (aFlags & eSetValue_Internal)
+    ? ValueChangeKind::Internal
+    : ValueChangeKind::Script;
+
   // XXX Should we stop notifying "value changed" if mTextCtrlElement has
   //     been cleared?
-  textControlElement->OnValueChanged(/* aNotify = */ !!mBoundFrame,
-                                     /* aWasInteractiveUserChange = */ false);
-
+  textControlElement->OnValueChanged(/* aNotify = */ !!mBoundFrame, changeKind);
   return true;
 }
 

dom/html/nsTextEditorState.h

@@ -163,24 +163,24 @@ class nsTextEditorState : public mozilla::SupportsWeakPtr<nsTextEditorState> {
 
   enum SetValueFlags {
     // The call is for internal processing.
-    eSetValue_Internal = 0,
+    eSetValue_Internal = 1 << 0,
     // The value is changed by a call of setUserInput() from chrome.
-    eSetValue_BySetUserInput = 1 << 0,
+    eSetValue_BySetUserInput = 1 << 1,
     // The value is changed by changing value attribute of the element or
     // something like setRangeText().
-    eSetValue_ByContent = 1 << 1,
+    eSetValue_ByContent = 1 << 2,
     // Whether the value change should be notified to the frame/contet nor not.
-    eSetValue_Notify = 1 << 2,
+    eSetValue_Notify = 1 << 3,
     // Whether to move the cursor to end of the value (in the case when we have
     // cached selection offsets), in the case when the value has changed.  If
     // this is not set, the cached selection offsets will simply be clamped to
     // be within the length of the new value.  In either case, if the value has
     // not changed the cursor won't move.
-    eSetValue_MoveCursorToEndIfValueChanged = 1 << 3,
+    eSetValue_MoveCursorToEndIfValueChanged = 1 << 4,
     // The value is changed for a XUL text control as opposed to for an HTML
     // text control.  Such value changes are different in that they preserve the
     // undo history.
-    eSetValue_ForXUL = 1 << 4,
+    eSetValue_ForXUL = 1 << 5,
   };
   MOZ_CAN_RUN_SCRIPT
   MOZ_MUST_USE bool SetValue(const nsAString& aValue,

editor/libeditor/tests/mochitest.ini

@@ -315,6 +315,7 @@ skip-if = toolkit == 'android' || (verify && debug && os == 'win') # bug 1147989
 [test_select_all_without_body.html]
 [test_spellcheck_pref.html]
 skip-if = toolkit == 'android'
+[test_state_change_on_reframe.html]
 [test_undo_after_spellchecker_replaces_word.html]
 skip-if = toolkit == 'android'
 [test_undo_redo_stack_after_setting_value.html]

editor/libeditor/tests/test_state_change_on_reframe.html

@@ -0,0 +1,29 @@
+<!doctype html>
+<title>Test for state change not bogusly changing during reframe (bug 1528644)</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<style>
+  .reframe { display: table }
+  input:invalid { color: red; }
+  input:valid { color: green; }
+</style>
+<form>
+  <input type="text" required minlength="4" onkeypress="this.classList.toggle('reframe')">
+</form>
+<script>
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(function() {
+  let input = document.querySelector("input");
+  input.focus();
+  requestAnimationFrame(() => {
+    synthesizeKey("a");
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        ok(!input.validity.valid);
+        is(getComputedStyle(input).display, "table");
+        SimpleTest.finish();
+      });
+    });
+  });
+});
+</script>

layout/base/RestyleManager.cpp

@@ -2976,10 +2976,7 @@ void RestyleManager::ClearSnapshots() {
 }
 
 ServoElementSnapshot& RestyleManager::SnapshotFor(Element& aElement) {
-  // FIXME(emilio, bug 1528644): This can actually happen, and cause observable
-  // behavior differences. Upgrade to a release assert when bug 1528644 is
-  // fixed.
-  MOZ_ASSERT(!mInStyleRefresh);
+  MOZ_RELEASE_ASSERT(!mInStyleRefresh);
 
   // NOTE(emilio): We can handle snapshots from a one-off restyle of those that
   // we do to restyle stuff for reconstruction, for example.
@@ -3195,10 +3192,7 @@ void RestyleManager::UpdateOnlyAnimationStyles() {
 
 void RestyleManager::ContentStateChanged(nsIContent* aContent,
                                          EventStates aChangedBits) {
-  // FIXME(emilio, bug 1528644): This can actually happen, and cause observable
-  // behavior differences. Upgrade to a release assert when bug 1528644 is
-  // fixed.
-  MOZ_ASSERT(!mInStyleRefresh);
+  MOZ_RELEASE_ASSERT(!mInStyleRefresh);
 
   if (!aContent->IsElement()) {
     return;