Scroll via HitTester #2736
Scroll via HitTester #2736
Conversation
|
r? anyone |
webrender/src/render_backend.rs
Outdated
| let mut should_render = true; | ||
| let node_index = match doc.hit_tester { | ||
| Some(ref hit_tester) => { | ||
| let test = HitTest::new(None, cursor, HitTestFlags::empty()); |
There was a problem hiding this comment.
since this is where we have the node index and this is where checking for scroll_nearest_scrolling_ancestor is really needed, can we rewrite this part as:
let should_render = match doc.hit_tester {
...
};There was a problem hiding this comment.
This bit of ugliness is necessary to avoid a double-borrow of doc, unfortunately. I can leave a comment about that.
webrender/src/clip_scroll_tree.rs
Outdated
| _ => {} | ||
| } | ||
|
|
||
| return self.find_nearest_scrolling_ancestor(node.parent) |
There was a problem hiding this comment.
nit: let's move this into _ => arm and remove return calls
| */ | ||
|
|
||
| // Assumes rect is in the z=0 plane! | ||
| pub fn ray_intersects_rect( |
There was a problem hiding this comment.
oh god, so nice to see the cleanup here!
| } | ||
|
|
||
| let clip_chain_index = clip_and_scroll.clip_chain_index; | ||
| clipped_in |= |
There was a problem hiding this comment.
should probably avoid calling this method if clipped_in is already true
There was a problem hiding this comment.
Yikes! Thanks for pointing out this bug. This should really be
clipped_in = clipped_in ||
self.is_point_clipped_in_for_clip_chain(point, clip_chain_index, &mut test);so that we can rely on short-circuit logic. If it's okay with you, I'll do this in another PR since the change is unrelated to this one.
There was a problem hiding this comment.
A fix for this has landed here: #2739
This change uses the HitTester to do scrolling, which allows us to remove some redundant code. It also will help us to remove bounds for reference frames, which will be useful when we separate the concept of reference frames and stacking contexts.
mrobinson
force-pushed
the
hit-testing-scroll-node
branch
from
c8f0625
to
f5d899e
Compare
|
Cool, thanks! |
|
📌 Commit f5d899e has been approved by |
Scroll via HitTester This change uses the HitTester to do scrolling, which allows us to remove some redundant code. It also will help us to remove bounds for reference frames, which will be useful when we separate the concept of reference frames and stacking contexts. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2736) <!-- Reviewable:end -->
|
💔 Test failed - status-taskcluster |
|
Interesting. Looks like a rawtest failure, but I'm not sure how this change could affect it: |
|
@mrobinson I'm tempted to say let's disable the raw tests for now. They were enabled recently and we can re-enable them without disrupting the other changes, hopefully. |
|
Could we just disable that particular rawtest instead of all of them?
…On Tue, May 8, 2018 at 11:35 AM, Dzmitry Malyshau ***@***.***> wrote:
@mrobinson <https://github.com/mrobinson> I'm tempted to say let's
disable the raw tests for now. They were enabled recently and we can
re-enable them without disrupting the other changes, hopefully.
@jrmuizel <https://github.com/jrmuizel> are you ok with this?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2736 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABFY4OlZIb6ONBwBEvd6Wj6ditBW8EF9ks5twbspgaJpZM4T0jDn>
.
|
|
@bors-servo retry |
Scroll via HitTester This change uses the HitTester to do scrolling, which allows us to remove some redundant code. It also will help us to remove bounds for reference frames, which will be useful when we separate the concept of reference frames and stacking contexts. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2736) <!-- Reviewable:end -->
|
☀️ Test successful - status-appveyor, status-taskcluster |
This change uses the HitTester to do scrolling, which allows us to
remove some redundant code. It also will help us to remove bounds for
reference frames, which will be useful when we separate the concept of
reference frames and stacking contexts.
This change is