Skip to content

Commit

Permalink
Fix selection{Start,End} when selectionDirection is "backward"
Browse files Browse the repository at this point in the history
Per the spec, selectionStart and selectionEnd should return the same
values regardless of the selectionDirection. (That is, selectionStart is
always less than or equal to selectionEnd; the direction then implies
which of selectionStart or selectionEnd is the cursor position.)

There was no explicit WPT test for this, so I added one.

This bug was initially quite hard to wrap my head around, and I think
part of the problem is the code in TextInput. Therefore, in the process
of fixing it I have refactored the implementation of TextInput:

* Rename selection_begin to selection_origin. This value doesn't
  necessarily correspond directly to the selectionStart DOM value - in
  the case of a backward selection, it corresponds to selectionEnd.
  I feel that "origin" doesn't imply a specific ordering as strongly as
  "begin" (or "start" for that matter) does.

* In various other cases where "begin" is used as a synonym for "start",
  just use "start" for consistency.

* Implement selection_start() and selection_end() methods (and their
  _offset() variants) which directly correspond to their DOM
  equivalents.

* Rename other related methods to make them less wordy and more
  consistent / intention-revealing.

* Add assertions to assert_ok_selection() to ensure that our assumptions
  about the ordering of selection_origin and edit_point are met. This
  then revealed a bug in adjust_selection_for_horizontal_change() where
  the value of selection_direction was not maintained correctly (causing
  a unit test failure when the new assertion failed).
  • Loading branch information
jonleighton committed Jan 26, 2018
1 parent 0148e97 commit 02883a6
Show file tree
Hide file tree
Showing 8 changed files with 210 additions and 168 deletions.
4 changes: 2 additions & 2 deletions components/script/dom/htmlinputelement.rs
Expand Up @@ -374,7 +374,7 @@ impl LayoutHTMLInputElementHelpers for LayoutDom<HTMLInputElement> {
match (*self.unsafe_get()).input_type() {
InputType::Password => {
let text = get_raw_textinput_value(self);
let sel = textinput.get_absolute_selection_range();
let sel = textinput.sorted_selection_offsets_range();

// Translate indices from the raw value to indices in the replacement value.
let char_start = text[.. sel.start].chars().count();
Expand All @@ -383,7 +383,7 @@ impl LayoutHTMLInputElementHelpers for LayoutDom<HTMLInputElement> {
let bytes_per_char = PASSWORD_REPLACEMENT_CHAR.len_utf8();
Some(char_start * bytes_per_char .. char_end * bytes_per_char)
}
input_type if input_type.is_textual() => Some(textinput.get_absolute_selection_range()),
input_type if input_type.is_textual() => Some(textinput.sorted_selection_offsets_range()),
_ => None
}
}
Expand Down
6 changes: 3 additions & 3 deletions components/script/dom/htmltextareaelement.rs
Expand Up @@ -81,7 +81,7 @@ impl LayoutHTMLTextAreaElementHelpers for LayoutDom<HTMLTextAreaElement> {
return None;
}
let textinput = (*self.unsafe_get()).textinput.borrow_for_layout();
Some(textinput.get_absolute_selection_range())
Some(textinput.sorted_selection_offsets_range())
}

#[allow(unsafe_code)]
Expand Down Expand Up @@ -247,7 +247,7 @@ impl HTMLTextAreaElementMethods for HTMLTextAreaElement {

// Step 1
let old_value = textinput.get_content();
let old_selection = textinput.selection_begin;
let old_selection = textinput.selection_origin;

// Step 2
textinput.set_content(value);
Expand All @@ -259,7 +259,7 @@ impl HTMLTextAreaElementMethods for HTMLTextAreaElement {
// Step 4
textinput.adjust_horizontal_to_limit(Direction::Forward, Selection::NotSelected);
} else {
textinput.selection_begin = old_selection;
textinput.selection_origin = old_selection;
}

self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
Expand Down
4 changes: 2 additions & 2 deletions components/script/dom/textcontrol.rs
Expand Up @@ -123,11 +123,11 @@ pub trait TextControl: DerivedFrom<EventTarget> + DerivedFrom<Node> {
}

fn selection_start(&self) -> u32 {
self.textinput().borrow().get_selection_start()
self.textinput().borrow().selection_start_offset() as u32
}

fn selection_end(&self) -> u32 {
self.textinput().borrow().get_absolute_insertion_point() as u32
self.textinput().borrow().selection_end_offset() as u32
}

fn selection_direction(&self) -> SelectionDirection {
Expand Down

0 comments on commit 02883a6

Please sign in to comment.