From bcaa62e1bd1c5b516370d85ae194f8a1bbd61726 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 1 Apr 2016 15:58:00 -0700 Subject: [PATCH 1/4] Slight refactoring of RunMapping flush method --- components/layout/text.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/components/layout/text.rs b/components/layout/text.rs index 0fb9e7cde05..7c311e05af1 100644 --- a/components/layout/text.rs +++ b/components/layout/text.rs @@ -230,17 +230,15 @@ impl TextRunScanner { let flush_mapping = flush_run || mapping.selected != selected; if flush_mapping { - if end_position > start_position { - mapping.flush(&mut mappings, - &mut run_info, - &**text, - insertion_point, - compression, - text_transform, - &mut last_whitespace, - &mut start_position, - end_position); - } + mapping.flush(&mut mappings, + &mut run_info, + &**text, + insertion_point, + compression, + text_transform, + &mut last_whitespace, + &mut start_position, + end_position); if run_info.text.len() > 0 { if flush_run { run_info_list.push(run_info); @@ -261,11 +259,6 @@ impl TextRunScanner { *paragraph_bytes_processed += character.len_utf8(); } - // If the mapping is zero-length, don't flush it. - if start_position == end_position { - continue - } - // Flush the last mapping we created for this fragment to the list. mapping.flush(&mut mappings, &mut run_info, @@ -557,6 +550,9 @@ impl RunMapping { last_whitespace: &mut bool, start_position: &mut usize, end_position: usize) { + if *start_position == end_position { + return; + } let old_byte_length = run_info.text.len(); *last_whitespace = util::transform_text(&text[(*start_position)..end_position], compression, From 831243af7c09fa0e878b3b29d84dae9224cfd924 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 1 Apr 2016 16:56:35 -0700 Subject: [PATCH 2/4] Fix insertion point layout for text runs with multiple fragments Before this fix, if a TextRun containing an insertion point was split into multiple ScannedText fragments, text layout would draw an insertion point inside of each of the fragments. This patch records the insertion point position at most once per TextRun, and copies it only into the appropriate ScannedText fragment. --- components/layout/text.rs | 64 ++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/components/layout/text.rs b/components/layout/text.rs index 7c311e05af1..731891e68b3 100644 --- a/components/layout/text.rs +++ b/components/layout/text.rs @@ -170,6 +170,8 @@ impl TextRunScanner { // First, transform/compress text of all the nodes. let (mut run_info_list, mut run_info) = (Vec::new(), RunInfo::new()); + let mut insertion_point = None; + for (fragment_index, in_fragment) in self.clump.iter().enumerate() { let mut mapping = RunMapping::new(&run_info_list[..], &run_info, fragment_index); let text; @@ -181,8 +183,12 @@ impl TextRunScanner { } _ => panic!("Expected an unscanned text fragment!"), }; - let insertion_point = match selection { - Some(range) if range.is_empty() => Some(range.begin()), + insertion_point = match selection { + Some(range) if range.is_empty() => { + // `range` is the range within the current fragment. To get the range + // within the text run, offset it by the length of the preceding fragments. + Some(range.begin() + CharIndex(run_info.character_length as isize)) + } _ => None }; @@ -233,7 +239,6 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, - insertion_point, compression, text_transform, &mut last_whitespace, @@ -241,7 +246,7 @@ impl TextRunScanner { end_position); if run_info.text.len() > 0 { if flush_run { - run_info_list.push(run_info); + run_info.flush(&mut run_info_list, &mut insertion_point); run_info = RunInfo::new(); } mapping = RunMapping::new(&run_info_list[..], @@ -263,7 +268,6 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, - insertion_point, compression, text_transform, &mut last_whitespace, @@ -272,7 +276,7 @@ impl TextRunScanner { } // Push the final run info. - run_info_list.push(run_info); + run_info.flush(&mut run_info_list, &mut insertion_point); // Per CSS 2.1 ยง 16.4, "when the resultant space between two characters is not the same // as the default space, user agents should not use ligatures." This ensures that, for @@ -346,11 +350,18 @@ impl TextRunScanner { if requires_line_break_afterward_if_wrapping_on_newlines { flags.insert(REQUIRES_LINE_BREAK_AFTERWARD_IF_WRAPPING_ON_NEWLINES); } + + let insertion_point = if mapping.contains_insertion_point(scanned_run.insertion_point) { + scanned_run.insertion_point + } else { + None + }; + let mut new_text_fragment_info = box ScannedTextFragmentInfo::new( scanned_run.run, mapping.char_range, text_size, - scanned_run.insertion_point, + insertion_point, flags); let new_metrics = new_text_fragment_info.run.metrics_for_range(&mapping.char_range); @@ -504,6 +515,26 @@ impl RunInfo { script: Script::Common, } } + + /// Finish processing this RunInfo and add it to the "done" list. + /// + /// * `insertion_point`: The position of the insertion point, in characters relative to the start + /// of this text run. + fn flush(mut self, + list: &mut Vec, + insertion_point: &mut Option) { + if let Some(idx) = *insertion_point { + let char_len = CharIndex(self.character_length as isize); + if idx <= char_len { + // The insertion point is in this text run. + self.insertion_point = insertion_point.take() + } else { + // Continue looking for the insertion point in the next text run. + *insertion_point = Some(idx - char_len) + } + } + list.push(self); + } } /// A mapping from a portion of an unscanned text fragment to the text run we're going to create @@ -544,7 +575,6 @@ impl RunMapping { mappings: &mut Vec, run_info: &mut RunInfo, text: &str, - insertion_point: Option, compression: CompressionMode, text_transform: text_transform::T, last_whitespace: &mut bool, @@ -568,12 +598,6 @@ impl RunMapping { *last_whitespace, is_first_run); - // Record the position of the insertion point if necessary. - if let Some(insertion_point) = insertion_point { - run_info.insertion_point = - Some(CharIndex(run_info.character_length as isize + insertion_point.0)) - } - run_info.character_length = run_info.character_length + character_count; *start_position = end_position; @@ -587,6 +611,18 @@ impl RunMapping { self.char_range.extend_by(CharIndex(character_count as isize)); mappings.push(self) } + + /// Is the insertion point for this text run within this mapping? + /// + /// NOTE: We treat the range as inclusive at both ends, since the insertion point can lie + /// before the first character *or* after the last character, and should be drawn even if the + /// text is empty. + fn contains_insertion_point(&self, insertion_point: Option) -> bool { + match insertion_point { + None => false, + Some(idx) => self.char_range.begin() <= idx && idx <= self.char_range.end() + } + } } From 32aad0838e63728f7dbe485d5b8858d4991a1dde Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 1 Apr 2016 20:18:28 -0700 Subject: [PATCH 3/4] Draw insertion point even for empty input fields This allows text layout to generate an empty text fragment if the fragment contains the insertion point for a text input box. --- components/layout/display_list_builder.rs | 19 ++++++++++++------- components/layout/fragment.rs | 3 +++ components/layout/text.rs | 12 ++++++++---- .../mozilla/tests/css/focus_selector_ref.html | 3 +++ 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/components/layout/display_list_builder.rs b/components/layout/display_list_builder.rs index 0c04c839450..8832eac31bc 100644 --- a/components/layout/display_list_builder.rs +++ b/components/layout/display_list_builder.rs @@ -1014,13 +1014,9 @@ impl FragmentDisplayListBuilding for Fragment { // display list items. let mut clip = (*clip).clone(); self.adjust_clip_for_style(&mut clip, &stacking_relative_border_box); - if !clip.might_intersect_rect(&stacking_relative_border_box) { - return; - } + let empty_rect = !clip.might_intersect_rect(&stacking_relative_border_box); - debug!("Fragment::build_display_list: intersected. Adding display item..."); - - if self.is_primary_fragment() { + if self.is_primary_fragment() && !empty_rect { // Add shadows, background, borders, and outlines, if applicable. if let Some(ref inline_context) = self.inline_context { for node in inline_context.nodes.iter().rev() { @@ -1080,14 +1076,23 @@ impl FragmentDisplayListBuilding for Fragment { &stacking_relative_border_box, &clip); } + } - // Paint the selection point if necessary. + if self.is_primary_fragment() { + // Paint the selection point if necessary. Even an empty text fragment may have an + // insertion point, so we do this even if `empty_rect` is true. self.build_display_items_for_selection_if_necessary(state, &stacking_relative_border_box, display_list_section, &clip); } + if empty_rect { + return; + } + + debug!("Fragment::build_display_list: intersected. Adding display item..."); + // Create special per-fragment-type display items. self.build_fragment_type_specific_display_items(state, &stacking_relative_border_box, diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index bb761603f2f..42fe28d29e9 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -1709,6 +1709,9 @@ impl Fragment { if other_info.requires_line_break_afterward_if_wrapping_on_newlines() { this_info.flags.insert(REQUIRES_LINE_BREAK_AFTERWARD_IF_WRAPPING_ON_NEWLINES); } + if other_info.insertion_point.is_some() { + this_info.insertion_point = other_info.insertion_point; + } self.border_padding.inline_end = next_fragment.border_padding.inline_end; } _ => panic!("Can only merge two scanned-text fragments!"), diff --git a/components/layout/text.rs b/components/layout/text.rs index 731891e68b3..99fd9ca8a4f 100644 --- a/components/layout/text.rs +++ b/components/layout/text.rs @@ -239,6 +239,7 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, + insertion_point, compression, text_transform, &mut last_whitespace, @@ -268,6 +269,7 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, + insertion_point, compression, text_transform, &mut last_whitespace, @@ -336,6 +338,7 @@ impl TextRunScanner { let scanned_run = runs[mapping.text_run_index].clone(); let requires_line_break_afterward_if_wrapping_on_newlines = + !mapping.byte_range.is_empty() && scanned_run.run.text.char_at_reverse(mapping.byte_range.end()) == '\n'; if requires_line_break_afterward_if_wrapping_on_newlines { mapping.char_range.extend_by(CharIndex(-1)); @@ -575,12 +578,13 @@ impl RunMapping { mappings: &mut Vec, run_info: &mut RunInfo, text: &str, + insertion_point: Option, compression: CompressionMode, text_transform: text_transform::T, last_whitespace: &mut bool, start_position: &mut usize, end_position: usize) { - if *start_position == end_position { + if *start_position == end_position && insertion_point.is_none() { return; } let old_byte_length = run_info.text.len(); @@ -601,9 +605,9 @@ impl RunMapping { run_info.character_length = run_info.character_length + character_count; *start_position = end_position; - // Don't flush empty mappings. - if character_count == 0 { - return + // Don't flush mappings that contain no characters and no insertion_point. + if character_count == 0 && !self.contains_insertion_point(insertion_point) { + return; } let new_byte_length = run_info.text.len(); diff --git a/tests/wpt/mozilla/tests/css/focus_selector_ref.html b/tests/wpt/mozilla/tests/css/focus_selector_ref.html index 8f91987a976..6ea1f341313 100644 --- a/tests/wpt/mozilla/tests/css/focus_selector_ref.html +++ b/tests/wpt/mozilla/tests/css/focus_selector_ref.html @@ -11,5 +11,8 @@ + From e72ae2c25a3268895c6899c74ab4fbcbc50cb714 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 1 Apr 2016 20:34:17 -0700 Subject: [PATCH 4/4] Basic insertion point reftest --- tests/wpt/mozilla/meta/MANIFEST.json | 24 +++++++++++++++++++ .../css/input_insertion_point_empty_a.html | 14 +++++++++++ .../css/input_insertion_point_empty_ref.html | 10 ++++++++ 3 files changed, 48 insertions(+) create mode 100644 tests/wpt/mozilla/tests/css/input_insertion_point_empty_a.html create mode 100644 tests/wpt/mozilla/tests/css/input_insertion_point_empty_ref.html diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 7d7df0a75ca..6f2ed90482c 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -2579,6 +2579,18 @@ "url": "/_mozilla/css/input_height_a.html" } ], + "css/input_insertion_point_empty_a.html": [ + { + "path": "css/input_insertion_point_empty_a.html", + "references": [ + [ + "/_mozilla/css/input_insertion_point_empty_ref.html", + "!=" + ] + ], + "url": "/_mozilla/css/input_insertion_point_empty_a.html" + } + ], "css/input_placeholder.html": [ { "path": "css/input_placeholder.html", @@ -9049,6 +9061,18 @@ "url": "/_mozilla/css/input_height_a.html" } ], + "css/input_insertion_point_empty_a.html": [ + { + "path": "css/input_insertion_point_empty_a.html", + "references": [ + [ + "/_mozilla/css/input_insertion_point_empty_ref.html", + "!=" + ] + ], + "url": "/_mozilla/css/input_insertion_point_empty_a.html" + } + ], "css/input_placeholder.html": [ { "path": "css/input_placeholder.html", diff --git a/tests/wpt/mozilla/tests/css/input_insertion_point_empty_a.html b/tests/wpt/mozilla/tests/css/input_insertion_point_empty_a.html new file mode 100644 index 00000000000..b3478b0b269 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/input_insertion_point_empty_a.html @@ -0,0 +1,14 @@ + + + + + empty input insertion point test + + + + + + + diff --git a/tests/wpt/mozilla/tests/css/input_insertion_point_empty_ref.html b/tests/wpt/mozilla/tests/css/input_insertion_point_empty_ref.html new file mode 100644 index 00000000000..6c393c74845 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/input_insertion_point_empty_ref.html @@ -0,0 +1,10 @@ + + + + + empty input insertion point reference + + + + +