Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Merged
merged 3 commits into from Nov 10, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

Don't mark start/end of {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT

  • Loading branch information
Permutatrix committed Nov 3, 2016
commit fec3a49d434572e2bde86291a7451625113a7cfb
@@ -29,7 +29,7 @@ use fragment::{InlineAbsoluteHypotheticalFragmentInfo, TableColumnFragmentInfo};
use fragment::{InlineBlockFragmentInfo, SpecificFragmentInfo, UnscannedTextFragmentInfo};
use fragment::WhitespaceStrippingResult;
use gfx::display_list::OpaqueNode;
use inline::{FIRST_FRAGMENT_OF_ELEMENT, InlineFlow, InlineFragmentNodeFlags};
use inline::{FIRST_FRAGMENT_OF_ELEMENT, InlineFlow};
use inline::{InlineFragmentNodeInfo, LAST_FRAGMENT_OF_ELEMENT};
use linked_list::prepend_from;
use list_item::{ListItemFlow, ListStyleTypeContent};
@@ -159,6 +159,32 @@ pub struct InlineBlockSplit {
pub flow: FlowRef,
}

/// Flushes the given accumulator to the new split and makes a new accumulator to hold any
/// subsequent fragments.
impl InlineBlockSplit {
fn new<ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>(fragment_accumulator: &mut InlineFragmentsAccumulator,
node: &ConcreteThreadSafeLayoutNode,
style_context: &SharedStyleContext,
flow: FlowRef)
-> InlineBlockSplit {
fragment_accumulator.enclosing_node.as_mut().expect(
"enclosing_node is None; Are {ib} splits being generated outside of an inline node?"
).flags.remove(LAST_FRAGMENT_OF_ELEMENT);

let split = InlineBlockSplit {
predecessors: mem::replace(
fragment_accumulator,
InlineFragmentsAccumulator::from_inline_node(
node, style_context)).to_intermediate_inline_fragments(),
flow: flow,
};

fragment_accumulator.enclosing_node.as_mut().unwrap().flags.remove(FIRST_FRAGMENT_OF_ELEMENT);

split
}
}

/// Holds inline fragments and absolute descendants.
#[derive(Clone)]
pub struct IntermediateInlineFragments {
@@ -222,7 +248,7 @@ impl InlineFragmentsAccumulator {
pseudo: node.get_pseudo_element_type().strip(),
style: node.style(style_context),
selected_style: node.selected_style(),
flags: InlineFragmentNodeFlags::empty(),
flags: FIRST_FRAGMENT_OF_ELEMENT | LAST_FRAGMENT_OF_ELEMENT,

This comment has been minimized.

@emilio

emilio Nov 3, 2016

Member

This looks good to me, but I wonder if there's a way to organize this so this doesn't start with this flags. It feels weird to use these flags here since we always remove them in to_intermediate_inline_fragments.

Maybe keeping in the accumulator something like saw_ib_split flag or similar, and don't insert the flags in that case?

This comment has been minimized.

@Permutatrix

Permutatrix Nov 3, 2016

Author Contributor

I think of InlineFragmentsAccumulator's enclosing_node as representing the "whole" InlineFragmentNodeInfo for this accumulator, i.e. what it would be if it was all one fragment. It's split in two each time an {ib} split is encountered, then split up again in to_intermediate_inline_fragments. I'm not convinced that it's conceptually more reasonable to save instructions for how to mark the nodes and follow them as late as possible rather than simply handling it this way as we go.

Practically speaking, Servo will one day have to handle the positioning of backgrounds across multiple line boxes. Information about this will likely be stored in InlineFragmentNodeInfo. Using a conceptual model that allows that information to be reused will make it easier.

Comments would definitely help, though. If we stick with my model, I'll add some explaining it.

This comment has been minimized.

@Permutatrix

Permutatrix Nov 3, 2016

Author Contributor

Actually, I suppose information about positioning backgrounds won't be split at this stage. But, uh, you know... Something else could be. In theory.

This comment has been minimized.

@pcwalton

pcwalton Nov 8, 2016

Contributor

I concur with @Permutatrix; let's make things as simple as possible. But definitely comment this explaining how things work and the rationale.

}),
bidi_control_chars: None,
restyle_damage: node.restyle_damage(),
@@ -245,19 +271,21 @@ impl InlineFragmentsAccumulator {
bidi_control_chars,
restyle_damage,
} = self;
if let Some(enclosing_node) = enclosing_node {
if let Some(mut enclosing_node) = enclosing_node {
let fragment_count = fragments.fragments.len();
for (index, fragment) in fragments.fragments.iter_mut().enumerate() {
let mut enclosing_node = enclosing_node.clone();
if index == 0 {
enclosing_node.flags.insert(FIRST_FRAGMENT_OF_ELEMENT)
if index != 0 {
enclosing_node.flags.remove(FIRST_FRAGMENT_OF_ELEMENT)
}
if index == fragment_count - 1 {
enclosing_node.flags.insert(LAST_FRAGMENT_OF_ELEMENT)
if index != fragment_count - 1 {
enclosing_node.flags.remove(LAST_FRAGMENT_OF_ELEMENT)
}
fragment.add_inline_context_style(enclosing_node);
}

enclosing_node.flags.remove(FIRST_FRAGMENT_OF_ELEMENT | LAST_FRAGMENT_OF_ELEMENT);

// Control characters are later discarded in transform_text, so they don't affect the
// is_first/is_last styles above.
if let Some((start, end)) = bidi_control_chars {
@@ -715,14 +743,8 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
} = split;
fragment_accumulator.push_all(predecessors);

let split = InlineBlockSplit {
predecessors: mem::replace(
fragment_accumulator,
InlineFragmentsAccumulator::from_inline_node(
node, self.style_context())).to_intermediate_inline_fragments(),
flow: kid_flow,
};
opt_inline_block_splits.push_back(split)
opt_inline_block_splits.push_back(
InlineBlockSplit::new(fragment_accumulator, node, self.style_context(), kid_flow));
}
}

@@ -749,17 +771,8 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
ConstructionResult::None => {}
ConstructionResult::Flow(flow, kid_abs_descendants) => {
if !flow::base(&*flow).flags.contains(IS_ABSOLUTELY_POSITIONED) {
// {ib} split. Flush the accumulator to our new split and make a new
// accumulator to hold any subsequent fragments we come across.
let split = InlineBlockSplit {
predecessors:
mem::replace(
&mut fragment_accumulator,
InlineFragmentsAccumulator::from_inline_node(
node, self.style_context())).to_intermediate_inline_fragments(),
flow: flow,
};
opt_inline_block_splits.push_back(split);
opt_inline_block_splits.push_back(InlineBlockSplit::new(
&mut fragment_accumulator, node, self.style_context(), flow));
abs_descendants.push_descendants(kid_abs_descendants);
} else {
// Push the absolutely-positioned kid as an inline containing block.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.