Do not create stacking contexts for text fragments #11035

mrobinson · 2016-05-05T21:12:03Z

Without this change, each text fragment in a block that establishes a
stacking context will establish its own stacking context. This is
unnecessary and increases the amount of work done during display list
construction. This change should not change output, but should improve
performance.

This change is

highfive · 2016-05-05T21:12:06Z

Warning

These commits modify layout code, but no tests are modified.Please consider adding a test!

mrobinson · 2016-05-05T21:15:32Z

An example of this problem is that following HTML creates around 15 stacking contexts:

<body>
<div style="overflow:scroll;">
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
text<br/>
</div>
</body>

mrobinson · 2016-05-05T21:15:37Z

@pcwalton r?

pcwalton · 2016-05-05T22:40:47Z

@bors-servo: r+

bors-servo · 2016-05-05T22:40:49Z

📌 Commit ea67804 has been approved by pcwalton

bors-servo · 2016-05-06T03:01:09Z

⌛ Testing commit ea67804 with merge b4ac2e7...


        Auto merge of #11035 - mrobinson:no-stacking-contexts-for-text-fragme…

…nts, r=pcwalton Do not create stacking contexts for text fragments Without this change, each text fragment in a block that establishes a stacking context will establish its own stacking context. This is unnecessary and increases the amount of work done during display list construction. This change should not change output, but should improve performance.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11035)

bors-servo · 2016-05-06T03:35:09Z

💔 Test failed - mac-rel-css

highfive · 2016-05-06T03:35:11Z

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/css-transforms-3d-on-anonymous-block-001.htm

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/regions-transforms-019.htm
  └   → /css-transforms-1_dev/html/regions-transforms-019.htm a73900b921283f0b23e810c65b52dfae53733c12
/css-transforms-1_dev/html/reference/regions-transforms-019-ref.htm 7e9b5786c95bdb8b960667bdac0e68e834e8a162
Testing a73900b921283f0b23e810c65b52dfae53733c12 == 7e9b5786c95bdb8b960667bdac0e68e834e8a162

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-abspos-002.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-abspos-007.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-applies-to-002.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-001.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-003.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-004.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-006.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-005.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-007.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-008.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-009.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-010.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-011.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-012.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-013.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-014.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-015.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-016.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-origin-004.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-origin-003.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-origin-005.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-origin-006.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-propagate-inherit-boolean-001.htm

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-transformable-inline-block.htm
  └   → /css-transforms-1_dev/html/transform-transformable-inline-block.htm 01362f021fc8dd139e5f4909ff0255082519ea11
/css-transforms-1_dev/html/reference/transform-transformable-inline-block-ref.htm d9d9e489bc5b4508c01b06c7ff92f19dcc2ece3c
Testing 01362f021fc8dd139e5f4909ff0255082519ea11 == d9d9e489bc5b4508c01b06c7ff92f19dcc2ece3c

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translate-001.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translate-002.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translate-003.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translate-004.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translate-005.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatex-001.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatex-002.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatex-003.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatex-004.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatex-005.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatey-001.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatey-002.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatey-003.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatey-004.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-translatey-005.htm

mrobinson · 2016-05-09T18:03:17Z

I investigated both newly failing tests. for both tests, the test and reference match before my change, but both are rendered incorrectly (according to companion text and display in Firefox and Chromium).

For the first test, the reference has progressed slightly, but the test has not. For the second test, the test itself has progressed, but the reference has not. I think the right thing to do here is to mark these tests as failing. The failing reference reflects an improvement in proper layout, but not a sufficient one to keep the tests passing. Later I can investigate why these are failing.

pcwalton · 2016-05-09T18:37:26Z

I agree with marking them both failing.

highfive · 2016-05-09T19:55:04Z

New code was committed to pull request.

pcwalton · 2016-05-09T22:35:00Z

@bors-servo: r+

bors-servo · 2016-05-09T22:35:01Z

