Integrate iframes into the display list by mrobinson · Pull Request #7950 · servo/servo

mrobinson · 2015-10-10T00:24:32Z

Instead of always promoting iframes to StackingContexts, integrate them
into the display list. This prevents stacking bugs when
non-stacking-context elements should be drawn on top of iframes.

To accomplish this, we add another step to ordering layer creation,
where LayeredItems in the DisplayList are added to layers described by
the LayerInfo structures collected at the end of the DisplayList.
Unlayered items that follow these layered items are added to
synthesized layers.

Another result of this change is that iframe layers can be positioned
directly at the location of the iframe fragment, eliminating the need
for the SubpageLayerInfo struct entirely.

Iframes are the first type of content treated this way, but this change
opens up the possibility to properly order canvas and all other layered
content that does not create a stacking context.

mrobinson · 2015-10-10T00:24:39Z

@pcwalton r?

nox · 2015-10-10T15:34:18Z

./components/gfx/paint_task.rs:22: use statement is not in alphabetical order
    expected: msg::compositor_msg::ScrollPolicy
    found: msg::compositor_msg::{Epoch, FrameTreeId, LayerId, LayerKind, LayerProperties, PaintListener}
./components/gfx/paint_task.rs:23: use statement is not in alphabetical order
    expected: msg::compositor_msg::{Epoch, FrameTreeId, LayerId, LayerKind, LayerProperties, PaintListener}
    found: msg::compositor_msg::ScrollPolicy
./components/gfx/display_list/mod.rs:102: extra space before :

pcwalton · 2015-10-13T00:05:46Z

components/gfx/display_list/mod.rs

nit: Could be if let

Thanks. This made the block quite a bit simpler.

pcwalton · 2015-10-15T15:53:38Z

@bors-servo: r+

bors-servo · 2015-10-15T15:53:38Z

📌 Commit 59ae167 has been approved by pcwalton

bors-servo · 2015-10-15T19:41:14Z

⌛ Testing commit 59ae167 with merge 2d443f8...

Integrate iframes into the display list Instead of always promoting iframes to StackingContexts, integrate them into the display list. This prevents stacking bugs when non-stacking-context elements should be drawn on top of iframes. To accomplish this, we add another step to ordering layer creation, where LayeredItems in the DisplayList are added to layers described by the LayerInfo structures collected at the end of the DisplayList. Unlayered items that follow these layered items are added to synthesized layers. Another result of this change is that iframe layers can be positioned directly at the location of the iframe fragment, eliminating the need for the SubpageLayerInfo struct entirely. Iframes are the first type of content treated this way, but this change opens up the possibility to properly order canvas and all other layered content that does not create a stacking context.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7950)

bors-servo · 2015-10-15T20:13:08Z

💔 Test failed - mac-rel-css

mrobinson · 2015-10-15T23:45:34Z

The latest version of this PR fixes the test failures by avoiding the creation of empty iframes. I tested things locally and the tests appear to be working.

bors-servo · 2015-10-16T14:06:03Z

☔ The latest upstream changes (presumably #8050) made this pull request unmergeable. Please resolve the merge conflicts.

mrobinson · 2015-10-16T20:57:28Z

I have uploaded a new version of this patch which resolves the merge conflict.

paulrouget · 2015-10-19T05:44:34Z

I think with this PR, Servo crashes with this example:

<!DOCTYPE html>

<style>
  div, iframe {
    transform: scale(1);
  }
</style>

<iframe></iframe>
<div></div>

Instead of always promoting iframes to StackingContexts, integrate them into the display list. This prevents stacking bugs when non-stacking-context elements should be drawn on top of iframes. To accomplish this, we add another step to ordering layer creation, where LayeredItems in the DisplayList are added to layers described by the LayerInfo structures collected at the end of the DisplayList. Unlayered items that follow these layered items are added to synthesized layers. Another result of this change is that iframe layers can be positioned directly at the location of the iframe fragment, eliminating the need for the SubpageLayerInfo struct entirely. Iframes are the first type of content treated this way, but this change opens up the possibility to properly order canvas and all other layered content that does not create a stacking context. Fixes servo#7566. Fixes servo#7796.

mrobinson · 2015-10-20T14:29:49Z

@paulrouget Thanks for the note. I've fixed the issue in the latest version of the patch.

pcwalton · 2015-10-20T22:01:35Z

@bors-servo: r+

bors-servo · 2015-10-20T22:01:37Z

📌 Commit ac5525a has been approved by pcwalton

bors-servo · 2015-10-20T22:01:40Z

⌛ Testing commit ac5525a with merge 11d23a4...

Integrate iframes into the display list Instead of always promoting iframes to StackingContexts, integrate them into the display list. This prevents stacking bugs when non-stacking-context elements should be drawn on top of iframes. To accomplish this, we add another step to ordering layer creation, where LayeredItems in the DisplayList are added to layers described by the LayerInfo structures collected at the end of the DisplayList. Unlayered items that follow these layered items are added to synthesized layers. Another result of this change is that iframe layers can be positioned directly at the location of the iframe fragment, eliminating the need for the SubpageLayerInfo struct entirely. Iframes are the first type of content treated this way, but this change opens up the possibility to properly order canvas and all other layered content that does not create a stacking context.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7950)

