layout: Make the stacking context take into account the children transform when calculating overflow areas. by emilio · Pull Request #12843 · servo/servo

emilio · 2016-08-13T09:50:47Z

This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes partially fix Incorrect clipping of nested stacking contexts. #12842 (github issue number if applicable).

There are tests for these changes.

This change is

emilio · 2016-08-13T09:52:23Z

r? @pcwalton

Doesn't deal with every possible transform, so I'll add that before landing this, but I'd like to verify the approach looks good to you.

Also...
@bors-servo: try

bors-servo · 2016-08-13T09:52:25Z

⌛ Trying commit b576864 with merge a143c8e...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

emilio · 2016-08-13T16:50:24Z

@bors-servo: try

bors-servo · 2016-08-13T16:50:27Z

⌛ Trying commit 3458ad8 with merge b0a0a86...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

bors-servo · 2016-08-13T17:01:28Z

💔 Test failed - linux-dev

emilio · 2016-08-13T17:11:09Z

I forgot to update the manifest, the rest of tests seem to be passing, but I'll double-check.

@bors-servo: try

bors-servo · 2016-08-13T17:11:13Z

⌛ Trying commit f40dabf with merge 6504326...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

bors-servo · 2016-08-13T17:58:55Z

💔 Test failed - linux-rel

emilio · 2016-08-13T18:37:01Z

@bors-servo: try

bors-servo · 2016-08-13T18:37:04Z

⌛ Trying commit 6f1fd46 with merge 934977d...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

bors-servo · 2016-08-13T19:34:25Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

glennw · 2016-08-16T21:43:11Z

Reviewed 4 of 5 files at r1, 5 of 5 files at r2.
Review status: 9 of 10 files reviewed at latest revision, 3 unresolved discussions.

components/gfx/display_list/mod.rs, line 703 [r2] (raw file):

                                                       0.0)
                                           .mul(&self.transform);
        let transform_2d = Matrix2D::new(transform.m11, transform.m12,

This doesn't look like it will work with perspective transforms?

I think it's fine to merge without that, since it seems like a net improvement, but perhaps add a TODO comment.

In particular, browser.html uses a perspective transform with a translateZ to zoom the content in and out. Does this handle that case?

tests/wpt/mozilla/meta/MANIFEST.json, line 3963 [r2] (raw file):

          }
        ],
        "css/overflow_transformed_sc_rotate_ref.html": [

This seems like an extra entry?

tests/wpt/mozilla/meta/MANIFEST.json, line 13207 [r2] (raw file):

        }
      ],
      "css/overflow_transformed_sc_rotate_ref.html": [

Same here

Comments from Reviewable

glennw · 2016-08-16T21:43:20Z

Reviewed 1 of 5 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

…sform when calculating overflow areas. This only works for simple translations and similar, but I want Patrick to validate my approach.

emilio · 2016-08-16T22:48:52Z

Review status: 4 of 10 files reviewed at latest revision, 3 unresolved discussions.

components/gfx/display_list/mod.rs, line 703 [r2] (raw file):

Previously, glennw (Glenn Watson) wrote…

This doesn't look like it will work with perspective transforms?

I think it's fine to merge without that, since it seems like a net improvement, but perhaps add a TODO comment.

In particular, browser.html uses a perspective transform with a translateZ to zoom the content in and out. Does this handle that case?

Yep, you're right. Added a TODO comment.

It handles at least as many transformations as before, since we're only increasing the overflow area. Just in case, just checked and it works fine with this patch applied.

tests/wpt/mozilla/meta/MANIFEST.json, line 3963 [r2] (raw file):

Previously, glennw (Glenn Watson) wrote…

This seems like an extra entry?

Yep, forgot to remove the `` from the reference.

Comments from Reviewable

glennw · 2016-08-17T01:06:39Z

@emilio r=me with those fixes

emilio · 2016-08-17T01:36:04Z

@bors-servo: r=glennw (they were already fixed).

bors-servo · 2016-08-17T01:36:05Z

📌 Commit babb5d4 has been approved by glennw

bors-servo · 2016-08-17T02:20:41Z

⌛ Testing commit babb5d4 with merge d85ed17...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

bors-servo · 2016-08-17T03:08:28Z

💔 Test failed - linux-rel

highfive · 2016-08-17T03:08:31Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/perspective-origin-004.htm
  └   → /css-transforms-1_dev/html/perspective-origin-004.htm 68d5e8d6d5edcd2bef5ecaaf62cbbbac863dee22
/css-transforms-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing 68d5e8d6d5edcd2bef5ecaaf62cbbbac863dee22 == d4aa213e3e31e41d0d8e84aec79e94f860f27178

emilio · 2016-08-17T04:00:05Z

@bors-servo: retry #9087

bors-servo · 2016-08-17T05:35:17Z

⌛ Testing commit babb5d4 with merge 5f16958...

@pcwalton

layout: Make the stacking context take into account the children transform when calculating overflow areas.  This is a potential fix for #12842. I have done only the math to handle simple transforms because it's three AM, but I'd like @pcwalton to verify my approach, or suggest an alternative. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes partially fix #12842 (github issue number if applicable).  - [x] There are tests for these changes.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12843)

bors-servo · 2016-08-17T06:32:58Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned ghost Aug 13, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 13, 2016

highfive assigned pcwalton and unassigned ghost Aug 13, 2016

emilio force-pushed the transforms branch 3 times, most recently from bf0b4bc to 3458ad8 Compare August 13, 2016 16:49

highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016

emilio force-pushed the transforms branch from 3458ad8 to f40dabf Compare August 13, 2016 17:10

highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016

emilio force-pushed the transforms branch from f40dabf to 6f1fd46 Compare August 13, 2016 18:36

highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016

emilio added 2 commits August 16, 2016 15:34

layout: Make the stacking context take into account the children tran…

fd3a99e

…sform when calculating overflow areas. This only works for simple translations and similar, but I want Patrick to validate my approach.

Add a test for nested stacking context overflow areas.

babb5d4

emilio force-pushed the transforms branch from 6f1fd46 to babb5d4 Compare August 16, 2016 22:45

highfive assigned glennw and unassigned pcwalton Aug 17, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 17, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 17, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 17, 2016

bors-servo merged commit babb5d4 into servo:master Aug 17, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 17, 2016

emilio deleted the transforms branch August 17, 2016 07:41

Uh oh!

Comments

Conversation

emilio commented Aug 13, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Aug 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

emilio commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

emilio commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

emilio commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

bors-servo commented Aug 13, 2016

Uh oh!

glennw commented Aug 16, 2016

Uh oh!

glennw commented Aug 16, 2016

Uh oh!

emilio commented Aug 16, 2016

Uh oh!

glennw commented Aug 17, 2016

Uh oh!

emilio commented Aug 17, 2016

Uh oh!

bors-servo commented Aug 17, 2016

Uh oh!

bors-servo commented Aug 17, 2016

Uh oh!

bors-servo commented Aug 17, 2016

Uh oh!

highfive commented Aug 17, 2016

Uh oh!

emilio commented Aug 17, 2016

Uh oh!

bors-servo commented Aug 17, 2016

Uh oh!

bors-servo commented Aug 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emilio commented Aug 13, 2016 •

edited by larsbergstrom

Loading

emilio commented Aug 13, 2016 •

edited

Loading