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

Remove isolation conflict log. #1956

Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Remove isolation conflict log.

  • Loading branch information
GuanWen-Chen authored and GuanWenChen committed Oct 27, 2017
commit fb0958c293ab9016c484353b6ca3cd50f94dea04
@@ -365,13 +365,6 @@ impl FrameBuilder {
// must be drawn with a transparent background, unless the parent stacking context
// is the root of the page
let isolation = &mut self.stacking_context_store[parent_index.0].isolation;
if *isolation != ContextIsolation::None {

This comment has been minimized.

@kvark

kvark Oct 28, 2017

Member

what if it's ContextIsolation::Items? overwriting it with Full would screw up the follow up logic

This comment has been minimized.

@GuanWen-Chen

GuanWen-Chen Nov 15, 2017

Author Contributor

Thank you for the feedback.

Maybe we could relax the condition here instead of removing it and make the stacking context able to contain multiple child stacking context with blend mode?

According to the code, we only use ContextIsolation::Items for elements which contain preserve-3d transform-style.
And in gecko, an additional stacking context will be created to contain stacking contexts with blend mode.
With this mechanism, we can prevent isolation conflict in a stacking context.
I am not sure if Servo has this mechanism, but I think it might be reasonable to do that.

I will push another pr for this.

This comment has been minimized.

@glennw

glennw Nov 15, 2017

Member

There is a PR nearly ready to merge which should solve most (or all?) of the isolation issues - #2031. Ideally, we'd get that to land, and then perhaps you could investigate if there are any remaining isolation failures and fix them on top of that patch (since it changes the way we handle stacking contexts internally very significantly)?

This comment has been minimized.

@GuanWen-Chen

GuanWen-Chen Nov 15, 2017

Author Contributor

Oh, thank you for the feedback! Then I will cancel the pr and wait for the issue solved.

This comment has been minimized.

@glennw

glennw Nov 15, 2017

Member

Sounds good, thank you!

error!(
"Isolation conflict detected on {:?}: {:?}",
parent_index,
*isolation
);
}
*isolation = ContextIsolation::Full;
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.