Skip to content
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

Integrate iframes into the display list #7950

Merged
merged 1 commit into from Oct 20, 2015

Conversation

@mrobinson
Copy link
Member

mrobinson commented Oct 10, 2015

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.

Review on Reviewable

@mrobinson
Copy link
Member Author

mrobinson commented Oct 10, 2015

@nox
Copy link
Member

nox commented Oct 10, 2015

./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 :
@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch 2 times, most recently from 7252c6a to 1b247e7 Oct 11, 2015
}

fn find_last_child_layer_info(stacking_context: &mut StackingContext) -> Option<LayerInfo> {
match stacking_context.display_list.layered_children.back() {

This comment has been minimized.

@pcwalton

pcwalton Oct 13, 2015

Contributor

nit: Could be if let

This comment has been minimized.

@mrobinson

mrobinson Oct 13, 2015

Author Member

Thanks. This made the block quite a bit simpler.

@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch from 1b247e7 to 59ae167 Oct 13, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Oct 15, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2015

📌 Commit 59ae167 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2015

Testing commit 59ae167 with merge 2d443f8...

bors-servo pushed a commit that referenced this pull request Oct 15, 2015
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.

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

bors-servo commented Oct 15, 2015

💔 Test failed - mac-rel-css

@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch from 59ae167 to 89c41d6 Oct 15, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

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.

@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch from 89c41d6 to 47bcc35 Oct 15, 2015
@jdm jdm removed the S-fails-tidy label Oct 16, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

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

@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch from 47bcc35 to 58a3071 Oct 16, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 16, 2015

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

@jdm jdm removed the S-needs-rebase label Oct 16, 2015
@paulrouget
Copy link
Contributor

paulrouget commented Oct 19, 2015

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 #7566.
Fixes #7796.
@mrobinson mrobinson force-pushed the mrobinson:layerize-iframes branch from 58a3071 to ac5525a Oct 20, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 20, 2015

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

@pcwalton
Copy link
Contributor

pcwalton commented Oct 20, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 20, 2015

📌 Commit ac5525a has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Oct 20, 2015

Testing commit ac5525a with merge 11d23a4...

bors-servo pushed a commit that referenced this pull request Oct 20, 2015
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.

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

bors-servo commented Oct 20, 2015

@bors-servo bors-servo merged commit ac5525a into servo:master Oct 20, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:layerize-iframes branch Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.