From 587314e52b3a5d74884a77f89d3cf7cae6e79b3d Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 15 Apr 2016 15:35:26 -0700 Subject: [PATCH 1/5] Improve debug logging of non-printable chars --- components/layout/fragment.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index 03638be5fab..6aacfe8b175 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -228,11 +228,11 @@ impl fmt::Debug for SpecificFragmentInfo { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { SpecificFragmentInfo::ScannedText(ref info) => { - write!(f, "\"{}\"", slice_chars(&*info.run.text, info.range.begin().get() as usize, + write!(f, "{:?}", slice_chars(&*info.run.text, info.range.begin().get() as usize, info.range.end().get() as usize)) } SpecificFragmentInfo::UnscannedText(ref info) => { - write!(f, "\"{}\"", info.text) + write!(f, "{:?}", info.text) } _ => Ok(()) } From b607b3a9322dc69e4d5629f27f78830483c31a66 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 15 Apr 2016 15:36:32 -0700 Subject: [PATCH 2/5] Fix meld_with_next_inline_fragment and add meld_with_prev_inline_fragment Factor out a new `meld_with_prev_inline_fragment` method that mirrors the existing `meld_with_next_inline_fragment`. This also fixes a bug in `meld_with_next` that was already fixed in the `meld_with_prev` by @notriddle in #10419. The bug is that it was traversing the inline context nodes in the wrong order. It should start at the outermost enclosing node, since the fragments might be at different nesting levels under some common ancestor. --- components/layout/construct.rs | 23 +------------------- components/layout/fragment.rs | 39 +++++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 4099e1f9af5..8bf1a416484 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -1779,28 +1779,7 @@ pub fn strip_ignorable_whitespace_from_start(this: &mut LinkedList) { WhitespaceStrippingResult::FragmentContainedOnlyWhitespace => { let removed_fragment = this.pop_front().unwrap(); if let Some(ref mut remaining_fragment) = this.front_mut() { - if let Some(ref mut inline_context_of_remaining_fragment) = - remaining_fragment.inline_context { - if let Some(ref inline_context_of_removed_fragment) = - removed_fragment.inline_context { - for (inline_context_node_from_removed_fragment, - inline_context_node_from_remaining_fragment) in - inline_context_of_removed_fragment.nodes.iter().rev().zip( - inline_context_of_remaining_fragment.nodes.iter_mut().rev() - ) { - if !inline_context_node_from_removed_fragment.flags.contains( - FIRST_FRAGMENT_OF_ELEMENT) { - continue - } - if inline_context_node_from_removed_fragment.address != - inline_context_node_from_remaining_fragment.address { - continue - } - inline_context_node_from_remaining_fragment.flags.insert( - FIRST_FRAGMENT_OF_ELEMENT); - } - } - } + remaining_fragment.meld_with_prev_inline_fragment(&removed_fragment); } } } diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index 6aacfe8b175..8f2d5b83af1 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -2497,26 +2497,49 @@ impl Fragment { pub fn meld_with_next_inline_fragment(&mut self, next_fragment: &Fragment) { if let Some(ref mut inline_context_of_this_fragment) = self.inline_context { if let Some(ref inline_context_of_next_fragment) = next_fragment.inline_context { - for (i, inline_context_node_from_next_fragment) in - inline_context_of_next_fragment.nodes.iter().enumerate() { - if i >= inline_context_of_this_fragment.nodes.len() { - continue - } + for (inline_context_node_from_this_fragment, + inline_context_node_from_next_fragment) + in inline_context_of_this_fragment.nodes.iter_mut().rev() + .zip(inline_context_of_next_fragment.nodes.iter().rev()) + { if !inline_context_node_from_next_fragment.flags.contains( LAST_FRAGMENT_OF_ELEMENT) { continue } if inline_context_node_from_next_fragment.address != - inline_context_of_this_fragment.nodes[i].address { + inline_context_node_from_this_fragment.address { continue } - inline_context_of_this_fragment.nodes[i].flags.insert( - LAST_FRAGMENT_OF_ELEMENT); + inline_context_node_from_this_fragment.flags.insert(LAST_FRAGMENT_OF_ELEMENT); } } } } + pub fn meld_with_prev_inline_fragment(&mut self, prev_fragment: &Fragment) { + if let Some(ref mut inline_context_of_this_fragment) = self.inline_context { + if let Some(ref inline_context_of_prev_fragment) = prev_fragment.inline_context { + for (inline_context_node_from_prev_fragment, + inline_context_node_from_this_fragment) + in inline_context_of_prev_fragment.nodes.iter().rev().zip( + inline_context_of_this_fragment.nodes.iter_mut().rev()) + { + if !inline_context_node_from_prev_fragment.flags.contains( + FIRST_FRAGMENT_OF_ELEMENT) { + continue + } + if inline_context_node_from_prev_fragment.address != + inline_context_node_from_this_fragment.address { + continue + } + inline_context_node_from_this_fragment.flags.insert( + FIRST_FRAGMENT_OF_ELEMENT); + } + } + } + } + + pub fn fragment_id(&self) -> usize { return self as *const Fragment as usize; } From fbef2724bf12c2204f0256cebaa5a4d0e8ef45f2 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 15 Apr 2016 15:44:44 -0700 Subject: [PATCH 3/5] Fix up inline context flags during text fragment scanning This fixes two problems that could cause scanned text fragments to end up with incorrect LAST_FRAGMENT_OF_ELEMENT or FIRST_FRAGMENT_OF_ELEMENT flags: 1. If a single unscanned fragment was split into multiple scanned fragments, then all of them would inherit its flags. We need to clear these flags, except for the first and last scanned fragment. 2. When an unscanned fragment generated zero scanned fragments, we correctly called `meld_with_next_inline_fragment` to transfer LAST_FRAGMENT flags to the preceding fragment, but we didn't do anything to transfer FIRST_FRAGMENT flags to the following fragment. We can fix this by calling `meld_with_prev_inline_fragment` on the following fragment. --- components/layout/text.rs | 40 +++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/components/layout/text.rs b/components/layout/text.rs index 41dbb8682f0..da89ae495b8 100644 --- a/components/layout/text.rs +++ b/components/layout/text.rs @@ -15,7 +15,7 @@ use gfx::font_context::FontContext; use gfx::text::glyph::CharIndex; use gfx::text::text_run::TextRun; use gfx::text::util::{self, CompressionMode}; -use inline::InlineFragments; +use inline::{FIRST_FRAGMENT_OF_ELEMENT, InlineFragments, LAST_FRAGMENT_OF_ELEMENT}; use range::{Range, RangeIndex}; use std::borrow::ToOwned; use std::collections::LinkedList; @@ -321,19 +321,26 @@ impl TextRunScanner { // Make new fragments with the runs and adjusted text indices. debug!("TextRunScanner: pushing {} fragment(s)", self.clump.len()); let mut mappings = mappings.into_iter().peekable(); + let mut prev_fragments_to_meld = Vec::new(); + for (logical_offset, old_fragment) in mem::replace(&mut self.clump, LinkedList::new()).into_iter().enumerate() { - loop { + let mut is_first_mapping_of_this_old_fragment = true; + loop { match mappings.peek() { Some(mapping) if mapping.old_fragment_index == logical_offset => {} Some(_) | None => { - if let Some(ref mut last_fragment) = out_fragments.last_mut() { - last_fragment.meld_with_next_inline_fragment(&old_fragment); + if is_first_mapping_of_this_old_fragment { + // There were no mappings for this unscanned fragment. Transfer its + // flags to the previous/next sibling elements instead. + if let Some(ref mut last_fragment) = out_fragments.last_mut() { + last_fragment.meld_with_next_inline_fragment(&old_fragment); + } + prev_fragments_to_meld.push(old_fragment); } break; } }; - let mut mapping = mappings.next().unwrap(); let scanned_run = runs[mapping.text_run_index].clone(); @@ -372,10 +379,31 @@ impl TextRunScanner { let bounding_box_size = bounding_box_for_run_metrics(&new_metrics, writing_mode); new_text_fragment_info.content_size = bounding_box_size; - let new_fragment = old_fragment.transform( + let mut new_fragment = old_fragment.transform( bounding_box_size, SpecificFragmentInfo::ScannedText(new_text_fragment_info)); + let is_last_mapping_of_this_old_fragment = match mappings.peek() { + Some(mapping) if mapping.old_fragment_index == logical_offset => false, + _ => true + }; + + if let Some(ref mut context) = new_fragment.inline_context { + for node in &mut context.nodes { + if !is_last_mapping_of_this_old_fragment { + node.flags.remove(LAST_FRAGMENT_OF_ELEMENT); + } + if !is_first_mapping_of_this_old_fragment { + node.flags.remove(FIRST_FRAGMENT_OF_ELEMENT); + } + } + } + + for prev_fragment in prev_fragments_to_meld.drain(..) { + new_fragment.meld_with_prev_inline_fragment(&prev_fragment); + } + + is_first_mapping_of_this_old_fragment = false; out_fragments.push(new_fragment) } } From 1807fd3cc5741e410b7eae4eb4c2f389c594b144 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 15 Apr 2016 15:57:24 -0700 Subject: [PATCH 4/5] Generate a fragment for an empty elements with borders or padding --- components/layout/construct.rs | 43 ++++++++++++++++++++++++++++++-- components/layout/incremental.rs | 14 +++++++++-- components/style/values.rs | 13 ++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 8bf1a416484..dd8b7ec0756 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -13,6 +13,7 @@ #![deny(unsafe_code)] +use app_units::Au; use block::BlockFlow; use context::LayoutContext; use data::{HAS_NEWLY_CONSTRUCTED_FLOW, PrivateLayoutData}; @@ -808,7 +809,9 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode> let mut abs_descendants = AbsoluteDescendants::new(); // Concatenate all the fragments of our kids, creating {ib} splits as necessary. + let mut is_empty = true; for kid in node.children() { + is_empty = false; if kid.get_pseudo_element_type() != PseudoElementType::Normal { self.process(&kid); } @@ -889,6 +892,22 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode> } } + if is_empty && node.style().has_padding_or_border() { + // An empty inline box needs at least one fragment to draw its background and borders. + let info = SpecificFragmentInfo::UnscannedText( + box UnscannedTextFragmentInfo::new(String::new(), None)); + let mut modified_style = node.style().clone(); + properties::modify_style_for_replaced_content(&mut modified_style); + properties::modify_style_for_text(&mut modified_style); + let fragment = Fragment::from_opaque_node_and_style(node.opaque(), + node.get_pseudo_element_type().strip(), + modified_style, + node.selected_style().clone(), + node.restyle_damage(), + info); + fragment_accumulator.fragments.fragments.push_back(fragment) + } + // Finally, make a new construction result. if opt_inline_block_splits.len() > 0 || !fragment_accumulator.fragments.is_empty() || abs_descendants.len() > 0 { @@ -923,8 +942,6 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode> } // If this node is ignorable whitespace, bail out now. - // - // FIXME(#2001, pcwalton): Don't do this if there's padding or borders. if node.is_ignorable_whitespace() { return ConstructionResult::ConstructionItem(ConstructionItem::Whitespace( node.opaque(), @@ -1852,3 +1869,25 @@ fn control_chars_to_fragment(node: &InlineFragmentNodeInfo, restyle_damage, info) } + +/// Convenience methods for computed CSS values +trait ComputedValueUtils { + /// Returns true if this node has non-zero padding or border. + fn has_padding_or_border(&self) -> bool; +} + +impl ComputedValueUtils for ServoComputedValues { + fn has_padding_or_border(&self) -> bool { + let padding = self.get_padding(); + let border = self.get_border(); + + !padding.padding_top.is_definitely_zero() || + !padding.padding_right.is_definitely_zero() || + !padding.padding_bottom.is_definitely_zero() || + !padding.padding_left.is_definitely_zero() || + border.border_top_width != Au(0) || + border.border_right_width != Au(0) || + border.border_bottom_width != Au(0) || + border.border_left_width != Au(0) + } +} diff --git a/components/layout/incremental.rs b/components/layout/incremental.rs index cc34dc49bcd..affb4f42c70 100644 --- a/components/layout/incremental.rs +++ b/components/layout/incremental.rs @@ -5,7 +5,7 @@ use flow::{self, AFFECTS_COUNTERS, Flow, HAS_COUNTER_AFFECTING_CHILDREN, IS_ABSOLUTELY_POSITIONED}; use std::fmt; use std::sync::Arc; -use style::computed_values::float; +use style::computed_values::{display, float}; use style::dom::TRestyleDamage; use style::properties::{ComputedValues, ServoComputedValues}; @@ -193,7 +193,17 @@ pub fn compute_damage(old: Option<&Arc>, new: &ServoCompute get_text.text_decoration, get_text.unicode_bidi, get_inheritedtable.empty_cells, get_inheritedtable.caption_side, get_column.column_width, get_column.column_count - ]) || add_if_not_equal!(old, new, damage, + ]) || (new.get_box().display == display::T::inline && + add_if_not_equal!(old, new, damage, + [REPAINT, STORE_OVERFLOW, BUBBLE_ISIZES, REFLOW_OUT_OF_FLOW, REFLOW, + RECONSTRUCT_FLOW], [ + // For inline boxes only, border/padding styles are used in flow construction (to decide + // whether to create fragments for empty flows). + get_border.border_top_width, get_border.border_right_width, + get_border.border_bottom_width, get_border.border_left_width, + get_padding.padding_top, get_padding.padding_right, + get_padding.padding_bottom, get_padding.padding_left + ])) || add_if_not_equal!(old, new, damage, [ REPAINT, STORE_OVERFLOW, BUBBLE_ISIZES, REFLOW_OUT_OF_FLOW, REFLOW ], [get_border.border_top_width, get_border.border_right_width, get_border.border_bottom_width, get_border.border_left_width, diff --git a/components/style/values.rs b/components/style/values.rs index 754125bfc21..79239788704 100644 --- a/components/style/values.rs +++ b/components/style/values.rs @@ -1662,9 +1662,22 @@ pub mod computed { } impl LengthOrPercentage { + #[inline] pub fn zero() -> LengthOrPercentage { LengthOrPercentage::Length(Au(0)) } + + /// Returns true if the computed value is absolute 0 or 0%. + /// + /// (Returns false for calc() values, even if ones that may resolve to zero.) + #[inline] + pub fn is_definitely_zero(&self) -> bool { + use self::LengthOrPercentage::*; + match *self { + Length(Au(0)) | Percentage(0.0) => true, + Length(_) | Percentage(_) | Calc(_) => false + } + } } impl fmt::Debug for LengthOrPercentage { From 1695d14a9e248edb3f51856d25a35a96861e251e Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Fri, 15 Apr 2016 16:00:07 -0700 Subject: [PATCH 5/5] Don't strip out all empty fragments Empty fragments may need to be layed out to draw borders, padding/background, and insertion points. (Fragments that consist of discardable whitespace and control characters, on the other hand, can still be discarded.) This ends up preserving some useless empty fragments. It's possible we could avoid this by storing some sort of flag on "important" empty fragments, so we can discard the rest. --- components/layout/construct.rs | 1 + components/layout/text.rs | 15 +++++++-------- .../metadata-css/css21_dev/html4/bidi-011.htm.ini | 3 --- 3 files changed, 8 insertions(+), 11 deletions(-) delete mode 100644 tests/wpt/metadata-css/css21_dev/html4/bidi-011.htm.ini diff --git a/components/layout/construct.rs b/components/layout/construct.rs index dd8b7ec0756..ee961942300 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -1861,6 +1861,7 @@ fn control_chars_to_fragment(node: &InlineFragmentNodeInfo, let info = SpecificFragmentInfo::UnscannedText( box UnscannedTextFragmentInfo::new(String::from(text), None)); let mut style = node.style.clone(); + properties::modify_style_for_replaced_content(&mut style); properties::modify_style_for_text(&mut style); Fragment::from_opaque_node_and_style(node.address, node.pseudo, diff --git a/components/layout/text.rs b/components/layout/text.rs index da89ae495b8..22fbeb9ad1a 100644 --- a/components/layout/text.rs +++ b/components/layout/text.rs @@ -173,6 +173,7 @@ impl TextRunScanner { let mut insertion_point = None; for (fragment_index, in_fragment) in self.clump.iter().enumerate() { + debug!(" flushing {:?}", in_fragment); let mut mapping = RunMapping::new(&run_info_list[..], &run_info, fragment_index); let text; let selection; @@ -239,7 +240,6 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, - insertion_point, compression, text_transform, &mut last_whitespace, @@ -269,7 +269,6 @@ impl TextRunScanner { mapping.flush(&mut mappings, &mut run_info, &**text, - insertion_point, compression, text_transform, &mut last_whitespace, @@ -606,15 +605,12 @@ 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 && insertion_point.is_none() { - return; - } + let was_empty = *start_position == end_position; let old_byte_length = run_info.text.len(); *last_whitespace = util::transform_text(&text[(*start_position)..end_position], compression, @@ -633,8 +629,11 @@ impl RunMapping { run_info.character_length = run_info.character_length + character_count; *start_position = end_position; - // Don't flush mappings that contain no characters and no insertion_point. - if character_count == 0 && !self.contains_insertion_point(insertion_point) { + // Don't save mappings that contain only discarded characters. + // (But keep ones that contained no characters to begin with, since they might have been + // generated by an empty flow to draw its borders/padding/insertion point.) + let is_empty = character_count == 0; + if is_empty && !was_empty { return; } diff --git a/tests/wpt/metadata-css/css21_dev/html4/bidi-011.htm.ini b/tests/wpt/metadata-css/css21_dev/html4/bidi-011.htm.ini deleted file mode 100644 index 01a83ba5f16..00000000000 --- a/tests/wpt/metadata-css/css21_dev/html4/bidi-011.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[bidi-011.htm] - type: reftest - expected: FAIL