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

layout: Rewrite anonymous table code, simplify and fix table intrinsic width calculation, and improve safety of flexbox code. #13870

Merged
merged 19 commits into from Oct 27, 2016

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Oct 21, 2016

Closes #13782.


This change is Reviewable

@highfive
Copy link

highfive commented Oct 21, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/selector_matching.rs, components/style/animation.rs, components/style/servo_selector_impl.rs, components/style/properties/properties.mako.rs, components/style/matching.rs
  • @KiChjang: components/script_layout_interface/wrapper_traits.rs
  • @fitzgen: components/script_layout_interface/wrapper_traits.rs
  • @emilio: components/style/selector_matching.rs, components/style/animation.rs, components/style/servo_selector_impl.rs, components/layout/multicol.rs, components/layout/list_item.rs, components/style/properties/properties.mako.rs, components/layout/table_caption.rs, components/layout/flex.rs, components/layout/table_cell.rs, components/layout/construct.rs, components/layout/flow.rs, components/layout/fragment.rs, components/layout/table_rowgroup.rs, components/layout/table_wrapper.rs, components/layout/block.rs, components/layout/table_row.rs, components/style/matching.rs, components/layout/flow_list.rs, components/layout/table.rs
@highfive
Copy link

highfive commented Oct 21, 2016

warning Warning warning

  • This pull request modifies the contents of
    tests/wpt/css-tests/, which are overwriten occasionally whenever the
    directory is synced from upstream.
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 21, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

Trying commit 392ab81 with merge f930608...

bors-servo added a commit that referenced this pull request Oct 21, 2016
(DO NOT MERGE) layout: Rewrite table stuff.

Closes #13782.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

💔 Test failed - mac-dev-unit

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

