Bug 1539777 - Properly handle readonly changes in nsFormFillController. r=MattN

There's a readonly shorcut, which prevents us altogether to register the
mutation observer which would notify us of that attribute changing.

Move the readonly check further down to StartControllingInput, so that we
register the mutation observer properly.

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

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

toolkit/components/satchel/nsFormFillController.cpp

@@ -849,7 +849,7 @@ nsFormFillController::HandleEvent(Event* aEvent) {
       }
 
       bool unused = false;
-      return (!mSuppressOnInput && mController && mFocusedInput)
+      return (!mSuppressOnInput && IsFocusedInputControlled())
                  ? mController->HandleText(&unused)
                  : NS_OK;
     }
@@ -860,13 +860,13 @@ nsFormFillController::HandleEvent(Event* aEvent) {
       return NS_OK;
     case eCompositionStart:
       NS_ASSERTION(mController, "should have a controller!");
-      if (mController && mFocusedInput) {
+      if (IsFocusedInputControlled()) {
         mController->HandleStartComposition();
       }
       return NS_OK;
     case eCompositionEnd:
       NS_ASSERTION(mController, "should have a controller!");
-      if (mController && mFocusedInput) {
+      if (IsFocusedInputControlled()) {
         mController->HandleEndComposition();
       }
       return NS_OK;
@@ -881,10 +881,8 @@ nsFormFillController::HandleEvent(Event* aEvent) {
         return NS_OK;
       }
 
-      if (mFocusedInput) {
-        if (doc == mFocusedInput->OwnerDoc()) {
-          StopControllingInput();
-        }
+      if (mFocusedInput && doc == mFocusedInput->OwnerDoc()) {
+        StopControllingInput();
       }
 
       // Only remove the observer notifications and marked autofill and password
@@ -948,13 +946,9 @@ void nsFormFillController::MaybeStartControllingInput(
     return;
   }
 
-  if (aInput->ReadOnly()) {
-    return;
-  }
-
   bool autocomplete = nsContentUtils::IsAutocompleteEnabled(aInput);
 
-  bool hasList = aInput->GetList() != nullptr;
+  bool hasList = !!aInput->GetList();
 
   bool isPwmgrInput = false;
   if (mPwmgrInputs.Get(aInput) ||
@@ -1021,7 +1015,7 @@ nsresult nsFormFillController::HandleFocus(HTMLInputElement* aInput) {
 
 nsresult nsFormFillController::Focus(Event* aEvent) {
   nsCOMPtr<nsIContent> input = do_QueryInterface(aEvent->GetComposedTarget());
-  return this->HandleFocus(HTMLInputElement::FromNodeOrNull(input));
+  return HandleFocus(HTMLInputElement::FromNodeOrNull(input));
 }
 
 nsresult nsFormFillController::KeyDown(Event* aEvent) {
@@ -1029,7 +1023,7 @@ nsresult nsFormFillController::KeyDown(Event* aEvent) {
 
   mPasswordPopupAutomaticallyOpened = false;
 
-  if (!mFocusedInput || !mController) {
+  if (!IsFocusedInputControlled()) {
     return NS_OK;
   }
 
@@ -1065,7 +1059,7 @@ nsresult nsFormFillController::KeyPress(Event* aEvent) {
 
   mPasswordPopupAutomaticallyOpened = false;
 
-  if (!mFocusedInput || !mController) {
+  if (!IsFocusedInputControlled()) {
     return NS_OK;
   }
 
@@ -1333,13 +1327,18 @@ void nsFormFillController::StartControllingInput(HTMLInputElement* aInput) {
   aInput->AddMutationObserverUnlessExists(this);
   mFocusedInput = aInput;
 
-  Element* list = mFocusedInput->GetList();
-  if (list) {
+  if (Element* list = mFocusedInput->GetList()) {
     list->AddMutationObserverUnlessExists(this);
     mListNode = list;
   }
 
-  mController->SetInput(this);
+  if (!mFocusedInput->ReadOnly()) {
+    mController->SetInput(this);
+  }
+}
+
+bool nsFormFillController::IsFocusedInputControlled() const {
+  return mFocusedInput && mController && !mFocusedInput->ReadOnly();
 }
 
 void nsFormFillController::StopControllingInput() {
@@ -1367,7 +1366,6 @@ void nsFormFillController::StopControllingInput() {
           ("StopControllingInput: Stopped controlling %p", mFocusedInput));
   if (mFocusedInput) {
     MaybeRemoveMutationObserver(mFocusedInput);
-
     mFocusedInput = nullptr;
   }
 

toolkit/components/satchel/nsFormFillController.h

@@ -74,6 +74,8 @@ class nsFormFillController final : public nsIFormFillController,
   void StartControllingInput(mozilla::dom::HTMLInputElement* aInput);
   void StopControllingInput();
 
+  bool IsFocusedInputControlled() const;
+
   nsresult HandleFocus(mozilla::dom::HTMLInputElement* aInput);
 
   /**

toolkit/components/satchel/test/mochitest.ini

@@ -11,6 +11,7 @@ skip-if = os == 'linux' # bug 1022386
 [test_bug_787624.html]
 skip-if = os == 'linux' # bug 1022386
 [test_datalist_with_caching.html]
+[test_datalist_readonly_change.html]
 [test_datalist_shadow_dom.html]
 [test_form_autocomplete.html]
 skip-if = (verify && debug && (os == 'win')) || os == 'linux' # linux - bug 1022386

toolkit/components/satchel/test/test_datalist_readonly_change.html

@@ -0,0 +1,76 @@
+<!DOCTYPE HTML>
+<title>Dynamic change to readonly doesn't prevent datalist to keep working</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/AddTask.js"></script>
+<script src="satchel_common.js"></script>
+<link rel="stylesheet" href="/tests/SimpleTest/test.css" />
+<input readonly list="suggest" type="text" name="field1">
+<datalist id="suggest">
+  <option value="First"></option>
+  <option value="Second"></option>
+  <option value="Secomundo"></option>
+</datalist>
+<pre id="test">
+<script class="testbody">
+const input = document.querySelector("input");
+
+SimpleTest.waitForExplicitFinish();
+
+var expectingPopup = null;
+
+function expectPopup() {
+  info("expecting a popup");
+  return new Promise(resolve => {
+    expectingPopup = resolve;
+  });
+}
+
+var testNum = 0;
+
+function popupShownListener() {
+  info("popup shown for test " + testNum);
+  if (expectingPopup) {
+    expectingPopup();
+    expectingPopup = null;
+  } else {
+    ok(false, "Autocomplete popup not expected during test " + testNum);
+  }
+}
+
+registerPopupShownListener(popupShownListener);
+
+function checkMenuEntries(expectedValues) {
+  let actualValues = getMenuEntries();
+  is(actualValues.length, expectedValues.length, testNum + " Checking length of expected menu");
+  for (let i = 0; i < expectedValues.length; i++) {
+    is(actualValues[i], expectedValues[i], testNum + " Checking menu entry #" + i);
+  }
+}
+
+function oneTick() {
+  return new Promise(resolve => SimpleTest.executeSoon(resolve));
+}
+
+async function runTests() {
+  input.focus();
+  ok(input.readOnly, "Input should be readonly");
+  is(document.activeElement, input, "Input should be focused");
+  input.removeAttribute("readonly");
+  await oneTick(); // AttributeChanged takes control of the input again off a runnable...
+  isnot(input.readOnly, "Input should not be readonly");
+  is(document.activeElement, input, "Should still be focused");
+  synthesizeKey("KEY_ArrowDown");
+  await expectPopup();
+
+  checkMenuEntries(["First", "Second", "Secomundo"]);
+  synthesizeKey("KEY_ArrowDown");
+  synthesizeKey("KEY_Enter");
+  is(input.value, "First");
+  SimpleTest.finish();
+}
+
+SimpleTest.waitForFocus(runTests);
+
+</script>
+</pre>