Bug 1343037 part 12. Implement nsTextEditorState::SetSelectionStart. r=ehsan
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 09 Mar 2017 14:44:05 -0500
changeset 347111 69da5429c4b4ecb2423132f0e33df1c67a55e771
parent 347110 b3416e9f0d5cf21b653bc1e9c2c457d7125a1fc9
child 347112 301369c6601ce010962483ac4a72f9b1ea309583
push id31491
push usercbook@mozilla.com
push dateMon, 13 Mar 2017 14:24:00 +0000
treeherdermozilla-central@8d9fd089cabd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1343037
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1343037 part 12. Implement nsTextEditorState::SetSelectionStart. r=ehsan This introduces three behavior changes: 1) Before this change, in cached mode, we did not enforce the "start <= end" invariant. 2) Before this change, in cached mode, we did not fire "select" events on selectionStart changes. 3) Changes the IDL type of HTMLInputElement's selectionStart attribute to "unsigned long" to match the spec and HTMLTextareaElement. MozReview-Commit-ID: JM9XXMMPUHM
dom/html/HTMLInputElement.cpp
dom/html/HTMLInputElement.h
dom/html/HTMLTextAreaElement.cpp
dom/html/nsTextEditorState.cpp
dom/html/nsTextEditorState.h
dom/webidl/HTMLInputElement.webidl
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/html/semantics/forms/textfieldselection/selection-start-end.html
testing/web-platform/tests/html/semantics/forms/textfieldselection/textfieldselection-setRangeText.html
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -6417,77 +6417,51 @@ HTMLInputElement::SetRangeText(const nsA
     default:
       MOZ_CRASH("Unknown mode!");
   }
 
   Optional<nsAString> direction;
   SetSelectionRange(aSelectionStart, aSelectionEnd, direction, aRv);
 }
 
-Nullable<int32_t>
+Nullable<uint32_t>
 HTMLInputElement::GetSelectionStart(ErrorResult& aRv)
 {
   if (!SupportsTextSelection()) {
-    return Nullable<int32_t>();
-  }
-
-  int32_t selStart = GetSelectionStartIgnoringType(aRv);
+    return Nullable<uint32_t>();
+  }
+
+  uint32_t selStart = GetSelectionStartIgnoringType(aRv);
   if (aRv.Failed()) {
-    return Nullable<int32_t>();
-  }
-
-  return Nullable<int32_t>(selStart);
-}
-
-int32_t
+    return Nullable<uint32_t>();
+  }
+
+  return Nullable<uint32_t>(selStart);
+}
+
+uint32_t
 HTMLInputElement::GetSelectionStartIgnoringType(ErrorResult& aRv)
 {
   int32_t selEnd, selStart;
   GetSelectionRange(&selStart, &selEnd, aRv);
   return selStart;
 }
 
 void