The latest upstream changes (presumably #13848) made this pull request unmergeable. Please resolve the merge conflicts.

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 21, 2016

r? @mbrubeck

All I'd complain about is spec-related :)

@highfive highfive assigned mbrubeck and unassigned Ms2ger Oct 21, 2016
Copy link
Member

emilio left a comment

Just a first pass with a few comments. The style parts look good to me. The layout parts make sense to me, though I'd have to do a lot of spec-reading to ensure it's correct.

@@ -19,7 +19,7 @@
#table
{
border-right: 10px solid blue;
display: inline-table;
display: block;

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

This will need to be upstreamed, or reverted.

</html>

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

I think all the newline changes inside tests/wpt should also be reverted.

fun(PseudoElement::ServoAnonymousTableWrapper);
fun(PseudoElement::ServoAnonymousTable);
fun(PseudoElement::ServoAnonymousTableRow);
fun(PseudoElement::ServoAnonymousTableCell);

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

Probably worth creating a file like gecko's pseudo_element_helper.rs to alleviate this and avoid the duplication (it didn't matter much when they were four pseudos, but...). Also we can specialize a few functions easily. In any case, not your business for this PR, I'll file an issue about it :)

@@ -13,6 +13,7 @@ use selectors::parser::{AttrSelector, ParserContext, SelectorImpl};
use std::fmt;
use string_cache::{Atom, Namespace};

/// NB: If you add to this list, be sure to update `each_pseudo_element` too.

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

I'm pretty sure I wrote this exact comment when I introduced each_pseudo_element, where did it go? Sorry it bited you :/

@@ -252,19 +253,29 @@ impl Stylist {

/// Computes the style for a given "precomputed" pseudo-element, taking the
/// universal rules and applying them.
///
/// If `inherit_all` is true, then all properties are inherited from the parent; otherwise,
/// non-inherited properties are reset to their initial values.

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

A comment saying that this is used for anonymous flows would be helpful IMO.

let table_style = node.style(self.style_context());
let wrapper_style = self.style_context()
.stylist
.precomputed_values_for_pseudo(&PseudoElement::ServoTableWrapper,

This comment has been minimized.

Copy link
@emilio

emilio Oct 21, 2016

Member

This pattern is becoming increasingly common, maybe add a helper for it in SharedStyleContext? Can be left as a followup, but please fill it if you decide to do so.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Oct 24, 2016

Author Contributor

Added a helper to Stylist (as not all the places that call precomputed_values_for_pseudo have a handle to the style context).

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 21, 2016

Some test results that weren't auto-linked above:

mac-rel-css

  ▶ PASS [expected FAIL] /mediaqueries-3_dev/html/min-width-tables-001.htm

linux-rel-css

Tests with unexpected results:
  ▶ PASS [expected FAIL] /css21_dev/html4/color-applies-to-014.htm

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-009.htm
  └   → /css21_dev/html4/table-anonymous-objects-009.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-020.htm
  └   → /css21_dev/html4/table-anonymous-objects-020.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-019.htm
  └   → /css21_dev/html4/table-anonymous-objects-019.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-010.htm
  └   → /css21_dev/html4/table-anonymous-objects-010.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-018.htm
  └   → /css21_dev/html4/table-anonymous-objects-018.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-012.htm
  └   → /css21_dev/html4/table-anonymous-objects-012.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-011.htm
  └   → /css21_dev/html4/table-anonymous-objects-011.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-017.htm
  └   → /css21_dev/html4/table-anonymous-objects-017.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-197.htm
  └   → /css21_dev/html4/table-anonymous-objects-197.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-198.htm
  └   → /css21_dev/html4/table-anonymous-objects-198.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-199.htm
  └   → /css21_dev/html4/table-anonymous-objects-199.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-200.htm
  └   → /css21_dev/html4/table-anonymous-objects-200.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-203.htm
  └   → /css21_dev/html4/table-anonymous-objects-203.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-202.htm
  └   → /css21_dev/html4/table-anonymous-objects-202.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-201.htm
  └   → /css21_dev/html4/table-anonymous-objects-201.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ FAIL [expected PASS] /css21_dev/html4/table-anonymous-objects-204.htm
  └   → /css21_dev/html4/table-anonymous-objects-204.htm 9a467359e61bf1cd03e915a871942253e111a547
/css21_dev/html4/reference/no_red_antialiasing_a_bc_d-ref.htm 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1
Testing 9a467359e61bf1cd03e915a871942253e111a547 == 398e25f2937dcbd574d5e6d4aeb2542b5e1752f1

  ▶ PASS [expected FAIL] /mediaqueries-3_dev/html/min-width-tables-001.htm
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 22, 2016

I've investigated the failures.

  • /css21_dev/html4/color-applies-to-014.htm: As far as I can tell, this is an incorrect test because it fails when browsers perform pixel snapping. Specifically, it tests to make sure that (effectively) <div>Filler Text</div> renders identitally to <table><tr><td>Filler</td><td>&nbsp;Text</td></tr></table> (with all borders/padding/margins/spacing on the tables removed). This can fail in browser engines like Servo that perform pixel snapping on table cells, since the "T" glyph can be subpixel aligned in the former case but not the latter.
  • /css21_dev/html4/table-anonymous-objects-???.htm: These tests rely on summing a set of fractional pixel quantities to overlay the green text "abcd" directly on top of red text "abcd". They then check that the result is precisely equal to the green text "abcd". The display lists come out correct from layout, but WebRender introduces a small amount of rounding error somewhere on Linux (only) and there end up being tiny one-pixel differences around the edges of the letters.

I propose:

  1. Ignoring color-applies-to-014.htm outright, as it's an incorrect test.
  2. Marking table-anonymous-objects-???.htm as failing on Linux, as they trigger minor WebRender bugs on that platform.

Thoughts?

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 22, 2016

As long as you mean "mark as failing" rather than "ignore".

@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 22, 2016

What's the benefit of marking a test we believe to be invalid as failing instead of ignoring it? Its success depends on the details of the font on the platform.

}

(FlowClass::Flex, FlowClass::Block) |
(FlowClass::Flex, FlowClass::Flex) => {

This comment has been minimized.

Copy link
@stshine

stshine Oct 24, 2016

Contributor

According to https://drafts.csswg.org/css-flexbox/#flex-items children of a flex container are blockified, and anonymous text is the only kind of elements that needs to be wrapped in a anonymous block.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Oct 24, 2016

Author Contributor

Flex is OK here because display: flex creates a block-level flex container box.

This comment has been minimized.

Copy link
@stshine

stshine Oct 24, 2016

Contributor

Oh, I mean IMO the wrapper block should only be needed for FlowClass::Inline since other children are guaranteed to be block-like.

parent,
&[],
SpecificFragmentInfo::Generic,
BlockFlow::from_fragment)

This comment has been minimized.

Copy link
@stshine

stshine Oct 24, 2016

Contributor

I wonder if this is still necessary since all situation with a flex container return true in try_to_add_child().

This comment has been minimized.

Copy link
@pcwalton

pcwalton Oct 24, 2016

Author Contributor

It shouldn't be. Will remove.

ServoAnonymousTable,
ServoAnonymousTableRow,
ServoAnonymousTableCell,
ServoAnonymousFlexBlock,

This comment has been minimized.

Copy link
@stshine

stshine Oct 24, 2016

Contributor

Anonymous blocks in flexbox are just general blocks (yes, in servo and gecko inline elements are wrapped in an InlineFlow rather than a anonymous block), thus I suggest to call this ServoAnonymousBlock since it can be used in other places.

@pcwalton pcwalton force-pushed the pcwalton:anonymous-table-rewrite branch from 392ab81 to 2e00b85 Oct 24, 2016
@pcwalton pcwalton force-pushed the pcwalton:anonymous-table-rewrite branch from 2e00b85 to ee3aebd Oct 24, 2016
pcwalton added 12 commits Oct 25, 2016
non-inheritable ones.

This works like the `modify_style_for_*` functions and will allow us to
easily migrate from them to real cascading.
flows' block sizes.

This ensures that we never collapse margins for flex flows.
This patch introduces a "legalizer", which encapsulates all the
anonymous flow generation logic in one place. The legalizer knows how
to, for example, place adjacent blocks with `display: table-cell` in the
same table row.

Improves etsy.com.

Closes #13782.
The base `Flow` implementation returns the incorrect value.
This is more straightforward when anonymous table object generation is
involved.
@pcwalton pcwalton force-pushed the pcwalton:anonymous-table-rewrite branch from 511362f to ba005a9 Oct 26, 2016
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 26, 2016

Both issues should be fixed now.

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 27, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2016

📌 Commit ba005a9 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2016

Testing commit ba005a9 with merge 6b40f97...

bors-servo added a commit that referenced this pull request Oct 27, 2016
layout: Rewrite anonymous table code, simplify and fix table intrinsic width calculation, and improve safety of flexbox code.

Closes #13782.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13870)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2016

@bors-servo bors-servo merged commit ba005a9 into servo:master Oct 27, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Oct 27, 2016
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.