bors-servo · 2015-10-20T22:46:54Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 10, 2015

nox closed this Oct 10, 2015

nox reopened this Oct 10, 2015

nox added A-gfx/displaylist S-fails-tidy `./mach test-tidy` reported errors. labels Oct 10, 2015

mrobinson force-pushed the layerize-iframes branch 2 times, most recently from 7252c6a to 1b247e7 Compare October 12, 2015 16:30

pcwalton reviewed Oct 13, 2015
View reviewed changes

jdm assigned pcwalton Oct 13, 2015

mrobinson mentioned this pull request Oct 13, 2015

absolute iframe z-index is wrong #7796

Closed

mrobinson force-pushed the layerize-iframes branch from 1b247e7 to 59ae167 Compare October 13, 2015 20:59

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 Oct 15, 2015

mrobinson mentioned this pull request Oct 15, 2015

Stacking context issue with CSS transforms #7779

Closed

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 Oct 15, 2015

mrobinson force-pushed the layerize-iframes branch from 59ae167 to 89c41d6 Compare October 15, 2015 23:44

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 15, 2015

mrobinson force-pushed the layerize-iframes branch from 89c41d6 to 47bcc35 Compare October 15, 2015 23:49

jdm removed the S-fails-tidy `./mach test-tidy` reported errors. label Oct 16, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 16, 2015

mrobinson force-pushed the layerize-iframes branch from 47bcc35 to 58a3071 Compare October 16, 2015 20:34

jdm removed the S-needs-rebase There are merge conflict errors. label Oct 16, 2015

paulrouget mentioned this pull request Oct 19, 2015

Servo + Browser.html: webplatform bugs browserhtml/browserhtml#562

Closed

92 tasks

mrobinson force-pushed the layerize-iframes branch from 58a3071 to ac5525a Compare October 20, 2015 14:29

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 Oct 20, 2015

bors-servo merged commit ac5525a into servo:master Oct 20, 2015

bors-servo mentioned this pull request Oct 20, 2015

compositing: Split Servo up into multiple sandboxed processes. #6884

Closed

mrobinson deleted the layerize-iframes branch October 20, 2015 23:22

Uh oh!

Conversation

mrobinson commented Oct 10, 2015

Uh oh!

mrobinson commented Oct 10, 2015

Uh oh!

nox commented Oct 10, 2015

Uh oh!

pcwalton Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

mrobinson Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

pcwalton commented Oct 15, 2015

Uh oh!

bors-servo commented Oct 15, 2015

Uh oh!

bors-servo commented Oct 15, 2015

Uh oh!

bors-servo commented Oct 15, 2015

Uh oh!

mrobinson commented Oct 15, 2015

Uh oh!

bors-servo commented Oct 16, 2015

Uh oh!

mrobinson commented Oct 16, 2015

Uh oh!

paulrouget commented Oct 19, 2015

Uh oh!

mrobinson commented Oct 20, 2015

Uh oh!

pcwalton commented Oct 20, 2015

Uh oh!

bors-servo commented Oct 20, 2015

Uh oh!

bors-servo commented Oct 20, 2015

Uh oh!

bors-servo commented Oct 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants