Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-08-06 22:15:33 +01:00
Code Issues Projects Releases Packages Wiki Activity

Fix selection{Start,End} when selectionDirection is "backward"

Browse source 
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).
This commit is contained in:
Jon Leighton 2017-12-09 20:17:49 +01:00
parent 0148e9705b
commit 02883a6f54
8 changed files with 221 additions and 179 deletions
Download patch file Download diff file Expand all files Collapse all files

4
components/script/dom/htmlinputelement.rs
View file

@ -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();
@ -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
}
}

6
components/script/dom/htmltextareaelement.rs
View file

@ -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)]
@ -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);
@ -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);

4
components/script/dom/textcontrol.rs
View file

@ -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 {

187
components/script/textinput.rs
View file

@ -48,7 +48,7 @@ impl From<SelectionDirection> for DOMString {
}
}
#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
#[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf, PartialEq, PartialOrd)]
pub struct TextPoint {
/// 0-based line number
pub line: usize,
@ -63,8 +63,9 @@ pub struct TextInput<T: ClipboardProvider> {
lines: Vec<DOMString>,
/// Current cursor input point
pub edit_point: TextPoint,
/// Beginning of selection range with edit_point as end that can span multiple lines.
pub selection_begin: Option<TextPoint>,
/// The current selection goes from the selection_origin until the edit_point. Note that the
/// selection_origin may be after the edit_point, in the case of a backward selection.
pub selection_origin: Option<TextPoint>,
/// Is this a multiline input?
multiline: bool,
#[ignore_malloc_size_of = "Can't easily measure this generic type"]
@ -156,7 +157,7 @@ impl<T: ClipboardProvider> TextInput<T> {
let mut i = TextInput {
lines: vec!(),
edit_point: Default::default(),
selection_begin: None,
selection_origin: None,
multiline: lines == Lines::Multiple,
clipboard_provider: clipboard_provider,
max_length: max_length,
@ -169,7 +170,7 @@ impl<T: ClipboardProvider> TextInput<T> {
/// Remove a character at the current editing point
pub fn delete_char(&mut self, dir: Direction) {
if self.selection_begin.is_none() || self.selection_begin == Some(self.edit_point) {
if self.selection_origin.is_none() || self.selection_origin == Some(self.edit_point) {
self.adjust_horizontal_by_one(dir, Selection::Selected);
}
self.replace_selection(DOMString::new());
@ -182,46 +183,84 @@ impl<T: ClipboardProvider> TextInput<T> {
/// Insert a string at the current editing point
pub fn insert_string<S: Into<String>>(&mut self, s: S) {
if self.selection_begin.is_none() {
self.selection_begin = Some(self.edit_point);
if self.selection_origin.is_none() {
self.selection_origin = Some(self.edit_point);
}
self.replace_selection(DOMString::from(s.into()));
}
pub fn get_sorted_selection(&self) -> Option<(TextPoint, TextPoint)> {
self.selection_begin.map(|begin| {
let end = self.edit_point;
if begin.line < end.line || (begin.line == end.line && begin.index < end.index) {
(begin, end)
} else {
(end, begin)
}
})
/// The selection origin, or the edit point if there is no selection. Note that the selection
/// origin may be after the edit point, in the case of a backward selection.
pub fn selection_origin_or_edit_point(&self) -> TextPoint {
self.selection_origin.unwrap_or(self.edit_point)
}
// Check that the selection is valid.
fn assert_ok_selection(&self) {
if let Some(begin) = self.selection_begin {
debug_assert!(begin.line < self.lines.len());
debug_assert!(begin.index <= self.lines[begin.line].len());
/// The start of the selection (or the edit point, if there is no selection). Always less than
/// or equal to selection_end(), regardless of the selection direction.
pub fn selection_start(&self) -> TextPoint {
match self.selection_direction {
SelectionDirection::None | SelectionDirection::Forward => self.selection_origin_or_edit_point(),
SelectionDirection::Backward => self.edit_point,
}
debug_assert!(self.edit_point.line < self.lines.len());
debug_assert!(self.edit_point.index <= self.lines[self.edit_point.line].len());
}
/// The UTF-8 byte offset of the selection_start()
pub fn selection_start_offset(&self) -> usize {
self.text_point_to_offset(&self.selection_start())
}
/// The end of the selection (or the edit point, if there is no selection). Always greater
/// than or equal to selection_start(), regardless of the selection direction.
pub fn selection_end(&self) -> TextPoint {
match self.selection_direction {
SelectionDirection::None | SelectionDirection::Forward => self.edit_point,
SelectionDirection::Backward => self.selection_origin_or_edit_point(),
}
}
/// The UTF-8 byte offset of the selection_end()
pub fn selection_end_offset(&self) -> usize {
self.text_point_to_offset(&self.selection_end())
}
/// Whether or not there is an active selection (the selection may be zero-length)
#[inline]
pub fn has_selection(&self) -> bool {
self.selection_origin.is_some()
}
/// Returns a tuple of (start, end) giving the bounds of the current selection. start is always
/// less than or equal to end.
pub fn sorted_selection_bounds(&self) -> (TextPoint, TextPoint) {
(self.selection_start(), self.selection_end())
}
/// Return the selection range as UTF-8 byte offsets from the start of the content.
///
/// If there is no selection, returns an empty range at the insertion point.
pub fn get_absolute_selection_range(&self) -> Range<usize> {
match self.get_sorted_selection() {
Some((begin, end)) => self.get_absolute_point_for_text_point(&begin) ..
self.get_absolute_point_for_text_point(&end),
None => {
let insertion_point = self.get_absolute_insertion_point();
insertion_point .. insertion_point
/// If there is no selection, returns an empty range at the edit point.
pub fn sorted_selection_offsets_range(&self) -> Range<usize> {
self.selection_start_offset() .. self.selection_end_offset()
}
// Check that the selection is valid.
fn assert_ok_selection(&self) {
if let Some(begin) = self.selection_origin {
debug_assert!(begin.line < self.lines.len());
debug_assert!(begin.index <= self.lines[begin.line].len());
match self.selection_direction {
SelectionDirection::None | SelectionDirection::Forward => {
debug_assert!(begin <= self.edit_point)
},
SelectionDirection::Backward => {
debug_assert!(self.edit_point <= begin)
},
}
}
debug_assert!(self.edit_point.line < self.lines.len());
debug_assert!(self.edit_point.index <= self.lines[self.edit_point.line].len());
}
pub fn get_selection_text(&self) -> Option<String> {
@ -242,26 +281,32 @@ impl<T: ClipboardProvider> TextInput<T> {
///
/// The accumulator `acc` can be mutated by the callback, and will be returned at the end.
fn fold_selection_slices<B, F: FnMut(&mut B, &str)>(&self, mut acc: B, mut f: F) -> B {
match self.get_sorted_selection() {
Some((begin, end)) if begin.line == end.line => {
f(&mut acc, &self.lines[begin.line][begin.index..end.index])
}
Some((begin, end)) => {
f(&mut acc, &self.lines[begin.line][begin.index..]);
for line in &self.lines[begin.line + 1 .. end.line] {
if self.has_selection() {
let (start, end) = self.sorted_selection_bounds();
if start.line == end.line {
f(&mut acc, &self.lines[start.line][start.index..end.index])
} else {
f(&mut acc, &self.lines[start.line][start.index..]);
for line in &self.lines[start.line + 1 .. end.line] {
f(&mut acc, "\n");
f(&mut acc, line);
}
f(&mut acc, "\n");
f(&mut acc, &self.lines[end.line][..end.index])
}
None => {}
}
acc
}
pub fn replace_selection(&mut self, insert: DOMString) {
if let Some((begin, end)) = self.get_sorted_selection() {
if !self.has_selection() {
return
}
let (start, end) = self.sorted_selection_bounds();
let allowed_to_insert_count = if let Some(max_length) = self.max_length {
let len_after_selection_replaced = self.utf16_len() - self.selection_utf16_len();
if len_after_selection_replaced >= max_length {
@ -281,9 +326,9 @@ impl<T: ClipboardProvider> TextInput<T> {
self.clear_selection();
let new_lines = {
let prefix = &self.lines[begin.line][..begin.index];
let prefix = &self.lines[start.line][..start.index];
let suffix = &self.lines[end.line][end.index..];
let lines_prefix = &self.lines[..begin.line];
let lines_prefix = &self.lines[..start.line];
let lines_suffix = &self.lines[end.line + 1..];
let mut insert_lines = if self.multiline {
@ -300,7 +345,7 @@ impl<T: ClipboardProvider> TextInput<T> {
let last_insert_lines_index = insert_lines.len() - 1;
self.edit_point.index = insert_lines[last_insert_lines_index].len();
self.edit_point.line = begin.line + last_insert_lines_index;
self.edit_point.line = start.line + last_insert_lines_index;
// FIXME(ajeffrey): effecient append for DOMStrings
insert_lines[last_insert_lines_index].push_str(suffix);
@ -313,7 +358,6 @@ impl<T: ClipboardProvider> TextInput<T> {
};
self.lines = new_lines;
}
self.assert_ok_selection();
}
@ -330,8 +374,8 @@ impl<T: ClipboardProvider> TextInput<T> {
}
if select == Selection::Selected {
if self.selection_begin.is_none() {
self.selection_begin = Some(self.edit_point);
if self.selection_origin.is_none() {
self.selection_origin = Some(self.edit_point);
}
} else {
self.clear_selection();
@ -398,14 +442,19 @@ impl<T: ClipboardProvider> TextInput<T> {
fn adjust_selection_for_horizontal_change(&mut self, adjust: Direction, select: Selection)
-> bool {
if select == Selection::Selected {
if self.selection_begin.is_none() {
self.selection_begin = Some(self.edit_point);
if self.selection_origin.is_none() {
self.selection_origin = Some(self.edit_point);
}
self.selection_direction = match adjust {
Direction::Backward => SelectionDirection::Backward,
Direction::Forward => SelectionDirection::Forward,
};
} else {
if let Some((begin, end)) = self.get_sorted_selection() {
if self.has_selection() {
self.edit_point = match adjust {
Direction::Backward => begin,
Direction::Forward => end,
Direction::Backward => self.selection_start(),
Direction::Forward => self.selection_end(),
};
self.clear_selection();
return true
@ -451,7 +500,7 @@ impl<T: ClipboardProvider> TextInput<T> {
/// Select all text in the input control.
pub fn select_all(&mut self) {
self.selection_begin = Some(TextPoint {
self.selection_origin = Some(TextPoint {
line: 0,
index: 0,
});
@ -463,7 +512,7 @@ impl<T: ClipboardProvider> TextInput<T> {
/// Remove the current selection.
pub fn clear_selection(&mut self) {
self.selection_begin = None;
self.selection_origin = None;
}
pub fn adjust_horizontal_by_word(&mut self, direction: Direction, select: Selection) {
@ -780,17 +829,12 @@ impl<T: ClipboardProvider> TextInput<T> {
};
self.edit_point.line = min(self.edit_point.line, self.lines.len() - 1);
self.edit_point.index = min(self.edit_point.index, self.current_line_length());
self.selection_begin = None;
self.selection_origin = None;
self.assert_ok_selection();
}
/// Get the insertion point as a byte offset from the start of the content.
pub fn get_absolute_insertion_point(&self) -> usize {
self.get_absolute_point_for_text_point(&self.edit_point)
}
/// Convert a TextPoint into a byte offset from the start of the content.
pub fn get_absolute_point_for_text_point(&self, text_point: &TextPoint) -> usize {
fn text_point_to_offset(&self, text_point: &TextPoint) -> usize {
self.lines.iter().enumerate().fold(0, |acc, (i, val)| {
if i < text_point.line {
acc + val.len() + 1 // +1 for the \n
@ -801,7 +845,7 @@ impl<T: ClipboardProvider> TextInput<T> {
}
/// Convert a byte offset from the start of the content into a TextPoint.
pub fn get_text_point_for_absolute_point(&self, abs_point: usize) -> TextPoint {
fn offset_to_text_point(&self, abs_point: usize) -> TextPoint {
let mut index = abs_point;
let mut line = 0;
@ -842,28 +886,17 @@ impl<T: ClipboardProvider> TextInput<T> {
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);
self.selection_origin = Some(self.offset_to_text_point(start));
self.edit_point = self.offset_to_text_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);
self.selection_origin = Some(self.offset_to_text_point(end));
self.edit_point = self.offset_to_text_point(start);
}
}
self.assert_ok_selection();
}
pub fn get_selection_start(&self) -> u32 {
let selection_start = match self.selection_begin {
Some(selection_begin_point) => {
self.get_absolute_point_for_text_point(&selection_begin_point)
},
None => self.get_absolute_insertion_point()
};
selection_start as u32
}
pub fn set_edit_point_index(&mut self, index: usize) {
let byte_size = self.lines[self.edit_point.line]
.graphemes(true)

41
tests/unit/script/textinput.rs
View file

@ -222,7 +222,7 @@ fn test_textinput_delete_char() {
let mut textinput = text_input(Lines::Single, "abcdefg");
textinput.adjust_horizontal(2, Selection::NotSelected);
// Set an empty selection range.
textinput.selection_begin = Some(textinput.edit_point);
textinput.selection_origin = Some(textinput.edit_point);
textinput.delete_char(Direction::Backward);
assert_eq!(textinput.get_content(), "acdefg");
}
@ -252,15 +252,15 @@ fn test_textinput_get_sorted_selection() {
let mut textinput = text_input(Lines::Single, "abcdefg");
textinput.adjust_horizontal(2, Selection::NotSelected);
textinput.adjust_horizontal(2, Selection::Selected);
let (begin, end) = textinput.get_sorted_selection().unwrap();
assert_eq!(begin.index, 2);
let (start, end) = textinput.sorted_selection_bounds();
assert_eq!(start.index, 2);
assert_eq!(end.index, 4);
textinput.clear_selection();
textinput.adjust_horizontal(-2, Selection::Selected);
let (begin, end) = textinput.get_sorted_selection().unwrap();
assert_eq!(begin.index, 2);
let (start, end) = textinput.sorted_selection_bounds();
assert_eq!(start.index, 2);
assert_eq!(end.index, 4);
}
@ -588,18 +588,18 @@ fn test_textinput_set_selection_with_direction() {
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);
assert!(textinput.selection_origin.is_some());
assert_eq!(textinput.selection_origin.unwrap().line, 0);
assert_eq!(textinput.selection_origin.unwrap().index, 2);
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);
assert!(textinput.selection_origin.is_some());
assert_eq!(textinput.selection_origin.unwrap().line, 0);
assert_eq!(textinput.selection_origin.unwrap().index, 6);
}
#[test]
@ -611,3 +611,22 @@ fn test_textinput_unicode_handling() {
textinput.set_edit_point_index(4);
assert_eq!(textinput.edit_point.index, 8);
}
#[test]
fn test_selection_bounds() {
let mut textinput = text_input(Lines::Single, "abcdef");
textinput.set_selection_range(2, 5, SelectionDirection::Forward);
assert_eq!(TextPoint { line: 0, index: 2 }, textinput.selection_origin_or_edit_point());
assert_eq!(TextPoint { line: 0, index: 2 }, textinput.selection_start());
assert_eq!(TextPoint { line: 0, index: 5 }, textinput.selection_end());
assert_eq!(2, textinput.selection_start_offset());
assert_eq!(5, textinput.selection_end_offset());
textinput.set_selection_range(3, 6, SelectionDirection::Backward);
assert_eq!(TextPoint { line: 0, index: 6 }, textinput.selection_origin_or_edit_point());
assert_eq!(TextPoint { line: 0, index: 3 }, textinput.selection_start());
assert_eq!(TextPoint { line: 0, index: 6 }, textinput.selection_end());
assert_eq!(3, textinput.selection_start_offset());
assert_eq!(6, textinput.selection_end_offset());
}

2
tests/wpt/metadata/MANIFEST.json
View file

@ -553039,7 +553039,7 @@
"testharness"
],
"html/semantics/forms/textfieldselection/selection-start-end.html": [
"3fd1c942f7ac3ed3097bbd1ec89db15fb0805476",
"0fd9c420f831943f0d992076a7828eac066b6596",
"testharness"
],
"html/semantics/forms/textfieldselection/selection-value-interactions.html": [

36
tests/wpt/metadata/html/semantics/forms/textfieldselection/selection-after-content-change.html.ini
View file

@ -1,56 +1,20 @@
[selection-after-content-change.html]
type: testharness
[input out of document: selection must not change when setting the same value]
expected: FAIL
[input out of document: selection must change when setting a different value]
expected: FAIL
[input out of document: selection must not change when setting a value that becomes the same after the value sanitization algorithm]
expected: FAIL
[input in document: selection must not change when setting the same value]
expected: FAIL
[input in document: selection must change when setting a different value]
expected: FAIL
[input in document: selection must not change when setting a value that becomes the same after the value sanitization algorithm]
expected: FAIL
[input in document, with focus: selection must not change when setting the same value]
expected: FAIL
[input in document, with focus: selection must change when setting a different value]
expected: FAIL
[input in document, with focus: selection must not change when setting a value that becomes the same after the value sanitization algorithm]
expected: FAIL
[textarea out of document: selection must not change when setting the same value]
expected: FAIL
[textarea out of document: selection must change when setting a different value]
expected: FAIL
[textarea out of document: selection must not change when setting the same normalized value]
expected: FAIL
[textarea in document: selection must not change when setting the same value]
expected: FAIL
[textarea in document: selection must change when setting a different value]
expected: FAIL
[textarea in document: selection must not change when setting the same normalized value]
expected: FAIL
[textarea in document, with focus: selection must not change when setting the same value]
expected: FAIL
[textarea in document, with focus: selection must change when setting a different value]
expected: FAIL
[textarea in document, with focus: selection must not change when setting the same normalized value]
expected: FAIL

26
tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-start-end.html
View file

@ -143,4 +143,30 @@
el.remove();
}
}, "selectionEnd edge-case values");
test(() => {
for (let el of createTestElements(testValue)) {
const start = 1;
const end = testValue.length - 1;
el.setSelectionRange(start, end);
assert_equals(el.selectionStart, start, `selectionStart on ${el.id}`);
assert_equals(el.selectionEnd, end, `selectionEnd on ${el.id}`);
el.selectionDirection = "backward";
assert_equals(el.selectionStart, start,
`selectionStart on ${el.id} after setting selectionDirection to "backward"`);
assert_equals(el.selectionEnd, end,
`selectionEnd on ${el.id} after setting selectionDirection to "backward"`);
el.selectionDirection = "forward";
assert_equals(el.selectionStart, start,
`selectionStart on ${el.id} after setting selectionDirection to "forward"`);
assert_equals(el.selectionEnd, end,
`selectionEnd on ${el.id} after setting selectionDirection to "forward"`);
}
}, "selectionStart and selectionEnd should remain the same when selectionDirection is changed");
</script>