servo: Merge #19461 - Handle cases where selection API doesn't apply (from jonleighton:issue-19171-4); r=KiChjang
authorJon Leighton <j@jonathanleighton.com>
Sun, 10 Dec 2017 18:37:58 -0600
changeset 396001 6d431fb67f3a76e20da22d11782d7fc9a65b1a75
parent 396000 52fbf40d3e7d0ee443fe8ee3c0d360c2ce87d2ce
child 396002 b6b78f4aa40fc67b891fef7c6fb995e3e380379f
push id33067
push usertoros@mozilla.com
push dateMon, 11 Dec 2017 09:54:39 +0000
treeherdermozilla-central@6d926a50dcf5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersKiChjang
milestone59.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
servo: Merge #19461 - Handle cases where selection API doesn't apply (from jonleighton:issue-19171-4); r=KiChjang The selection API only applies to certain <input> types: https://html.spec.whatwg.org/multipage/#do-not-apply This commit ensures that we handle that correctly. Some notes: 1. TextControl::set_dom_selection_direction now calls set_selection_range(), which means that setting selectionDirection will now fire a selection event, as it should per the spec. 2. There is a test for the firing of the select event in tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html, however the test did not run due to this syntax error: (pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration" This happens due to the us of the "for (const foo of ...)" construct. Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of this should actually work, so it's somewhat unsatisfying to have to change the test. 3. I removed tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application-textarea.html because it doesn't seem to add any extra value - the selection API always applies to textarea elements, and the API is tested elsewhere. 4. If an `<input>`'s type is unset, it defaults to a text, and the selection API applies. Also, if an `<input>`'s type is set to an invalid value, it defaults to a text too. This second case doesn't currently work, and I'll need to do more restructuring of the code in a future commit. See discussion with nox in IRC: https://mozilla.logbot.info/servo/20171201#c13946454-c13946594 Source-Repo: https://github.com/servo/servo Source-Revision: db41cc00be93523114092125859534b107ec772a
servo/components/script/dom/htmlinputelement.rs
servo/components/script/dom/htmltextareaelement.rs
servo/components/script/dom/textcontrol.rs
servo/components/script/dom/webidls/HTMLInputElement.webidl
servo/components/script/dom/webidls/HTMLTextAreaElement.webidl
servo/components/script/textinput.rs
servo/tests/unit/script/textinput.rs
--- a/servo/components/script/dom/htmlinputelement.rs
+++ b/servo/components/script/dom/htmlinputelement.rs
@@ -400,16 +400,28 @@ impl LayoutHTMLInputElementHelpers for L
         self.upcast::<Element>().get_state_for_layout().contains(ElementState::IN_INDETERMINATE_STATE)
     }
 }
 
 impl TextControl for HTMLInputElement {
     fn textinput(&self) -> &DomRefCell<TextInput<ScriptToConstellationChan>> {
         &self.textinput
     }
+
+    // https://html.spec.whatwg.org/multipage/#concept-input-apply
+    fn selection_api_applies(&self) -> bool {
+        match self.input_type() {
+            InputType::Text | InputType::Search | InputType::Url
+            | InputType::Tel | InputType::Password => {
+                true
+            },
+
+            _ => false
+        }
+    }
 }
 
 impl HTMLInputElementMethods for HTMLInputElement {
     // https://html.spec.whatwg.org/multipage/#dom-input-accept
     make_getter!(Accept, "accept");
 
     // https://html.spec.whatwg.org/multipage/#dom-input-accept
     make_setter!(SetAccept, "accept");
@@ -671,48 +683,48 @@ impl HTMLInputElementMethods for HTMLInp
             let window = window_from_node(self);
             NodeList::empty(&window)
         } else {
             self.upcast::<HTMLElement>().labels()
         }
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn SelectionStart(&self) -> u32 {
-        self.dom_selection_start()
+    fn GetSelectionStart(&self) -> Option<u32> {
+        self.get_dom_selection_start()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn SetSelectionStart(&self, start: u32) {
-        self.set_dom_selection_start(start);
+    fn SetSelectionStart(&self, start: Option<u32>) -> ErrorResult {
+        self.set_dom_selection_start(start)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn SelectionEnd(&self) -> u32 {
-        self.dom_selection_end()
+    fn GetSelectionEnd(&self) -> Option<u32> {
+        self.get_dom_selection_end()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn SetSelectionEnd(&self, end: u32) {
+    fn SetSelectionEnd(&self, end: Option<u32>) -> ErrorResult {
         self.set_dom_selection_end(end)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn SelectionDirection(&self) -> DOMString {
-        self.dom_selection_direction()
+    fn GetSelectionDirection(&self) -> Option<DOMString> {
+        self.get_dom_selection_direction()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn SetSelectionDirection(&self, direction: DOMString) {
-        self.set_dom_selection_direction(direction);
+    fn SetSelectionDirection(&self, direction: Option<DOMString>) -> ErrorResult {
+        self.set_dom_selection_direction(direction)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-setselectionrange
-    fn SetSelectionRange(&self, start: u32, end: u32, direction: Option<DOMString>) {
-        self.set_dom_selection_range(start, end, direction);
+    fn SetSelectionRange(&self, start: u32, end: u32, direction: Option<DOMString>) -> ErrorResult {
+        self.set_dom_selection_range(start, end, direction)
     }
 
     // Select the files based on filepaths passed in,
     // enabled by dom.htmlinputelement.select_files.enabled,
     // used for test purpose.
     // check-tidy: no specs after this line
     fn SelectFiles(&self, paths: Vec<DOMString>) {
         if self.input_type() == InputType::File {
--- a/servo/components/script/dom/htmltextareaelement.rs
+++ b/servo/components/script/dom/htmltextareaelement.rs
@@ -3,16 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use dom::attr::Attr;
 use dom::bindings::cell::DomRefCell;
 use dom::bindings::codegen::Bindings::EventBinding::EventMethods;
 use dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding;
 use dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::HTMLTextAreaElementMethods;
 use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
+use dom::bindings::error::ErrorResult;
 use dom::bindings::inheritance::Castable;
 use dom::bindings::root::{DomRoot, LayoutDom, MutNullableDom};
 use dom::bindings::str::DOMString;
 use dom::document::Document;
 use dom::element::{AttributeMutation, Element};
 use dom::element::RawLayoutElementHelpers;
 use dom::event::{Event, EventBubbles, EventCancelable};
 use dom::globalscope::GlobalScope;
@@ -140,16 +141,20 @@ impl HTMLTextAreaElement {
         el.set_placeholder_shown_state(has_placeholder);
     }
 }
 
 impl TextControl for HTMLTextAreaElement {
     fn textinput(&self) -> &DomRefCell<TextInput<ScriptToConstellationChan>> {
         &self.textinput
     }
+
+    fn selection_api_applies(&self) -> bool {
+        true
+    }
 }
 
 impl HTMLTextAreaElementMethods for HTMLTextAreaElement {
     // TODO A few of these attributes have default values and additional
     // constraints
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea-cols
     make_uint_getter!(Cols, "cols", DEFAULT_COLS);
@@ -255,48 +260,48 @@ impl HTMLTextAreaElementMethods for HTML
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
     fn Labels(&self) -> DomRoot<NodeList> {
         self.upcast::<HTMLElement>().labels()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn SelectionStart(&self) -> u32 {
-        self.dom_selection_start()
+    fn GetSelectionStart(&self) -> Option<u32> {
+        self.get_dom_selection_start()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn SetSelectionStart(&self, start: u32) {
-        self.set_dom_selection_start(start);
+    fn SetSelectionStart(&self, start: Option<u32>) -> ErrorResult {
+        self.set_dom_selection_start(start)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn SelectionEnd(&self) -> u32 {
-        self.dom_selection_end()
+    fn GetSelectionEnd(&self) -> Option<u32> {
+        self.get_dom_selection_end()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn SetSelectionEnd(&self, end: u32) {
-        self.set_dom_selection_end(end);
+    fn SetSelectionEnd(&self, end: Option<u32>) -> ErrorResult {
+        self.set_dom_selection_end(end)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn SelectionDirection(&self) -> DOMString {
-        self.dom_selection_direction()
+    fn GetSelectionDirection(&self) -> Option<DOMString> {
+        self.get_dom_selection_direction()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn SetSelectionDirection(&self, direction: DOMString) {
-        self.set_dom_selection_direction(direction);
+    fn SetSelectionDirection(&self, direction: Option<DOMString>) -> ErrorResult {
+        self.set_dom_selection_direction(direction)
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-setselectionrange
-    fn SetSelectionRange(&self, start: u32, end: u32, direction: Option<DOMString>) {
-        self.set_dom_selection_range(start, end, direction);
+    fn SetSelectionRange(&self, start: u32, end: u32, direction: Option<DOMString>) -> ErrorResult {
+        self.set_dom_selection_range(start, end, direction)
     }
 }
 
 
 impl HTMLTextAreaElement {
     pub fn reset(&self) {
         // https://html.spec.whatwg.org/multipage/#the-textarea-element:concept-form-reset-control
         self.SetValue(self.DefaultValue());
--- a/servo/components/script/dom/textcontrol.rs
+++ b/servo/components/script/dom/textcontrol.rs
@@ -1,82 +1,142 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use dom::bindings::cell::DomRefCell;
 use dom::bindings::conversions::DerivedFrom;
+use dom::bindings::error::{Error, ErrorResult};
 use dom::bindings::str::DOMString;
 use dom::event::{EventBubbles, EventCancelable};
 use dom::eventtarget::EventTarget;
 use dom::node::{Node, NodeDamage, window_from_node};
 use script_traits::ScriptToConstellationChan;
 use textinput::{SelectionDirection, TextInput};
 
 pub trait TextControl: DerivedFrom<EventTarget> + DerivedFrom<Node> {
     fn textinput(&self) -> &DomRefCell<TextInput<ScriptToConstellationChan>>;
+    fn selection_api_applies(&self) -> bool;
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn dom_selection_start(&self) -> u32 {
-        self.textinput().borrow().get_selection_start()
+    fn get_dom_selection_start(&self) -> Option<u32> {
+        // Step 1
+        if !self.selection_api_applies() {
+            return None;
+        }
+
+        // Steps 2-3
+        Some(self.selection_start())
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
-    fn set_dom_selection_start(&self, start: u32) {
+    fn set_dom_selection_start(&self, start: Option<u32>) -> ErrorResult {
+        // Step 1
+        if !self.selection_api_applies() {
+            return Err(Error::InvalidState);
+        }
+
         // Step 2
-        let mut end = self.dom_selection_end();
+        let mut end = self.selection_end();
 
         // Step 3
-        if end < start {
-            end = start;
+        if let Some(s) = start {
+            if end < s {
+                end = s;
+            }
         }
 
         // Step 4
-        self.set_selection_range(start, end, self.selection_direction());
+        self.set_selection_range(start, Some(end), Some(self.selection_direction()));
+        Ok(())
+    }
+
+    // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
+    fn get_dom_selection_end(&self) -> Option<u32> {
+        // Step 1
+        if !self.selection_api_applies() {
+            return None;
+        }
+
+        // Steps 2-3
+        Some(self.selection_end())
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn dom_selection_end(&self) -> u32 {
-        self.textinput().borrow().get_absolute_insertion_point() as u32
+    fn set_dom_selection_end(&self, end: Option<u32>) -> ErrorResult {
+        // Step 1
+        if !self.selection_api_applies() {
+            return Err(Error::InvalidState);
+        }
+
+        // Step 2
+        self.set_selection_range(Some(self.selection_start()), end, Some(self.selection_direction()));
+        Ok(())
     }
 
-    // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionend
-    fn set_dom_selection_end(&self, end: u32) {
-        self.set_selection_range(self.dom_selection_start(), end, self.selection_direction());
+    // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
+    fn get_dom_selection_direction(&self) -> Option<DOMString> {
+        // Step 1
+        if !self.selection_api_applies() {
+            return None;
+        }
+
+        Some(DOMString::from(self.selection_direction()))
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn dom_selection_direction(&self) -> DOMString {
-        DOMString::from(self.selection_direction())
-    }
+    fn set_dom_selection_direction(&self, direction: Option<DOMString>) -> ErrorResult {
+        // Step 1
+        if !self.selection_api_applies() {
+            return Err(Error::InvalidState);
+        }
 
-    // https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectiondirection
-    fn set_dom_selection_direction(&self, direction: DOMString) {
-        self.textinput().borrow_mut().selection_direction = SelectionDirection::from(direction);
+        // Step 2
+        self.set_selection_range(
+            Some(self.selection_start()),
+            Some(self.selection_end()),
+            direction.map(|d| SelectionDirection::from(d))
+        );
+        Ok(())
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-setselectionrange
-    fn set_dom_selection_range(&self, start: u32, end: u32, direction: Option<DOMString>) {
-        // Step 4
-        let direction = direction.map_or(SelectionDirection::None, |d| SelectionDirection::from(d));
+    fn set_dom_selection_range(&self, start: u32, end: u32, direction: Option<DOMString>) -> ErrorResult {
+        // Step 1
+        if !self.selection_api_applies() {
+            return Err(Error::InvalidState);
+        }
 
-        self.set_selection_range(start, end, direction);
+        // Step 2
+        self.set_selection_range(Some(start), Some(end), direction.map(|d| SelectionDirection::from(d)));
+        Ok(())
+    }
+
+    fn selection_start(&self) -> u32 {
+        self.textinput().borrow().get_selection_start()
+    }
+
+    fn selection_end(&self) -> u32 {
+        self.textinput().borrow().get_absolute_insertion_point() as u32
     }
 
     fn selection_direction(&self) -> SelectionDirection {
         self.textinput().borrow().selection_direction
     }
 
     // https://html.spec.whatwg.org/multipage/#set-the-selection-range
-    fn set_selection_range(&self, start: u32, end: u32, direction: SelectionDirection) {
-        // Step 5
-        self.textinput().borrow_mut().selection_direction = direction;
+    fn set_selection_range(&self, start: Option<u32>, end: Option<u32>, direction: Option<SelectionDirection>) {
+        // Step 1
+        let start = start.unwrap_or(0);
 
-        // Step 3
-        self.textinput().borrow_mut().set_selection_range(start, end);
+        // Step 2
+        let end = end.unwrap_or(0);
+
+        // Steps 3-5
+        self.textinput().borrow_mut().set_selection_range(start, end, direction.unwrap_or(SelectionDirection::None));
 
         // Step 6
         let window = window_from_node(self);
         let _ = window.user_interaction_task_source().queue_event(
             &self.upcast::<EventTarget>(),
             atom!("select"),
             EventBubbles::Bubbles,
             EventCancelable::NotCancelable,
--- a/servo/components/script/dom/webidls/HTMLInputElement.webidl
+++ b/servo/components/script/dom/webidls/HTMLInputElement.webidl
@@ -85,23 +85,27 @@ interface HTMLInputElement : HTMLElement
   //readonly attribute DOMString validationMessage;
   //boolean checkValidity();
   //boolean reportValidity();
   //void setCustomValidity(DOMString error);
 
   readonly attribute NodeList labels;
 
   //void select();
-           attribute unsigned long selectionStart;
-           attribute unsigned long selectionEnd;
-           attribute DOMString selectionDirection;
+  [SetterThrows]
+           attribute unsigned long? selectionStart;
+  [SetterThrows]
+           attribute unsigned long? selectionEnd;
+  [SetterThrows]
+           attribute DOMString? selectionDirection;
   //void setRangeText(DOMString replacement);
   //void setRangeText(DOMString replacement, unsigned long start, unsigned long end,
   //                  optional SelectionMode selectionMode = "preserve");
-  void setSelectionRange(unsigned long start, unsigned long end, optional DOMString direction);
+  [Throws]
+           void setSelectionRange(unsigned long start, unsigned long end, optional DOMString direction);
 
   // also has obsolete members
 
   // Select with file-system paths for testing purpose
   [Pref="dom.testing.htmlinputelement.select_files.enabled"]
   void selectFiles(sequence<DOMString> path);
 
 };
--- a/servo/components/script/dom/webidls/HTMLTextAreaElement.webidl
+++ b/servo/components/script/dom/webidls/HTMLTextAreaElement.webidl
@@ -46,16 +46,20 @@ interface HTMLTextAreaElement : HTMLElem
   // readonly attribute DOMString validationMessage;
   // boolean checkValidity();
   // boolean reportValidity();
   // void setCustomValidity(DOMString error);
 
   readonly attribute NodeList labels;
 
   // void select();
-           attribute unsigned long selectionStart;
-           attribute unsigned long selectionEnd;
-           attribute DOMString selectionDirection;
+  [SetterThrows]
+           attribute unsigned long? selectionStart;
+  [SetterThrows]
+           attribute unsigned long? selectionEnd;
+  [SetterThrows]
+           attribute DOMString? selectionDirection;
   // void setRangeText(DOMString replacement);
   // void setRangeText(DOMString replacement, unsigned long start, unsigned long end,
   //                   optional SelectionMode selectionMode = "preserve");
-  void setSelectionRange(unsigned long start, unsigned long end, optional DOMString direction);
+  [Throws]
+           void setSelectionRange(unsigned long start, unsigned long end, optional DOMString direction);
 };
--- a/servo/components/script/textinput.rs
+++ b/servo/components/script/textinput.rs
@@ -16,17 +16,17 @@ use std::usize;
 use unicode_segmentation::UnicodeSegmentation;
 
 #[derive(Clone, Copy, PartialEq)]
 pub enum Selection {
     Selected,
     NotSelected
 }
 
-#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
+#[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf, PartialEq)]
 pub enum SelectionDirection {
     Forward,
     Backward,
     None,
 }
 
 impl From<DOMString> for SelectionDirection {
     fn from(direction: DOMString) -> SelectionDirection {
@@ -820,29 +820,31 @@ impl<T: ClipboardProvider> TextInput<T> 
             }
         });
 
         TextPoint {
             line: line, index: index
         }
     }
 
-    pub fn set_selection_range(&mut self, start: u32, end: u32) {
+    pub fn set_selection_range(&mut self, start: u32, end: u32, direction: SelectionDirection) {
         let mut start = start as usize;
         let mut end = end as usize;
         let text_end = self.get_content().len();
 
         if end > text_end {
             end = text_end;
         }
         if start > end {
             start = end;
         }
 
-        match self.selection_direction {
+        self.selection_direction = direction;
+
+        match direction {
             SelectionDirection::None |
             SelectionDirection::Forward => {
                 self.selection_begin = Some(self.get_text_point_for_absolute_point(start));
                 self.edit_point = self.get_text_point_for_absolute_point(end);
             },
             SelectionDirection::Backward => {
                 self.selection_begin = Some(self.get_text_point_for_absolute_point(end));
                 self.edit_point = self.get_text_point_for_absolute_point(start);
--- a/servo/tests/unit/script/textinput.rs
+++ b/servo/tests/unit/script/textinput.rs
@@ -578,29 +578,29 @@ fn test_textinput_cursor_position_correc
     assert_eq!(textinput.edit_point.index, 0);
     assert_eq!(textinput.edit_point.line, 0);
 }
 
 
 #[test]
 fn test_textinput_set_selection_with_direction() {
     let mut textinput = text_input(Lines::Single, "abcdef");
-    textinput.selection_direction = SelectionDirection::Forward;
-    textinput.set_selection_range(2, 6);
+    textinput.set_selection_range(2, 6, SelectionDirection::Forward);
     assert_eq!(textinput.edit_point.line, 0);
     assert_eq!(textinput.edit_point.index, 6);
+    assert_eq!(textinput.selection_direction, SelectionDirection::Forward);
 
     assert!(textinput.selection_begin.is_some());
     assert_eq!(textinput.selection_begin.unwrap().line, 0);
     assert_eq!(textinput.selection_begin.unwrap().index, 2);
 
-    textinput.selection_direction = SelectionDirection::Backward;
-    textinput.set_selection_range(2, 6);
+    textinput.set_selection_range(2, 6, SelectionDirection::Backward);
     assert_eq!(textinput.edit_point.line, 0);
     assert_eq!(textinput.edit_point.index, 2);
+    assert_eq!(textinput.selection_direction, SelectionDirection::Backward);
 
     assert!(textinput.selection_begin.is_some());
     assert_eq!(textinput.selection_begin.unwrap().line, 0);
     assert_eq!(textinput.selection_begin.unwrap().index, 6);
 }
 
 #[test]
 fn test_textinput_unicode_handling() {