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: Fix float speculation with percentage inline sizes, rewrite vertical alignment, fix inline block ascent/descent computation, and fix absolute inline-block hypothetical boxes. #10691

Merged
merged 12 commits into from May 4, 2016

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Apr 18, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Apr 18, 2016

warning Warning warning

  • These commits modify gfx and layout code, but no tests are modified. Please consider adding a test!
@pcwalton
Copy link
Contributor Author

pcwalton commented Apr 18, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

Trying commit 872136d with merge f992004...

bors-servo added a commit that referenced this pull request Apr 18, 2016
layout: Various fixes for floats. (NOT READY YET; DO NOT MERGE)
@@ -1528,6 +1528,9 @@ impl BlockFlow {
left_float_width + right_float_width);

self.base.intrinsic_inline_sizes = computation.finish();
/*if self.base.flags.is_float() {
println!("intrinsic inline sizes for float={:?}", self.base.intrinsic_inline_sizes);

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2016

Member

Heh, printf-based debugging is the best kind of debugging ;-)

Jokes aside, great work trying to take all this floats bugs down!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

💔 Test failed - linux-rel

@emilio
Copy link
Member

emilio commented Apr 18, 2016

  ▶ FAIL [expected PASS] /_mozilla/css/white-space-mixed-002.htm
  └   → /_mozilla/css/white-space-mixed-002.htm 094ec397037e69456a51b9f41e839bb07517361b
/_mozilla/css/white-space-mixed-002-ref.htm c11114d62a0df65ec487600581335e11858dc85a
Testing 094ec397037e69456a51b9f41e839bb07517361b == c11114d62a0df65ec487600581335e11858dc85a

Andd... A bunch of failing tests in test-css:

EDIT: Moved them into a gist

EDIT 2: Actually there are a few passes:

  ▶ PASS [expected FAIL] /css21_dev/html4/c414-flt-wrap-001.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/floats-132.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/floats-wrap-top-below-inline-002l.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/floats-wrap-top-below-inline-002r.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/floats-wrap-top-below-inline-003r.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/floats-wrap-top-below-inline-003l.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/inline-replaced-width-012.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/inline-replaced-width-013.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/inline-replaced-width-015.htm
  ▶ PASS [expected FAIL] /css21_dev/html4/c414-flt-wrap-001.htm
@@ -444,7 +444,7 @@ impl LineBreaker {
// Initially, pretend a splittable fragment has zero inline-size. We will move it later if
// it has nonzero inline-size and that causes problems.
let placement_inline_size = if first_fragment.can_split() {
Au(0)
first_fragment.minimum_splittable_inline_size()

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Apr 19, 2016

Contributor

This fixes #6997.

@mbrubeck mbrubeck mentioned this pull request Apr 19, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 20, 2016

@highfive highfive assigned mbrubeck and unassigned Ms2ger Apr 20, 2016
@pcwalton pcwalton force-pushed the pcwalton:float-fixes branch from 55f7b2e to 6d06c6c Apr 21, 2016
@pcwalton
Copy link
Contributor Author

pcwalton commented Apr 21, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

Trying commit 6d06c6c with merge 6e6d0a1...

bors-servo added a commit that referenced this pull request Apr 21, 2016
layout: Various fixes for floats. (NOT READY YET; DO NOT MERGE)

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

bors-servo commented Apr 21, 2016

💔 Test failed - mac-rel-wpt

@pcwalton pcwalton changed the title layout: Various fixes for floats. (NOT READY YET; DO NOT MERGE) layout: Rewrite the vertical alignment code to integrate properly with float placement. (NOT READY YET; DO NOT MERGE) Apr 21, 2016
@pcwalton pcwalton force-pushed the pcwalton:float-fixes branch from 6d06c6c to c909d81 Apr 21, 2016
@pcwalton
Copy link
Contributor Author

pcwalton commented Apr 21, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

Trying commit c909d81 with merge 0f41226...

bors-servo added a commit that referenced this pull request Apr 21, 2016
layout: Rewrite the vertical alignment code to integrate properly with float placement. (NOT READY YET; DO NOT MERGE)

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

bors-servo commented Apr 21, 2016

💔 Test failed - mac-rel-wpt

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2016

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

pcwalton added 9 commits Apr 27, 2016
inline sizes are nonzero.

This is a bit of a hack.
baselines when aligning inline fragments per CSS 2.1 § 10.8.1.
This makes the line breaker determine the final block positions of each
line rather than doing it in a separate pass afterward. Not only does
this simplify the code, it makes `vertical-align` and float placement
interact properly.
`position: absolute` inline per CSS 2.1 § 10.3.7.
@pcwalton pcwalton force-pushed the pcwalton:float-fixes branch from 332ad76 to dacc482 May 4, 2016
@highfive highfive removed the S-tests-failed label May 4, 2016
pcwalton added 3 commits Apr 27, 2016
It depends on the vertical alignment of button text, which is a spec
bug.
@pcwalton
Copy link
Contributor Author

pcwalton commented May 4, 2016

@bors-servo: r=mbrubeck

Just updated a test (the test fails on Gecko without this fix)

@bors-servo
Copy link
Contributor

bors-servo commented May 4, 2016

📌 Commit dacc482 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented May 4, 2016

Testing commit dacc482 with merge 35ba293...

bors-servo added a commit that referenced this pull request May 4, 2016
layout: Fix float speculation with percentage inline sizes, rewrite vertical alignment, fix inline block ascent/descent computation, and fix absolute inline-block hypothetical boxes.

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

bors-servo commented May 4, 2016

@bors-servo bors-servo merged commit dacc482 into servo:master May 4, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pcwalton pcwalton deleted the pcwalton:float-fixes branch May 4, 2016
@paulrouget
Copy link
Contributor

paulrouget commented May 7, 2016

regression: #11063

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

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