📌 Commit f7134ca has been approved by pcwalton

bors-servo · 2016-05-09T22:35:03Z

⌛ Testing commit f7134ca with merge 4473f91...


        Auto merge of #11035 - mrobinson:no-stacking-contexts-for-text-fragme…

…nts, r=pcwalton Do not create stacking contexts for text fragments Without this change, each text fragment in a block that establishes a stacking context will establish its own stacking context. This is unnecessary and increases the amount of work done during display list construction. This change should not change output, but should improve performance.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11035)

bors-servo · 2016-05-09T23:15:44Z

💔 Test failed - mac-rel-css

highfive · 2016-05-11T14:49:03Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/css-transforms-3d-on-anonymous-block-001.htm
  └   → /css-transforms-1_dev/html/css-transforms-3d-on-anonymous-block-001.htm b0b9af54431bf5cb94d9c97797dd47c2e795f60d
/css-transforms-1_dev/html/reference/css-transforms-3d-anonymous-block-ref.htm eb80639ff23c155cc08ffbcf47490be3b2d32e2e
Testing b0b9af54431bf5cb94d9c97797dd47c2e795f60d == eb80639ff23c155cc08ffbcf47490be3b2d32e2e

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-007.htm
  └   → /css-transforms-1_dev/html/transform-abspos-007.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8

mrobinson · 2016-05-11T15:01:30Z

So it seems like the results differ between mac and linux. Very curious.

highfive · 2016-05-27T22:21:15Z

New code was committed to pull request.


        Do not create stacking contexts for text fragments

Without this change, each text fragment in a block that establishes a stacking context will establish its own stacking context. This is unnecessary and increases the amount of work done during display list construction.

highfive · 2016-06-01T22:40:26Z

New code was committed to pull request.

mrobinson · 2016-06-01T22:40:42Z

@bors-servo try

bors-servo · 2016-06-01T22:40:45Z

⌛ Trying commit 7e4cf6a with merge a0fadf9...


        Auto merge of #11035 - mrobinson:no-stacking-contexts-for-text-fragme…

…nts, r=<try> Do not create stacking contexts for text fragments Without this change, each text fragment in a block that establishes a stacking context will establish its own stacking context. This is unnecessary and increases the amount of work done during display list construction. This change should not change output, but should improve performance.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11035)

bors-servo · 2016-06-01T23:30:55Z

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

mrobinson · 2016-06-01T23:36:25Z

@pcwalton So, I think that the failure in '/css-transforms-1_dev/html/css-transforms-3d-on-anonymous-block-001.htm' is platform specific due to font issues. I looked at the failing image and the text is shifted a few pixels up on Linux and not on Mac. I've posted a new patch which marks the test as failing only on Linux. Does this seem like a reasonable way forward?

pcwalton · 2016-06-02T00:19:16Z

Sure, that's fine.

mrobinson · 2016-06-02T00:22:07Z

@pcwalton Thanks! I'll hand it to bors again.

mrobinson · 2016-06-02T00:22:20Z

@bors-servo r=pcwalton

bors-servo · 2016-06-02T00:22:20Z

📌 Commit 7e4cf6a has been approved by pcwalton

bors-servo · 2016-06-02T00:22:22Z

⌛ Testing commit 7e4cf6a with merge cc7953e...


        Auto merge of #11035 - mrobinson:no-stacking-contexts-for-text-fragme…

…nts, r=pcwalton Do not create stacking contexts for text fragments Without this change, each text fragment in a block that establishes a stacking context will establish its own stacking context. This is unnecessary and increases the amount of work done during display list construction. This change should not change output, but should improve performance.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11035)

bors-servo · 2016-06-02T01:45:40Z

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

jdm · 2016-06-03T15:44:05Z

This PR made css-transforms-1_dev/html/transform-abspos-002.htm and css-transforms-1_dev/html/transform-abspos-007.htm very unstable.

