Skip to content

/ servo

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: Remove some bogus code that tried to handle absolutely-positioned flows separately when storing overflow. #9405

Merged
merged 1 commit into from Jan 25, 2016
Merged

layout: Remove some bogus code that tried to handle absolutely-positioned flows separately when storing overflow. #9405

merged 1 commit into from Jan 25, 2016

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Jan 23, 2016

This code dates back to the time when absolutely positioned flows were
ignored by all of their ancestors up to the containing block. This
hasn't been true for at least a year.

Closes #9306.
Closes #9309.
Is a partial fix for #9308.

r? @glennw

Review on Reviewable
@highfive
Copy link

highfive commented Jan 23, 2016

warning Warning warning

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

glennw commented Jan 23, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2016

📌 Commit d81d1df has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2016

⌛ Testing commit d81d1df with merge 3061d50...
bors-servo added a commit that referenced this pull request Jan 23, 2016
@bors-servo
Auto merge of #9405 - pcwalton:absolute-positioning-overflow, r=glennw
3061d50 
layout: Remove some bogus code that tried to handle absolutely-positioned flows separately when storing overflow.

This code dates back to the time when absolutely positioned flows were
ignored by all of their ancestors up to the containing block. This
hasn't been true for at least a year.

Closes #9306.
Closes #9309.
Is a partial fix for #9308.

r? @glennw

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9405)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Jan 23, 2016 

Ran 9335 tests finished in 334.0 seconds.
  • 9334 ran as expected. 55 tests skipped.
  • 1 tests passed unexpectedly

Tests with unexpected results:
  ▶ PASS [expected FAIL] /css21_dev/html4/numbers-units-018.htm

Intermittent pass or real test pass?
The test-wpt failure is #8157.
@nox
Copy link
Member

nox commented Jan 23, 2016

@KiChjang Looks like a proper pass, there are some absolutely-positioned elements in that test.
@jdm
Copy link
Member

jdm commented Jan 23, 2016

@pcwalton Just to confirm, does the presence of the tests under the html/ directory imply that these changes to can only be verified manually?
@glennw
Copy link
Member

glennw commented Jan 23, 2016

@jdm I had assumed so, since the fix changes the bounding rectangle of the scrolling region. But perhaps that does interact with layout / paint in another way that can be tested @pcwalton ?
@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 23, 2016

Yeah, I can't think of a good way to test this automatically.
@pcwalton pcwalton force-pushed the pcwalton:absolute-positioning-overflow branch from d81d1df to 9c7b9fa Jan 25, 2016
@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 25, 2016

@bors-servo: r=glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2016

📌 Commit 9c7b9fa has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2016

⌛ Testing commit 9c7b9fa with merge 4755cc5...
bors-servo added a commit that referenced this pull request Jan 25, 2016
@bors-servo
Auto merge of #9405 - pcwalton:absolute-positioning-overflow, r=glennw
Loading status checks…
4755cc5 
layout: Remove some bogus code that tried to handle absolutely-positioned flows separately when storing overflow.

This code dates back to the time when absolutely positioned flows were
ignored by all of their ancestors up to the containing block. This
hasn't been true for at least a year.

Closes #9306.
Closes #9309.
Is a partial fix for #9308.

r? @glennw

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9405)
<!-- Reviewable:end -->
@pcwalton
layout: Remove some bogus code that tried to handle
Loading status checks…
9c7b9fa 
absolutely-positioned flows separately when storing overflow.

This code dates back to the time when absolutely positioned flows were
ignored by all of their ancestors up to the containing block. This
hasn't been true for at least a year.

Closes #9306.
Closes #9309.
Is a partial fix for #9308.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 9c7b9fa into servo:master Jan 25, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@ionutgoldan ionutgoldan mentioned this pull request Sep 15, 2017
Merged
3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@glennw glennw

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

unexpected scrollable area with right:-Xpx + overflow:hidden
unexpected overflow
7 participants
@pcwalton @highfive @glennw @bors-servo @KiChjang @nox @jdm
You can’t perform that action at this time.