Box with overflow:hidden cut inner div with absolute position by content edge, not by padding edge #16576

ocerman · 2017-04-23T07:04:41Z

Tested on Windows 8.1 x64

<html>
	<body>
		<style>
			#box{
				background-color: #080b0c;
				height: 100px;
				overflow: hidden;
				padding: 0 15px;
				position: relative;
			}
			#inside {
				position: absolute;
				right: 0px;
				width: 100px;
				height: 100%;
				background: red;
			}
		</style>
		<div id="box">
			<div id="inside">
			</div>
		</div>
	</body>
</html>

aacunningham · 2017-04-28T02:18:11Z

What's the best way to recreate these manual testcases? Would it be to create a wpt? Or just create a html file and point servo to it?

jdm · 2017-04-28T03:09:47Z

@aacunningham It's easiest to create a local html file and point Servo at it. To submit an actual fix, it's recommended to convert it into a reftest as part of the automated testsuite.

aacunningham · 2017-04-28T06:19:57Z

After reading through some of the spec on block widths, learning some lingo, and reading a bit of code, I think I'd like to take this on. Do I assign myself with highfive or through you, @jdm?

sciguyryan · 2017-04-29T17:01:41Z

Is it possible that issue #16559 is also related to this since they do seem to have some traits in common?

jdm · 2017-04-29T17:35:31Z

I am doubtful.

aacunningham · 2017-04-29T17:40:14Z

Same, since that issue seems to apply more to the hover action and not an issue with the rendered width of boxes. I'm about to do some debugging today, but I'm going to start my look in the width calculations for absolute nonreplaced blocks and see where that gets me

Edit: that or look into how overflow is handled, who knows

jdm · 2017-04-29T17:59:28Z

https://github.com/servo/servo/wiki/Getting-started-with-layout may have tips to help you understand the code a bit better.

jdm · 2017-04-29T18:00:40Z

Additionally, we have https://github.com/servo/servo/tree/master/etc/layout_viewer which can help break down what happens in which part of layout for a given testcase.

aacunningham · 2017-04-29T18:00:56Z

Don't think I've read that one yet, thanks!

jdm · 2017-04-29T18:13:51Z

I retract my earlier doubt. @sciguyryan is right; this looks like it's a regression from #16336 as well.

jdm · 2017-04-29T18:14:12Z

cc @mrobinson

aacunningham · 2017-04-29T18:19:47Z

I probably shouldn't have had any doubt since I'm pretty new to this, but also retracted. What's the best way moving forward with this ticket then? Would it make more sense to have a new issue made that's a little more overarching, or is this issue fine? And since mrobinson would know the most about how the regression was introduced, would it make more sense to have them take over, or is it fine if I keep looking into the issue?

jdm · 2017-04-29T18:28:03Z

Clearly the cause of regression is the same, but the fix for both issues might be different, so I don't think it makes sense to file an overarching one. I don't think there's anything wrong with you continuing to investigate this weekend; if you come up with any conclusions then perhaps @mrobinson could provide useful input on Monday.

aacunningham · 2017-04-29T18:32:18Z

Perfect, thanks a lot!

aacunningham · 2017-04-30T06:17:01Z

Alright, here's what I've discovered:

Using -Z dump-display-list, I got this info:

Pre #16336