-HTMLInputElement::SetSelectionStart(const Nullable<int32_t>& aSelectionStart,
+HTMLInputElement::SetSelectionStart(const Nullable<uint32_t>& aSelectionStart,
                                     ErrorResult& aRv)
 {
   if (!SupportsTextSelection()) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
-  int32_t selStart = 0;
-  if (!aSelectionStart.IsNull()) {
-    selStart = aSelectionStart.Value();
-  }
-
   nsTextEditorState* state = GetEditorState();
-  if (state && state->IsSelectionCached()) {
-    state->GetSelectionProperties().SetStart(selStart);
-    return;
-  }
-
-  nsAutoString direction;
-  GetSelectionDirection(direction, aRv);
-  if (aRv.Failed()) {
-    return;
-  }
-
-  int32_t start, end;
-  GetSelectionRange(&start, &end, aRv);
-  if (aRv.Failed()) {
-    return;
-  }
-
-  start = selStart;
-  if (end < start) {
-    end = start;
-  }
-
-  aRv = SetSelectionRange(start, end, direction);
+  MOZ_ASSERT(state, "SupportsTextSelection() returned true!");
+  state->SetSelectionStart(aSelectionStart, aRv);
 }
 
 Nullable<int32_t>
 HTMLInputElement::GetSelectionEnd(ErrorResult& aRv)
 {
   if (!SupportsTextSelection()) {
     return Nullable<int32_t>();
   }
--- a/dom/html/HTMLInputElement.h
+++ b/dom/html/HTMLInputElement.h
@@ -241,17 +241,17 @@ public:
   NS_IMETHOD_(void) OnValueChanged(bool aNotify, bool aWasInteractiveUserChange) override;
   NS_IMETHOD_(bool) HasCachedSelection() override;
   virtual void GetSelectionRange(int32_t* aSelectionStart,
                                  int32_t* aSelectionEnd,
                                  ErrorResult& aRv) override;
 
   // Methods for nsFormFillController so it can do selection operations on input
   // types the HTML spec doesn't support them on, like "email".
-  int32_t GetSelectionStartIgnoringType(ErrorResult& aRv);
+  uint32_t GetSelectionStartIgnoringType(ErrorResult& aRv);
   int32_t GetSelectionEndIgnoringType(ErrorResult& aRv);
 
   void GetDisplayFileName(nsAString& aFileName) const;
 
   const nsTArray<OwningFileOrDirectory>& GetFilesOrDirectoriesInternal() const
   {
     return mFilesOrDirectories;
   }
@@ -712,18 +712,18 @@ public:
   Decimal GetStep() const;
 
   void GetValidationMessage(nsAString& aValidationMessage, ErrorResult& aRv);
 
   // XPCOM GetCustomVisibility() is OK
 
   // XPCOM Select() is OK
 
-  Nullable<int32_t> GetSelectionStart(ErrorResult& aRv);
-  void SetSelectionStart(const Nullable<int32_t>& aValue, ErrorResult& aRv);
+  Nullable<uint32_t> GetSelectionStart(ErrorResult& aRv);
+  void SetSelectionStart(const Nullable<uint32_t>& aValue, ErrorResult& aRv);
 
   Nullable<int32_t> GetSelectionEnd(ErrorResult& aRv);
   void SetSelectionEnd(const Nullable<int32_t>& aValue, ErrorResult& aRv);
 
   void GetSelectionDirection(nsAString& aValue, ErrorResult& aRv);
   void SetSelectionDirection(const nsAString& aValue, ErrorResult& aRv);
 
   void SetSelectionRange(int32_t aStart, int32_t aEnd,
--- a/dom/html/HTMLTextAreaElement.cpp
+++ b/dom/html/HTMLTextAreaElement.cpp
@@ -705,44 +705,17 @@ HTMLTextAreaElement::GetSelectionStart(E
   GetSelectionRange(&selStart, &selEnd, aError);
   return Nullable<uint32_t>(selStart);
 }
 
 void
 HTMLTextAreaElement::SetSelectionStart(const Nullable<uint32_t>& aSelectionStart,
                                        ErrorResult& aError)
 {
-  int32_t selStart = 0;
-  if (!aSelectionStart.IsNull()) {
-    selStart = aSelectionStart.Value();
-  }
-
-  if (mState.IsSelectionCached()) {
-    mState.GetSelectionProperties().SetStart(selStart);
-    return;
-  }
-
-  nsAutoString direction;
-  GetSelectionDirection(direction, aError);
-  if (aError.Failed()) {
-    return;
-  }
-  int32_t start, end;
-  GetSelectionRange(&start, &end, aError);
-  if (aError.Failed()) {
-    return;
-  }
-  start = selStart;
-  if (end < start) {
-    end = start;
-  }
-  nsresult rv = SetSelectionRange(start, end, direction);
-  if (NS_FAILED(rv)) {
-    aError.Throw(rv);
-  }
+  mState.SetSelectionStart(aSelectionStart, aError);
 }
 
 Nullable<uint32_t>
 HTMLTextAreaElement::GetSelectionEnd(ErrorResult& aError)
 {
   int32_t selStart, selEnd;
   GetSelectionRange(&selStart, &selEnd, aError);
   return Nullable<uint32_t>(selEnd);
--- a/dom/html/nsTextEditorState.cpp
+++ b/dom/html/nsTextEditorState.cpp
@@ -1703,16 +1703,45 @@ nsTextEditorState::SetSelectionRange(int
     asyncDispatcher->PostDOMEvent();
   }
 
   if (NS_FAILED(rv)) {
     aRv.Throw(rv);
   }
 }
 
+void
+nsTextEditorState::SetSelectionStart(const mozilla::dom::Nullable<uint32_t>& aStart,
+                                     ErrorResult& aRv)
+{
+  int32_t start = 0;
+  if (!aStart.IsNull()) {
+    // XXXbz This will do the wrong thing for input values that are out of the
+    // int32_t range...
+    start = aStart.Value();
+  }
+
+  int32_t ignored, end;
+  GetSelectionRange(&ignored, &end, aRv);
+  if (aRv.Failed()) {
+    return;
+  }
+
+  nsITextControlFrame::SelectionDirection dir = GetSelectionDirection(aRv);
+  if (aRv.Failed()) {
+    return;
+  }
+
+  if (end < start) {
+    end = start;
+  }
+
+  SetSelectionRange(start, end, dir, aRv);
+}
+
 HTMLInputElement*
 nsTextEditorState::GetParentNumberControl(nsFrame* aFrame) const
 {
   MOZ_ASSERT(aFrame);
   nsIContent* content = aFrame->GetContent();
   MOZ_ASSERT(content);
   nsIContent* parent = content->GetParent();
   if (!parent) {
--- a/dom/html/nsTextEditorState.h
+++ b/dom/html/nsTextEditorState.h
@@ -10,16 +10,17 @@
 #include "nsString.h"
 #include "nsITextControlElement.h"
 #include "nsITextControlFrame.h"
 #include "nsCycleCollectionParticipant.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/WeakPtr.h"
+#include "mozilla/dom/Nullable.h"
 
 class nsTextInputListener;
 class nsTextControlFrame;
 class nsTextInputSelectionImpl;
 class nsAnonDivObserver;
 class nsISelectionController;
 class nsFrameSelection;
 class nsIEditor;
@@ -292,16 +293,22 @@ public:
   // If we have a frame, this method will scroll the selection into view.
   //
   // XXXbz This should really take uint32_t, but none of our guts (either the
   // frame or our cached selection state) work with uint32_t at the moment...
   void SetSelectionRange(int32_t aStart, int32_t aEnd,
                          nsITextControlFrame::SelectionDirection aDirection,
                          mozilla::ErrorResult& aRv);
 
+  // Set the selection start.  This basically implements the
+  // https://html.spec.whatwg.org/multipage/forms.html#dom-textarea/input-selectionstart
+  // setter.
+  void SetSelectionStart(const mozilla::dom::Nullable<uint32_t>& aStart,
+                         mozilla::ErrorResult& aRv);
+
   void UpdateEditableState(bool aNotify) {
     if (mRootNode) {
       mRootNode->UpdateEditableState(aNotify);
     }
   }
 
 private:
   friend class RestoreSelectionState;
--- a/dom/webidl/HTMLInputElement.webidl
+++ b/dom/webidl/HTMLInputElement.webidl
@@ -112,18 +112,17 @@ interface HTMLInputElement : HTMLElement
   boolean reportValidity();
   void setCustomValidity(DOMString error);
 
   // Bug 850365 readonly attribute NodeList labels;
 
   void select();
 
   [Throws]
-           // TODO: unsigned vs signed
-           attribute long? selectionStart;
+           attribute unsigned long? selectionStart;
   [Throws]
            attribute long? selectionEnd;
   [Throws]
            attribute DOMString? selectionDirection;
   [Throws]
   void setRangeText(DOMString replacement);
   [Throws]
   void setRangeText(DOMString replacement, unsigned long start,
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -93537,16 +93537,22 @@
     ]
    ],
    "html/semantics/forms/textfieldselection/selection-not-application.html": [
     [
      "/html/semantics/forms/textfieldselection/selection-not-application.html",
      {}
     ]
    ],
+   "html/semantics/forms/textfieldselection/selection-start-end.html": [
+    [
+     "/html/semantics/forms/textfieldselection/selection-start-end.html",
+     {}
+    ]
+   ],
    "html/semantics/forms/textfieldselection/selection.html": [
     [
      "/html/semantics/forms/textfieldselection/selection.html",
      {
       "timeout": "long"
      }
     ]
    ],
@@ -176658,22 +176664,26 @@
   "html/semantics/forms/textfieldselection/selection-not-application-textarea.html": [
    "629f3682421060ea18e9046a637402212be1b1fd",
    "testharness"
   ],
   "html/semantics/forms/textfieldselection/selection-not-application.html": [
    "39ecb031aca3655a06152f94a514981fe59ebbaf",
    "testharness"
   ],
+  "html/semantics/forms/textfieldselection/selection-start-end.html": [
+   "1f3184b72aba5631d6db4379dfa98035ee047283",
+   "testharness"
+  ],
   "html/semantics/forms/textfieldselection/selection.html": [
-   "d869799718137671a2eacc323aa26ea4364e845f",
+   "f7674721b84ec8fca0e5e40258447ce857b87784",
    "testharness"
   ],
   "html/semantics/forms/textfieldselection/textfieldselection-setRangeText.html": [
-   "0caf2b08ccc5a35578291af8f5adaf7de9537d66",
+   "3bbd350321f5ec9e0a8f3d47da4e11aaa3ad4d68",
    "testharness"
   ],
   "html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html": [
    "462049246a2ef3e66c22017ec6ad362e07b467e6",
    "testharness"
   ],
   "html/semantics/forms/the-button-element/.gitkeep": [
    "da39a3ee5e6b4b0d3255bfef95601890afd80709",
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/html/semantics/forms/textfieldselection/selection-start-end.html
@@ -0,0 +1,68 @@
+<!doctype html>
+<meta charset=utf-8>
+<title></title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<div id=log></div>
+<script>
+  function createInputElement(value, append) {
+    var el = document.createElement("input");
+    el.type = "text";
+    el.value = value;
+    el.id = "input" + (append ? "-appended" : "-not-appended");
+    if (append) {
+      document.body.appendChild(el);
+    }
+    return el;
+  };
+
+  function createTextareaElement(value, append) {
+    var el = document.createElement("textarea");
+    el.value = value;
+
+    el.id = "textarea" + (append ? "-appended" : "-not-appended");
+    if (append) {
+      document.body.appendChild(el);
+    }
+    return el;
+  };
+
+  function createTestElements(value) {
+    return [ createInputElement(value, true),
+             createInputElement(value, false),
+             createTextareaElement(value, true),
+             createTextareaElement(value, false) ];
+  }
+
+  const testValue = "abcdefghij";
+
+  test(function() {
+    assert_equals(testValue.length, 10);
+  }, "Sanity check for testValue length; if this fails, variou absolute offsets in the test below need to be adjusted to be less than testValue.length");
+
+  test(function() {
+    for (let el of createTestElements(testValue)) {
+      assert_equals(el.selectionStart, testValue.length,
+                    `Initial .value set on ${el.id} should set selectionStart to end of value`);
+      var t = async_test(`onselect should fire when selectionStart is changed on ${el.id}`);
+      el.onselect = t.step_func_done(function(e) {
+        assert_equals(e.type, "select");
+        el.remove();
+      });
+      el.selectionStart = 2;
+    }
+  }, "onselect should fire when selectionStart is changed");
+
+  test(function() {
+    for (let el of createTestElements(testValue)) {
+      assert_equals(el.selectionStart, testValue.length,
+                    `Initial .value set on ${el.id} should set selectionStart to end of value`);
+      el.selectionStart = 0;
+      el.selectionEnd = 5;
+      el.selectionStart = 8;
+      assert_equals(el.selectionStart, 8, `selectionStart on ${el.id}`);
+      assert_equals(el.selectionEnd, 8, `selectionEnd on ${el.id}`);
+      el.remove();
+    }
+  }, "Setting selectionStart to a value larger than selectionEnd should increase selectionEnd");
+</script>
--- a/testing/web-platform/tests/html/semantics/forms/textfieldselection/textfieldselection-setRangeText.html
+++ b/testing/web-platform/tests/html/semantics/forms/textfieldselection/textfieldselection-setRangeText.html
@@ -101,20 +101,28 @@
 
     test(function(){
       assert_throws(new TypeError(), function() {
         element.setRangeText();
       });
     }, element.id + " setRangeText without argument throws a type error");
 
     async_test(function() {
-      var q = false;
-      element.onselect = this.step_func_done(function(e) {
-        assert_true(q, "event should be queued");
-        assert_true(e.isTrusted, "event is trusted");
-        assert_true(e.bubbles, "event bubbles");
-        assert_false(e.cancelable, "event is not cancelable");
-      });
-      element.setRangeText("foobar2", 0, 6);
-      q = true;
+      // At this point there are already "select" events queued up on
+      // "element".  Give them time to fire; otherwise we can get spurious
+      // passes.
+      //
+      // This is unfortunately racy in that we might _still_ get spurious
+      // passes.  I'm not sure how best to handle that.
+      setTimeout(this.step_func(function() {
+        var q = false;
+        element.onselect = this.step_func_done(function(e) {
+          assert_true(q, "event should be queued");
+          assert_true(e.isTrusted, "event is trusted");
+          assert_true(e.bubbles, "event bubbles");
+          assert_false(e.cancelable, "event is not cancelable");
+        });
+        element.setRangeText("foobar2", 0, 6);
+        q = true;
+      }), 10);
     }, element.id + " setRangeText fires a select event");
   })
 </script>