Inherit the clip chain of IFrame #2875

kvark · 2018-07-06T15:57:32Z

~~This is an experiment to fix #2834 on our side by inheriting the clip chains of the iFrames.~~
Fixes #2834
~~I really don't like the way it turns out... and the old code got me confused because the info.clip_node_id was completely ignored in flatten_iframe.~~
@mrobinson please take a look, cc @staktrace

This change is

bors-servo · 2018-07-06T20:04:34Z

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

mrobinson · 2018-07-09T08:03:07Z

Hrm. Weird. It doesn't seem like this should be necessary. I left an explanation here: #2834 (comment)

gw3583 · 2018-07-16T02:26:34Z

Do we want to keep this open?

mrobinson · 2018-07-17T16:56:28Z

Apologies! I didn't understand what this PR did before, but now I do. I think this is one way that we can fix the issue. Tomorrow I'll take another look at this.

kvark · 2018-07-17T19:14:14Z

I updated (or, rather reimplemented) the code on latest master.
Edit: also, https://treeherder.mozilla.org/#/jobs?repo=try&revision=2af2bc05f7ca7043c234805de9f2eeaaca7bcf92

mrobinson

Thanks for fixing this and sorry again that I didn't understand this the first time!

mrobinson · 2018-07-18T06:39:24Z

webrender/src/display_list_flattener.rs

@@ -169,6 +169,9 @@ pub struct DisplayListFlattener<'a> {
    /// The map of all font instances.
    font_instances: FontInstanceMap,

+    /// The map keeping track of the root clip chains associated with pipelines.
+    pipeline_clip_chains: FastHashMap<PipelineId, ClipChainIndex>,


If you want to avoid the cost of hashing here, this can just be a stack. You would push ClipChainIndex(0) for the root pipeline and then push again when processing an iframe element.

good point!

mrobinson · 2018-07-18T06:39:24Z

webrender/src/display_list_flattener.rs

@@ -539,6 +543,17 @@ impl<'a> DisplayListFlattener<'a> {
            ),
        );

+        match clip_and_scroll_ids.clip_node_id {


I think what you want to do here is to simply use the newly added clip node. So

self.pipeline_clip_chains.insert( iframe_pipeline_id, self.id_to_index_mapper.get_node_index(info.clip_id) );

but that's the whole point of the PR - to take clip_and_scroll_ids into account

mrobinson · 2018-07-18T06:39:24Z

webrender/src/display_list_flattener.rs

+                    .iter()
+                    .map(|id| self.id_to_index_mapper.get_clip_node_index(*id))
+                    .collect();
+                let parent = match info.parent {


I think you can map_or_else here:

let parent = info.parent.map_or_else( || self.pipeline_clip_chains.get(&pipeline_id).cloned(), |id| self.id_to_index_mapper.get_clip_node_index(*id)) );

Oh, I see that map_or_else won't wok here with parent accepting an Option. Perhaps it makes sense to always fall back to ClipChainIndex(0).

kvark · 2018-07-18T17:02:14Z

Thanks for the review @mrobinson ! Please take another look.

mrobinson

Looking good, though I still think we should always use the newly added clip node. See below for an explanation.

mrobinson · 2018-07-18T17:14:49Z

webrender/src/display_list_flattener.rs

@@ -539,6 +543,16 @@ impl<'a> DisplayListFlattener<'a> {
            ),
        );

+        let clip_chain_index = match clip_and_scroll_ids.clip_node_id {


Here I think you always want to use the newly added clip node which is represented by info.clip_id. This ensures that contents don't escape the iframe at all. Trying to reach for the parent means that the content might escape the iframe and only be contained by whatever clip happens to be the parent here. If Gecko and Servo want to contain the content to the iframe (which I think they always do), they would end up needing to push an extra clip node. I think that info.clip_id is the correct choice here to ensure consistent behavior.

In the future maybe it makes sense that iframes don't automatically clip, in which case we wouldn't push this clip node and we would in that case look at the clipping node of clip_and_scroll_ids in this spot. That would allow clients to allow iframe contents to escape their rectangles, while still avoiding having to push an extra clip node to prevent it.

I agree that Gecko at least always wants to contain stuff inside the iframe to the iframe clip. If I'm understanding this right, what @mrobinson is suggesting would fix https://bugzilla.mozilla.org/show_bug.cgi?id=1473940

kvark · 2018-07-18T20:10:26Z

Here is a thing. I tested the following variants on @staktrace reduced test case:

original FF from m-c
my PR
@mrobinson 's proposed change

And I'm seeing all these producing the same faulty behavior: content is mostly clipped, except when scrolling. I was able to (wasn't easy!) catch a screenshot of it:

So, looks like we are back to square one? The behavior was certainly fixed when I first filed the PR. Something has changed now.

staktrace · 2018-07-18T21:14:22Z

It's possible that there are two different bugs here. I'll test the other STR (from bug 1462955) and see if this patch fixes that.

staktrace · 2018-07-18T21:19:27Z

Confirmed that the build from your try push fixes bug 1462955. So I think this is still worth landing. Bug 1458373 is likely a different root cause.

kvark · 2018-07-18T21:57:07Z

@staktrace thanks for verifying!
I pushed another try with the change proposed by @mrobinson , gotta see if it works as well as my original change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c5e83647b894b6585ad0b249801ffab82df9f67

staktrace · 2018-07-19T02:23:55Z

@kvark The try push in the previous comment also fixes the bug


        Inherit the clip chain of IFrame

kvark · 2018-07-19T02:48:01Z

Alright, I changed the code according to @mrobinson suggestion, we had a good try push, and I believe all the review concerns are addressed. Let's not hold this patch for too much longer then.
@bors-servo r=mrobinson

bors-servo · 2018-07-19T02:48:02Z

📌 Commit 415f1a8 has been approved by mrobinson

bors-servo · 2018-07-19T03:56:49Z

⌛ Testing commit 415f1a8 with merge fc91e86...


        Auto merge of #2875 - kvark:clip-chain-inherit, r=mrobinson

@mrobinson

Inherit the clip chain of IFrame ~~This is an experiment to fix #2834 on our side by inheriting the clip chains of the iFrames.~~ Fixes #2834 ~~I really don't like the way it turns out... and the old code got me confused because the `info.clip_node_id` was completely ignored in `flatten_iframe`.~~ @mrobinson please take a look, cc @staktrace  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2875)

bors-servo · 2018-07-19T05:58:25Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: mrobinson
Pushing fc91e86 to master...

kvark mentioned this pull request Jul 9, 2018

Content area stuff is drawn over the chrome in Firefox #2834

Closed

kvark force-pushed the kvark:clip-chain-inherit branch from c1b91fa to b96be63 Jul 17, 2018

mrobinson requested changes Jul 18, 2018

View changes

kvark force-pushed the kvark:clip-chain-inherit branch from b96be63 to 1514925 Jul 18, 2018

mrobinson requested changes Jul 18, 2018

View changes

Inherit the clip chain of IFrame

Loading status checks…

415f1a8

kvark force-pushed the kvark:clip-chain-inherit branch from 1514925 to 415f1a8 Jul 19, 2018

kvark changed the title ~~[WIP] Inherit the clip chain of IFrame~~ Inherit the clip chain of IFrame Jul 19, 2018

bors-servo merged commit 415f1a8 into servo:master Jul 19, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

kvark deleted the kvark:clip-chain-inherit branch Jul 19, 2018

kvark mentioned this pull request Aug 23, 2018

Fix the yaml reader so that clip ids from complex-clip are correctly represented. #2982

Closed

servo / webrender

Inherit the clip chain of IFrame #2875

Inherit the clip chain of IFrame #2875

kvark commented Jul 6, 2018 •

edited

bors-servo commented Jul 6, 2018

mrobinson commented Jul 9, 2018

gw3583 commented Jul 16, 2018

mrobinson commented Jul 17, 2018

kvark commented Jul 17, 2018 •

edited

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 18, 2018

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 18, 2018

staktrace commented Jul 18, 2018

staktrace commented Jul 18, 2018

kvark commented Jul 18, 2018 •

edited

staktrace commented Jul 19, 2018

kvark commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

servo / webrender

Join GitHub today

Inherit the clip chain of IFrame #2875

Inherit the clip chain of IFrame #2875

Conversation

kvark commented Jul 6, 2018 • edited

bors-servo commented Jul 6, 2018

mrobinson commented Jul 9, 2018

gw3583 commented Jul 16, 2018

mrobinson commented Jul 17, 2018

kvark commented Jul 17, 2018 • edited

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 18, 2018

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 18, 2018

staktrace commented Jul 18, 2018

staktrace commented Jul 18, 2018

kvark commented Jul 18, 2018 • edited

staktrace commented Jul 19, 2018

kvark commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

kvark commented Jul 6, 2018 •

edited

kvark commented Jul 17, 2018 •

edited

kvark commented Jul 18, 2018 •

edited