mbrubeck · 2016-06-03T16:33:14Z

Possible also #11574.

mrobinson · 2016-06-03T17:30:10Z

Uugh. Sorry for this breakage. I'm going to try to allocate some time to look at this today.

highfive assigned jdm May 5, 2016

highfive added the S-awaiting-review label May 5, 2016

jdm assigned pcwalton and unassigned jdm May 5, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 6, 2016

mrobinson force-pushed the mrobinson:no-stacking-contexts-for-text-fragments branch from ea67804 to f7134ca May 9, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 9, 2016

highfive removed the S-awaiting-review label May 9, 2016

highfive added the S-awaiting-merge label May 9, 2016

highfive added the S-tests-failed label May 11, 2016

mrobinson force-pushed the mrobinson:no-stacking-contexts-for-text-fragments branch from 41f5cc8 to ee4c8f1 May 27, 2016

mrobinson force-pushed the mrobinson:no-stacking-contexts-for-text-fragments branch from ee4c8f1 to 7e4cf6a Jun 1, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 2, 2016

bors-servo merged commit 7e4cf6a into servo:master Jun 2, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 2, 2016

mrobinson deleted the mrobinson:no-stacking-contexts-for-text-fragments branch Jun 2, 2016

jdm mentioned this pull request Jun 3, 2016

Intermittent failure in css-transforms-1_dev/html/transform-abspos-002.htm and css-transforms-1_dev/html/transform-abspos-007.htm #11561

Closed

mbrubeck mentioned this pull request Jun 3, 2016

Intermittent failure in /css-transforms-1_dev/html/transform-table-007.htm #11574

Closed

servo / servo

Do not create stacking contexts for text fragments #11035

Do not create stacking contexts for text fragments #11035

mrobinson commented May 5, 2016 •

edited by larsbergstrom

highfive commented May 5, 2016

mrobinson commented May 5, 2016

mrobinson commented May 5, 2016

pcwalton commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

mrobinson commented May 9, 2016

pcwalton commented May 9, 2016

highfive commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

highfive commented May 11, 2016

mrobinson commented May 11, 2016

highfive commented May 27, 2016

highfive commented Jun 1, 2016

mrobinson commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

mrobinson commented Jun 1, 2016

pcwalton commented Jun 2, 2016

mrobinson commented Jun 2, 2016 •

edited

mrobinson commented Jun 2, 2016

bors-servo commented Jun 2, 2016

bors-servo commented Jun 2, 2016

bors-servo commented Jun 2, 2016

jdm commented Jun 3, 2016

mbrubeck commented Jun 3, 2016

mrobinson commented Jun 3, 2016

servo / servo

Join GitHub today

Do not create stacking contexts for text fragments #11035

Do not create stacking contexts for text fragments #11035

Conversation

mrobinson commented May 5, 2016 • edited by larsbergstrom

highfive commented May 5, 2016

mrobinson commented May 5, 2016

mrobinson commented May 5, 2016

pcwalton commented May 5, 2016

bors-servo commented May 5, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

mrobinson commented May 9, 2016

pcwalton commented May 9, 2016

highfive commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

highfive commented May 11, 2016

mrobinson commented May 11, 2016

highfive commented May 27, 2016

highfive commented Jun 1, 2016

mrobinson commented Jun 1, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

mrobinson commented Jun 1, 2016

pcwalton commented Jun 2, 2016

mrobinson commented Jun 2, 2016 • edited

mrobinson commented Jun 2, 2016

bors-servo commented Jun 2, 2016

bors-servo commented Jun 2, 2016

bors-servo commented Jun 2, 2016

jdm commented Jun 3, 2016

mbrubeck commented Jun 3, 2016

mrobinson commented Jun 3, 2016

mrobinson commented May 5, 2016 •

edited by larsbergstrom

mrobinson commented Jun 2, 2016 •

edited