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: Don't use the existing block position as the float ceiling when placing block formatting contexts in the sequential fallback. #13685

Merged

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Oct 11, 2016

The existing block position isn't yet computed at that time, so it
contains junk data. It just so happened to work on first reflow because
that value is usually set to zero, but it usually failed on subsequent
reflows.

Improves certain Wikipedia pages.

Closes #13630 (though Google is still broken; it was a separate bug and
will be split off into a separate issue).

r? @notriddle


This change is Reviewable

placing block formatting contexts in the sequential fallback.

The existing block position isn't yet computed at that time, so it
contains junk data. It just so happened to work on first reflow because
that value is usually set to zero, but it usually failed on subsequent
reflows.

Improves certain Wikipedia pages.

Closes #13630 (though Google is still broken; it was a separate bug and
will be split off into a separate issue).
@highfive
Copy link

highfive commented Oct 11, 2016

warning Warning warning

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

notriddle commented Oct 11, 2016

Now that I look at the code again, what I intended for that member variable to do is probably already being covered by floats.offset. It would've been double-counting the block offset, if it wasn't (almost) always zero.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2016

📌 Commit 5afe12b has been approved by notriddle

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2016

Testing commit 5afe12b with merge 1c54c84...

bors-servo added a commit that referenced this pull request Oct 11, 2016
…cement, r=notriddle

layout: Don't use the existing block position as the float ceiling when placing block formatting contexts in the sequential fallback.

The existing block position isn't yet computed at that time, so it
contains junk data. It just so happened to work on first reflow because
that value is usually set to zero, but it usually failed on subsequent
reflows.

Improves certain Wikipedia pages.

Closes #13630 (though Google is still broken; it was a separate bug and
will be split off into a separate issue).

r? @notriddle

<!-- 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/13685)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2016

💔 Test failed - linux-rel-wpt

@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 11, 2016

@bors-servo: retry

Timeout in DOM APIs, doesn't look related

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2016

Testing commit 5afe12b with merge cad5a4e...

bors-servo added a commit that referenced this pull request Oct 11, 2016
…cement, r=notriddle

layout: Don't use the existing block position as the float ceiling when placing block formatting contexts in the sequential fallback.

The existing block position isn't yet computed at that time, so it
contains junk data. It just so happened to work on first reflow because
that value is usually set to zero, but it usually failed on subsequent
reflows.

Improves certain Wikipedia pages.

Closes #13630 (though Google is still broken; it was a separate bug and
will be split off into a separate issue).

r? @notriddle

<!-- 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/13685)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2016

@bors-servo bors-servo merged commit 5afe12b into servo:master Oct 11, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pcwalton pcwalton deleted the pcwalton:block-formatting-context-fallback-placement branch Oct 11, 2016
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

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