┌ Display List
│  ├─ Items
│  │  ├─ PushStackingContext(StackingContext at TypedRect(1920px×1043px at (0px,0px)) with overflow TypedRect(1920px×1043px at (0px,0px)): StackingContextId(0)) StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0, 0, 0, 0) @ TypedRect(1920px×1043px at (0px,0px)) ClippingRegion::Max StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0, 0, 0, 0) @ TypedRect(1904px×100px at (8px,8px)) ClippingRegion::Max StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ PushStackingContext(StackingContext at TypedRect(1904px×100px at (8px,8px)) with overflow TypedRect(1904px×100px at (0px,0px)): StackingContextId(139840704336464)) StackingContext: StackingContextId(139840704336464) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0.03137255, 0.043137256, 0.047058824, 1) @ TypedRect(1904px×100px at (0px,0px)) ClippingRegion::Max StackingContext: StackingContextId(139840704336464) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(1, 0, 0, 1) @ TypedRect(100px×100px at (1804px,0px)) ClippingRegion(Rect=TypedRect(1904px×100px at (0px,0px)), Complex=[]) StackingContext: StackingContextId(139840704336608) ScrollRoot: ScrollRootId(0)
│  │  ├─ PopStackingContext(StackingContextId(139840704336464) StackingContext: StackingContextId(139840704336464) ScrollRoot: ScrollRootId(0)
│  │  └─ PopStackingContext(StackingContextId(0) StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)

Post #16336

┌ Display List
│  ├─ Items
│  │  ├─ PushStackingContext(StackingContext at TypedRect(1920px×1043px at (0px,0px)) with overflow TypedRect(1920px×1043px at (0px,0px)): StackingContextId(0)) StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0, 0, 0, 0) @ TypedRect(1920px×1043px at (0px,0px)) ClippingRegion::Max StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0, 0, 0, 0) @ TypedRect(1904px×100px at (8px,8px)) ClippingRegion::Max StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ PushStackingContext(StackingContext at TypedRect(1904px×100px at (8px,8px)) with overflow TypedRect(1904px×100px at (0px,0px)): StackingContextId(140606955442768)) StackingContext: StackingContextId(140606955442768) ScrollRoot: ScrollRootId(0)
│  │  ├─ PushScrollRoot(ScrollRoot { id: ScrollRootId(140606955442768), parent_id: ScrollRootId(0), clip: ClippingRegion(Rect=TypedRect(1874px×100px at (0px,0px)), Complex=[]), content_rect: TypedRect(1874px×100px at (15px,0px)) } StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(0.03137255, 0.043137256, 0.047058824, 1) @ TypedRect(1904px×100px at (0px,0px)) ClippingRegion::Max StackingContext: StackingContextId(140606955442768) ScrollRoot: ScrollRootId(0)
│  │  ├─ SolidColor rgba(1, 0, 0, 1) @ TypedRect(100px×100px at (1804px,0px)) ClippingRegion::Max StackingContext: StackingContextId(140606955442912) ScrollRoot: ScrollRootId(140606955442768)
│  │  ├─ PopStackingContext(StackingContextId(140606955442768) StackingContext: StackingContextId(140606955442768) ScrollRoot: ScrollRootId(0)
│  │  └─ PopStackingContext(StackingContextId(0) StackingContext: StackingContextId(0) ScrollRoot: ScrollRootId(0)

Diffed the two, found the main difference being that the new code adds a ScrollRoot. I'm not entirely sure what ScrollRoot is, BUT, if it's a representation of the scroll containers referenced in this CSS spec, then it's appropriate to have ScrollRoot added. So correct me if I'm wrong, but I think the issue isn't that ScrollRoot is being added, but that there is something wrong in the calculations for ScrollRoot; Post #16336, ScrollRoot's ClippingRegion is 30px smaller than it should (from the 15px padding).

Going into this, I found that the ClippingRegion for the new ScrollRoot isn't based on the border box but the content box (which is the border box but with the padding added, not sure if those are technical terms, just variable names). So I modified the code that added the padding and the test case is passing now. I'm not sure if that's the correct solution, but it does work. Trying to run tests, but they run pretty slowly on this machine so it might be easier to have the CI systems handle it.

If none of this seems completely crazy, I'll clean up my code, learn how to add a reftest, and push up a PR.

jdm · 2017-04-30T12:39:49Z

Seems plausible! https://github.com/servo/servo/blob/master/tests/wpt/README.md#writing-new-tests has information about adding a new test.


        Fix the size and position of overflow:scroll clip

The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. Fixes servo#16576.


        Fix the size and position of overflow:scroll clip

The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes servo#16576.

mrobinson · 2017-06-30T17:29:07Z

I have a fix for this at mrobinson@f190445. It's just waiting on the WebRender update.


        Fix the size and position of overflow:scroll clip

The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes servo#16576.


        Fix the size and position of overflow:scroll clip

The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes servo#16576.


        Fix the size and position of overflow:scroll clip

The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes servo#16576.


        Auto merge of #17712 - mrobinson:overflow-scroll-clip, r=<try>

Fix the size and position of overflow:scroll clip The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes #16576.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16576 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17712)


        Auto merge of #17712 - mrobinson:overflow-scroll-clip, r=emilio

Fix the size and position of overflow:scroll clip The clip for overflow:scroll clip was not including the space for padding, which could lead to some content being clipped when it shouldn't. This changes fixes that issue. A test is skipped because this fix also stops hiding a failure due to mispositioned text. Fixes #16576.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16576 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17712)

jdm added A-layout/block C-has-manual-testcase labels Apr 23, 2017

KiChjang added the C-assigned label Apr 28, 2017

mrobinson self-assigned this Jun 27, 2017

mrobinson mentioned this issue Jul 13, 2017

Fix the size and position of overflow:scroll clip #17712

Merged

4 of 5 tasks complete

bors-servo closed this in #17712 Jul 13, 2017

servo / servo

Box with overflow:hidden cut inner div with absolute position by content edge, not by padding edge #16576

Box with overflow:hidden cut inner div with absolute position by content edge, not by padding edge #16576

ocerman commented Apr 23, 2017

aacunningham commented Apr 28, 2017 •

edited

jdm commented Apr 28, 2017

aacunningham commented Apr 28, 2017

sciguyryan commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017 •

edited

jdm commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017

jdm commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017 •

edited

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017

aacunningham commented Apr 30, 2017 •

edited

jdm commented Apr 30, 2017

mrobinson commented Jun 30, 2017

servo / servo

Join GitHub today

Box with overflow:hidden cut inner div with absolute position by content edge, not by padding edge #16576

Box with overflow:hidden cut inner div with absolute position by content edge, not by padding edge #16576

Comments

ocerman commented Apr 23, 2017

aacunningham commented Apr 28, 2017 • edited

jdm commented Apr 28, 2017

aacunningham commented Apr 28, 2017

sciguyryan commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017 • edited

jdm commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017

jdm commented Apr 29, 2017

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017 • edited

jdm commented Apr 29, 2017

aacunningham commented Apr 29, 2017

aacunningham commented Apr 30, 2017 • edited

jdm commented Apr 30, 2017

mrobinson commented Jun 30, 2017

aacunningham commented Apr 28, 2017 •

edited

aacunningham commented Apr 29, 2017 •

edited

aacunningham commented Apr 29, 2017 •

edited

aacunningham commented Apr 30, 2017 •

edited