Implement improved apz hit testing

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: kats)

Tracking

26 Branch
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

Attachments

(4 attachments, 21 obsolete attachments)

7.58 KB, patch
botond
: review+
Details | Diff | Splinter Review
13.12 KB, patch
botond
: review+
roc
: review+
Details | Diff | Splinter Review
16.15 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.94 KB, patch
botond
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
Currently apz hit testing is based on bounding rects which doesn't take into account semi-transparent elements that might overlay a scrollable frame. See bug 915213 for details.
I was talking to kats about this this morning. Currently the APZC event targeting code doesn't take account of:
-- Non-scrollable content that can receive events, covering a scrollable element with an APZC
-- A scrollable element with an APZC which is pointer-events:none (we'll accept touch events anyway) (but it could have pointer-events:auto descendants which are touchable)
Bug 845169 is relevant since there we have some code to compute the event-targetable region (of an entire subframe).
But the bug 845169 approach doesn't work with compositor-animated transforms.
The most straightforward way to fix this would be to generate, for each layer, a region that indicates which parts of it are transparent to events. That would mean extending the display list we build for painting so that it has all the event-handling information as well. That's kind of a pain since it means generating zillions of nsDisplayBackgroundColor layers currently, one for every single element --- and many pages have a lot of elements that paint nothing of their own, so this would be a large expansion in the display list :-(.
One thing to think about is how important it is to have panning gesture events initiated on the compositor thread. Obviously it would be nice. However, for scrollable elements other than the rootmost content viewport, we currently don't create layers for the scrolled contents initially, so we can't start async scrolling until we've done one trip through the main thread event loop to build layers for the newly-scrolled element. So starting the panning on the compositor thread doesn't actually help us in that case.
Depends on: 928833
No longer blocks: 795567
Component: Graphics: Layers → Panning and Zooming
Hardware: x86_64 → All
Gonna start looking into what's left here. Bug 945203 landed a while back to add touch-sensitive regions to the layers but last I checked it wasn't working properly.
Assignee: nobody → bugmail.mozilla
Without this we don't hit APZ correctness target for 1.4.
blocking-b2g: --- → 1.4+
This is what I have so far, but it's completely untested. I'm a little concerned that we'll need to apply transforms to the hit regions from the layer before we can combine them using nsIntRegion::Or and nsIntRegion::And so that might be tricky.
Depends on: 977831
Depends on: 980544

Comment 12

5 years ago
PM Triage: remains 1.4+
Status update: in trying to implement this I ran into some problems for which I filed the above dependency bugs. Somebody from layout (probably roc and/or mattwoodrow) need to look into those before I can really continue working on this. There's a bit more I could do here but not sure it's worth doing unless those bugs are fixed because they might end up causing rework here.
We made this blocking at the time where it looked like the frequency of occurrence would be high - we're not actually seeing that, and the one issue we did find is not a blocker anymore.  While it's still important for this to be done, it doesn't look like a blocker anymore.
blocking-b2g: 1.4+ → ---
Comment on attachment 8382619 [details] [diff] [review]
Part 3 - Group things properly in header file

Moved this patch to bug 987314.
Attachment #8382619 - Attachment is obsolete: true
Not actively working on this; will pick it up again once the dependencies are sorted out.
Assignee: bugmail.mozilla → nobody
No longer blocks: 1021413
No longer blocks: 973096
Depends on: 928839
No longer depends on: 977831
No longer depends on: 980544
Depends on: 973105
Comment on attachment 8382617 [details] [diff] [review]
Part 1 - Enable creation of hit regions

This patch obsoleted by bug 1073024.
Attachment #8382617 - Attachment is obsolete: true
Comment on attachment 8382618 [details] [diff] [review]
Part 2 - Minor cleanup for code reuse

This patch obsoleted by bug 998025 which included this change.
Attachment #8382618 - Attachment is obsolete: true
Blocks: 1071941
Depends on: 1009786
Depends on: 1071686
No longer depends on: 1009786
Until bug 1082594 is fixed it's hard to get subframe scrolling working. This is a workaround that falls back to using the composition bounds from the scrollinfo layers to make subframes scroll.
Assignee: nobody → bugmail.mozilla
feature-b2g: --- → 2.2+
Depends on: 1083395
Attachment #8504757 - Attachment is obsolete: true
Depends on: 1090398
Comment on attachment 8507350 [details] [diff] [review]
Part 1 (WIP) - Store hit rects in APZC using EventRegions

This patch was moved to bug 1090398, same with the other two.
Attachment #8507350 - Attachment is obsolete: true
Attachment #8507351 - Attachment is obsolete: true
Attachment #8507352 - Attachment is obsolete: true
This is pretty much the simplest thing I could come up with that had a shot at working, but I haven't tested it yet.
Comment on attachment 8521501 [details] [diff] [review]
Part 2 (WIP) - Dispatch SetTargetAPZC from TabChild

Review of attachment 8521501 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +2394,5 @@
> +
> +      APZCCallbackHelper::GetOrCreateScrollIdentifiers(
> +        GetScrollableAncestor(targetElement),
> +        &(guid.mPresShellId),
> +        &(guid.mScrollId));

If |GetScrollableAncestor(targetElement)| returns null, should we set the scroll id to NULL_SCROLL_ID?
Refactoring to make part 2 cleaner.
Attachment #8521500 - Attachment is obsolete: true
Attachment #8523933 - Flags: review?(botond)
Attachment #8521501 - Attachment is obsolete: true
Attachment #8523934 - Flags: review?(botond)
This works in the scenarios I've tested (basic scrolling cases, and the "scrollbar" tests in the UI Tests app on B2G) but I need to test it more. Specifically I should test with more deeply nested scrollframes, because there will be entire handoff chains that need to be activated and I'm not sure if that's handled yet.
Attachment #8521502 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Specifically I should test with more deeply nested scrollframes, because
> there will be entire handoff chains that need to be activated and I'm not
> sure if that's handled yet.

So yes, this is a problem - if I disable using scrollinfo layers to pre-generate APZCs, and just use these patches to drive layer activation and APZC creation, then starting a scroll on a deeply nested scrollable element will cause handoff to fail. This happens because the ancestor scrollframes don't have displayports. I'll have to either walk the chain of ancestor scrollframes and set displayports on them all or come up with some other solution to this. It will probably affect bug 1009786 one way or the other. I discussed a bit with mstange on IRC but we need to think about this bit more.
Thoughts on doing something like this? The intention is to solve the problem described in comment 33.
Attachment #8524683 - Flags: review?(tnikkel)
Attachment #8524683 - Flags: review?(mstange)
Comment on attachment 8523934 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild

Found a bug in this part; if you tap outside of a scrollframe it doesn't find a target APZC at all and so the tap gets thrown away.
Attachment #8523934 - Flags: review?(botond)
Comment on attachment 8524683 [details] [diff] [review]
Part 5 - Layerize ancestor scrollframes of a layerized scrollframe

Review of attachment 8524683 [details] [diff] [review]:
-----------------------------------------------------------------

This is really nice, I like it.
Attachment #8524683 - Flags: review?(mstange) → review+
Updated part 2 to hit the RCD-RSF even if it has overflow:hidden.
Attachment #8523934 - Attachment is obsolete: true
The above part 2 patch still has a problem where if there is a scrollframe that is not actually scrollable (e.g. it is overflow:scroll but the contents fit inside the bounds, so the scroll range is 0) we will set a displayport on it anyway and force it to get layerized. One manifestation of this is in the B2G video app, when you scroll to the bottom and into overscroll some of the content will stretch differently from other content. There might be another bug here as well but we definitely shouldn't be triggering this behaviour. I'm working on an updated part 2 that solves this problem.
This version solves that problem too. I didn't find any other problems after playing around on B2G for a while so I think this is ready to get reviewed. r? to roc as well to make sure the code I'm using to find the scrollable ancestor is sane.
Attachment #8524832 - Attachment is obsolete: true
Attachment #8524897 - Flags: review?(roc)
Attachment #8524897 - Flags: review?(botond)
Attachment #8523935 - Flags: review?(roc)
Attachment #8523935 - Flags: review?(botond)
Attachment #8523936 - Flags: review?(botond)
Comment on attachment 8524683 [details] [diff] [review]
Part 5 - Layerize ancestor scrollframes of a layerized scrollframe

The subdocument changes are tricky because if after we call BuildDisplayListForStackingContext and we are told to make ourselves have a scrollable layer it's too late, we would have wanted to have called SetIgnoreScrollFrame before BuildDisplayListForStackingContext so that the root scroll frame went down the right path.

I think the best way to solve this is just to make root scroll frames use containerless scrolling first (without doing that I think it will just be too complicated and a waste of effort for when we get containerless scrolling).
Attachment #8524683 - Flags: review?(tnikkel)
Comment on attachment 8524897 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild

Review of attachment 8524897 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good other than that...

::: dom/ipc/TabChild.cpp
@@ +2379,5 @@
> +    // frame should be the one with the displayport set on it, so find that
> +    // element and return it.
> +    for (nsIContent* content = scrolledFrame->GetContent(); content; content = content->GetParent()) {
> +      if (content->IsElement()) {
> +        return content->AsElement();

I'm not sure why this loop is necessary. How can scrolledFrame->GetContent() not be an element?
Attachment #8524897 - Flags: review?(roc) → review-
(In reply to Timothy Nikkel (:tn) from comment #41)
> Comment on attachment 8524683 [details] [diff] [review]
> Part 5 - Layerize ancestor scrollframes of a layerized scrollframe
> 
> The subdocument changes are tricky because if after we call
> BuildDisplayListForStackingContext and we are told to make ourselves have a
> scrollable layer it's too late, we would have wanted to have called
> SetIgnoreScrollFrame before BuildDisplayListForStackingContext so that the
> root scroll frame went down the right path.

Thanks for pointing this out. I thought about it and realized that this problem occurs in the nsGfxScrollFrame code as well - for example if we have a non-scrollable scrollframe like I described in comment 38 (an overflow:scroll div whose scrollHeight <= clientHeight) then shouldBuildLayer will be false for it. However if that div has a child that is scrollable, then my patch would have activated the non-scrollable div as well which is not desirable. Instead we should just skip over any frames that are not "willing" to be layerized. I'm updating my patch to do this, it might resolve your concerns.
Updated part 2 to address roc's comment. I wasn't really sure if the scrollframe->GetContent might return a non-element so I figured the loop was safer, but I took it out and replaced it with an assert.
Attachment #8524897 - Attachment is obsolete: true
Attachment #8524897 - Flags: review?(botond)
Attachment #8525335 - Flags: review?(roc)
Attachment #8525335 - Flags: review?(botond)
Updated part 5 to skip over frames that aren't participating in scroll handoff. The patch doesn't have all the context I would like, but note that the MOZ_ASSERT conditions match the code at [1] and [2].

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=0b19236090ee#2966
[2] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=466d3ff030e6#476
Attachment #8524683 - Attachment is obsolete: true
Attachment #8525338 - Flags: review?(tnikkel)
Attachment #8525338 - Flags: review?(mstange)
Attachment #8525338 - Flags: review?(mstange) → review+
Attachment #8523933 - Flags: review?(botond) → review+
Comment on attachment 8525338 [details] [diff] [review]
Part 5 - Layerize ancestor scrollframes of a layerized scrollframe (v2)

So this patch would make us build a containerless scroll layer for a root scroll frame that was force activated. Then if it gets a displayport we would go back to a containerful scroll layer. Not sure if I like that. We'd have to make sure that containerless scrolling worked for this patch, so I'm not sure what this gets us without going to containerless scrolling.
Comment on attachment 8525335 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild (v2)

Review of attachment 8525335 [details] [diff] [review]:
-----------------------------------------------------------------

I'm concerned about the following case:

  - You have a parent scrollable element P, which is also zoomable.
  - You have a child scrollable element C inside P.
  - You pinch-zoom, with one finger being on C, and and one P but not C.

In this scenario, APZ hit testing hits C for the first touch point and P for the second, and the uses the common ancestor of these two in the APZC tree (which would be P) as the overall target APZC for the event.

I'm not sure what Gecko hit testing does with such a touch event: does it compute the targets of the two touch points independently, or does it also get the common ancestor and set it as the target of each touch point? If it's the former, then the GetScrollableAncestor() of the first touch point's target might be C, and we might end up re-targeting the touch block to C's APZC, which is bad.

Dropping the review flag until we clear this up.

::: layout/base/nsLayoutUtils.cpp
@@ +1839,5 @@
>         f->GetParent() : nsLayoutUtils::GetCrossDocParentFrame(f)) {
>      nsIScrollableFrame* scrollableFrame = do_QueryFrame(f);
>      if (scrollableFrame) {
> +      if (aFlags & SCROLLABLE_ONLY_ASYNC_SCROLLABLE) {
> +        // If the SCROLLABLE_ONLY_ASYNC_SCROLLABLE flag is set, then we only

I would prefer that these comments go with the definition of the enumerator. (As a bonus, we can take this opportunity to document the existing two enumerators, too!)

@@ +1858,5 @@
> +      // If the SCROLLABLE_ALWAYS_MATCH_ROOT flag is set, then return the
> +      // root scrollable frame for the root content document if we hit it.
> +      nsPresContext* pc = f->PresContext();
> +      if (pc->IsRootContentDocument() && pc->PresShell()->GetRootScrollFrame() == f) {
> +        return scrollableFrame;

I would imagine |scrollableFrame| won't be null in this case, but I think logically it still makes sense to have this inside the |if (scrollableFrame)| block.
Attachment #8525335 - Flags: review?(botond)
Comment on attachment 8525335 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild (v2)

Review of attachment 8525335 [details] [diff] [review]:
-----------------------------------------------------------------

I'm also going to defer to :roc on the question of GetScrollableAncestor() being symmetric to Gecko hit testing.

Specifically, I would expect the following:

  Let L be a scrollable layer with no dispatch-to-content region.
  Let E_L be the content element which owns the displayport of L.
  Let T be a touch event with one touch point.
  Suppose APZ hit testing of T hits L based on the hit regions of L and surrounding layers.
  Suppose Gecko hit testing of T hits the content element E_T.
  ==> Then GetScrollableAncestor(E_T) should return E_L.

but I can't convince myself that this is the case without much deeper knowledge of frame trees and Gecko hit testing (DispatchWidgetEvent).
Comment on attachment 8523935 [details] [diff] [review]
Part 3 - Activate scrollframes hit by touchstart

Review of attachment 8523935 [details] [diff] [review]:
-----------------------------------------------------------------

This generally looks good! I'd like to hear back about the calculating-a-displayport issue before r+'ing.

::: dom/ipc/TabChild.cpp
@@ +2438,5 @@
> +    // Once that takes effect (i.e. we get a notification from the RefreshDriver
> +    // that the refresh is done), then we send the notification.
> +    if (nsCOMPtr<nsIDOMWindowUtils> utils = APZCCallbackHelper::GetDOMWindowUtils(aElement)) {
> +      nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aElement);
> +      nsresult rv = utils->SetDisplayPortMarginsForElement(0, 0, 0, 0, element, 0);

Two comments on this:

  1) Should we try to calculate initial display port margins the way
     nsLayoutUtils::GetOrMaybeCreateDisplayPort() does? The advantage
     would be that if we get the margins right (which I think should 
     happen most of the time), we can avoid a repaint and possibly
     the allocation of a different-sized buffer when APZC calculates
     its displayport.

  2) It might be cleaner to call nsLayoutUtils::SetDisplayPortMargins()
     directly.

@@ +2441,5 @@
> +      nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aElement);
> +      nsresult rv = utils->SetDisplayPortMarginsForElement(0, 0, 0, 0, element, 0);
> +      if (rv == NS_OK) {
> +        nsCOMPtr<nsIDocument> doc = GetDocument();
> +        nsCOMPtr<nsIPresShell> presShell = (doc ? doc->GetShell() : nullptr);

nit: I'd rather we be consistent with using nested-ifs vs. conditionals for dealing with nulls.

@@ +2504,4 @@
>          &(guid.mPresShellId),
>          &(guid.mScrollId));
>      }
> +    EnsureDisplayportAndSendTargetAPZC(aInputBlockId, guid, guidIsValid, scrollAncestor);

nit: /* aTryToSetDisplayPort = */ guidIsValid

::: dom/ipc/TabChild.h
@@ +574,5 @@
>                                             const ScrollableLayerGuid& aGuid);
> +    void EnsureDisplayportAndSendTargetAPZC(const uint64_t& aInputBlockId,
> +                                            const ScrollableLayerGuid& aGuid,
> +                                            bool aTryToSetDisplayport,
> +                                            nsCOMPtr<dom::Element>& aElement);

nit: I find "SendTargetAPZC" unintuitive; it suggests we're sending an APZC. I'd suggest "SetTargetAPZC".
Comment on attachment 8523936 [details] [diff] [review]
Part 4 - Add some logging

Review of attachment 8523936 [details] [diff] [review]:
-----------------------------------------------------------------

This logging looks good.

I'd like to suggest one other piece of logging, easily greppable and enabled by default, at least until this feature stabilizes: whenever TabChild calls SetTargetAPZC with a guid that's different from the incoming guid. I think it'll be very useful to be able to see those specific cases - basically, the re-targeting cases - in the first few weeks of exercising this code.
Attachment #8523936 - Flags: review?(botond) → review+
Comment on attachment 8525335 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild (v2)

Dropping review as these patches will change somewhat based on botond's feedback so far.
Attachment #8525335 - Flags: review?(roc)
(In reply to Timothy Nikkel (:tn) from comment #46)
> Comment on attachment 8525338 [details] [diff] [review]
> Part 5 - Layerize ancestor scrollframes of a layerized scrollframe (v2)
> 
> So this patch would make us build a containerless scroll layer for a root
> scroll frame that was force activated. Then if it gets a displayport we
> would go back to a containerful scroll layer. Not sure if I like that. We'd
> have to make sure that containerless scrolling worked for this patch, so I'm
> not sure what this gets us without going to containerless scrolling.

I'm not sure I fully understand this (both the code and your explanation), but a more immediate question I have is: what would this patch look like after containerless scrolling? I'm assuming the patch on bug 1076192 is pretty close to the final thing, and as far as I can tell this patch will be more or less the same after that. Is that correct?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52)
> (In reply to Timothy Nikkel (:tn) from comment #46)
> > Comment on attachment 8525338 [details] [diff] [review]
> > Part 5 - Layerize ancestor scrollframes of a layerized scrollframe (v2)
> > 
> > So this patch would make us build a containerless scroll layer for a root
> > scroll frame that was force activated. Then if it gets a displayport we
> > would go back to a containerful scroll layer. Not sure if I like that. We'd
> > have to make sure that containerless scrolling worked for this patch, so I'm
> > not sure what this gets us without going to containerless scrolling.
> 
> I'm not sure I fully understand this (both the code and your explanation),
> but a more immediate question I have is: what would this patch look like
> after containerless scrolling? I'm assuming the patch on bug 1076192 is
> pretty close to the final thing, and as far as I can tell this patch will be
> more or less the same after that. Is that correct?

No, we wouldn't need the subdocument frame bits at all. Because the subdocument frame no longer would generate a scrollable layer. Subdocument frame generates scrollable layer for root scroll frame => containerful scrolling of root scroll frames. Containerless scrolling means that the scroll frame does all the scrollable layer work.
(In reply to Timothy Nikkel (:tn) from comment #53)
> No, we wouldn't need the subdocument frame bits at all. Because the
> subdocument frame no longer would generate a scrollable layer. Subdocument
> frame generates scrollable layer for root scroll frame => containerful
> scrolling of root scroll frames. Containerless scrolling means that the
> scroll frame does all the scrollable layer work.

Ah, gotcha. Would you be ok with me just landing the nsGfxScrollFrame bits then?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54)
> (In reply to Timothy Nikkel (:tn) from comment #53)
> > No, we wouldn't need the subdocument frame bits at all. Because the
> > subdocument frame no longer would generate a scrollable layer. Subdocument
> > frame generates scrollable layer for root scroll frame => containerful
> > scrolling of root scroll frames. Containerless scrolling means that the
> > scroll frame does all the scrollable layer work.
> 
> Ah, gotcha. Would you be ok with me just landing the nsGfxScrollFrame bits
> then?

If you just land the nsGfxScrollFrame bits then we will be effectively creating a containerless scroll layer for that root scroll frame. Which means that we will have both containerful and containerless scroll layers floating around at the same time. So we will have to both make containerless scroll layers work, and deal with the additional complexity (not sure how much) of having both kinds at the same time.

So ideally we should make containerless scrolling work. And then land your patches with just the nsGfxScrollFrame bits.
(In reply to Botond Ballo [:botond] from comment #47)
> I'm concerned about the following case:
> 
>   - You have a parent scrollable element P, which is also zoomable.
>   - You have a child scrollable element C inside P.
>   - You pinch-zoom, with one finger being on C, and and one P but not C.
> 
> In this scenario, APZ hit testing hits C for the first touch point and P for
> the second, and the uses the common ancestor of these two in the APZC tree
> (which would be P) as the overall target APZC for the event.
> 
> I'm not sure what Gecko hit testing does with such a touch event: does it
> compute the targets of the two touch points independently, or does it also
> get the common ancestor and set it as the target of each touch point? If
> it's the former, then the GetScrollableAncestor() of the first touch point's
> target might be C, and we might end up re-targeting the touch block to C's
> APZC, which is bad.
> 
> Dropping the review flag until we clear this up.

Yes, this is a point I had overlooked. I'm reworking the patch series to account for this. My plan is to make SetTargetAPZC take an array of ScrollableLayerGuids instead of just one, so that there is a target for each touch point. The APZCTM will then figure out the common ancestor APZC.

> > +        // If the SCROLLABLE_ONLY_ASYNC_SCROLLABLE flag is set, then we only
> 
> I would prefer that these comments go with the definition of the enumerator.
> (As a bonus, we can take this opportunity to document the existing two
> enumerators, too!)

Fair enough.

> @@ +1858,5 @@
> > +      // If the SCROLLABLE_ALWAYS_MATCH_ROOT flag is set, then return the
> > +      // root scrollable frame for the root content document if we hit it.
> > +      nsPresContext* pc = f->PresContext();
> > +      if (pc->IsRootContentDocument() && pc->PresShell()->GetRootScrollFrame() == f) {
> > +        return scrollableFrame;
> 
> I would imagine |scrollableFrame| won't be null in this case, but I think
> logically it still makes sense to have this inside the |if
> (scrollableFrame)| block.

I think that scrollableFrame can't be null, but logically it makes more sense to have it outside the if block. The way I'm thinking about the SCROLLABLE_ALWAYS_MATCH_ROOT flag is that if you hit the root scrollable frame for the root content document, then you return the nsIScrollableFrame that goes with that, even if it is null (which it can't be). A different way of writing this would be:

      if (pc->IsRootContentDocument() && pc->PresShell()->GetRootScrollFrame() == f) {
        return pc->PresShell()->GetRootScrollFrameAsScrollable();

which might make it clearer that it's "independent" of whether or not that returns null.

(In reply to Botond Ballo [:botond] from comment #49)
> Two comments on this:
> 
>   1) Should we try to calculate initial display port margins the way
>      nsLayoutUtils::GetOrMaybeCreateDisplayPort() does? The advantage
>      would be that if we get the margins right (which I think should 
>      happen most of the time), we can avoid a repaint and possibly
>      the allocation of a different-sized buffer when APZC calculates
>      its displayport.

That function requires a nsDisplayListBuilder which I probably can't have outside of a display-list build. It might be ok to extract the guts of that function but I'm not sure it makes sense to do. Consider the case where we have a deeply nested scrollframe that the user starts scrolling. Do we actually want to create "large" displayports for the enclosing scrollframes as soon as the user starts scrolling? The user may stop scrolling before triggering the scroll handoff up the chain, and go do something else. In that case we wasted a bunch of memory that we didn't really need.

>   2) It might be cleaner to call nsLayoutUtils::SetDisplayPortMargins()
>      directly.

I'd have to get a presShell which would make it less clean :(

> nit: I'd rather we be consistent with using nested-ifs vs. conditionals for
> dealing with nulls.
> 
> nit: /* aTryToSetDisplayPort = */ guidIsValid
> 
> nit: I find "SendTargetAPZC" unintuitive; it suggests we're sending an APZC.
> I'd suggest "SetTargetAPZC".

This code is changing in my new patches, but I'll make sure to fix these nits if they are still around.

(In reply to Botond Ballo [:botond] from comment #50)
> I'd like to suggest one other piece of logging, easily greppable and enabled
> by default, at least until this feature stabilizes: whenever TabChild calls
> SetTargetAPZC with a guid that's different from the incoming guid. I think
> it'll be very useful to be able to see those specific cases - basically, the
> re-targeting cases - in the first few weeks of exercising this code.

Good idea, thanks! I'll add that somewhere.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> > @@ +1858,5 @@
> > > +      // If the SCROLLABLE_ALWAYS_MATCH_ROOT flag is set, then return the
> > > +      // root scrollable frame for the root content document if we hit it.
> > > +      nsPresContext* pc = f->PresContext();
> > > +      if (pc->IsRootContentDocument() && pc->PresShell()->GetRootScrollFrame() == f) {
> > > +        return scrollableFrame;
> > 
> > I would imagine |scrollableFrame| won't be null in this case, but I think
> > logically it still makes sense to have this inside the |if
> > (scrollableFrame)| block.
> 
> I think that scrollableFrame can't be null, but logically it makes more
> sense to have it outside the if block. The way I'm thinking about the
> SCROLLABLE_ALWAYS_MATCH_ROOT flag is that if you hit the root scrollable
> frame for the root content document, then you return the nsIScrollableFrame
> that goes with that, even if it is null (which it can't be). A different way
> of writing this would be:
> 
>       if (pc->IsRootContentDocument() &&
> pc->PresShell()->GetRootScrollFrame() == f) {
>         return pc->PresShell()->GetRootScrollFrameAsScrollable();
> 
> which might make it clearer that it's "independent" of whether or not that
> returns null.

Ok. Maybe add a "shouldn't be null, but OK if it is" comment?

> (In reply to Botond Ballo [:botond] from comment #49)
> > Two comments on this:
> > 
> >   1) Should we try to calculate initial display port margins the way
> >      nsLayoutUtils::GetOrMaybeCreateDisplayPort() does? The advantage
> >      would be that if we get the margins right (which I think should 
> >      happen most of the time), we can avoid a repaint and possibly
> >      the allocation of a different-sized buffer when APZC calculates
> >      its displayport.
> 
> That function requires a nsDisplayListBuilder which I probably can't have
> outside of a display-list build. It might be ok to extract the guts of that
> function but I'm not sure it makes sense to do. 

Yeah, we don't need the parts of that function that depend on nsDisplayListBuilder. We only need the bits inside this if block [1], which can extracted out into a function.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=767ad1362a2d#2869

> Consider the case where we
> have a deeply nested scrollframe that the user starts scrolling. Do we
> actually want to create "large" displayports for the enclosing scrollframes
> as soon as the user starts scrolling? The user may stop scrolling before
> triggering the scroll handoff up the chain, and go do something else. In
> that case we wasted a bunch of memory that we didn't really need.

Who's talking about creating displayports for enclosing scrollframes? As far as I can tell, we're just creating one displayport for one scroll frame, the one that's being scrolled by this touch block.

> >   2) It might be cleaner to call nsLayoutUtils::SetDisplayPortMargins()
> >      directly.
> 
> I'd have to get a presShell which would make it less clean :(

Never mind then :)
(In reply to Botond Ballo [:botond] from comment #57)
> > Consider the case where we
> > have a deeply nested scrollframe that the user starts scrolling. Do we
> > actually want to create "large" displayports for the enclosing scrollframes
> > as soon as the user starts scrolling? The user may stop scrolling before
> > triggering the scroll handoff up the chain, and go do something else. In
> > that case we wasted a bunch of memory that we didn't really need.
> 
> Who's talking about creating displayports for enclosing scrollframes? As far
> as I can tell, we're just creating one displayport for one scroll frame, the
> one that's being scrolled by this touch block.

Markus pointed out that the Part 5 patch layerizes the enclosing scrollframes as well. However, as far as I can tell, it doesn't set displayports for them, so I believe my point stands.
(In reply to Botond Ballo [:botond] from comment #58)
> Markus pointed out that the Part 5 patch layerizes the enclosing
> scrollframes as well. However, as far as I can tell, it doesn't set
> displayports for them, so I believe my point stands.

Yup, you're right; I confused myself about what was going on. I'll make the change then.
The notable difference in this version is that now instead of using the target element from the input *after* it has been dispatched, I'm doing a hit-test directly using the event point and nsLayoutUtils *before* it has been dispatched. This is for two reasons:

1) After dispatching the event, the event may have some of its touch points removed. In particular, if non-primary touch points land in a different document than the primary touch point, they might get removed entirely [1], or retargeted to the <iframe> element of the primary touch point's document [2]. These behaviours are undesirable with respect to finding target APZCs when pinch-zooming. The removal of touch points I could have dealt with but the retargeting is not easy to work around without modifying existing behaviour or adding a whack of new code.

2) Doing the hit-test and dispatching SetTargetAPZC before the event is dispatched should result in (on average) sooner processing of the event on the APZ side, because it won't have to wait for touch listeners to run. The worst-case is still the same, but some of the intermediate cases are made strictly better.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=59f3a9bd26f0#7552
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=59f3a9bd26f0#7535
Attachment #8523933 - Attachment is obsolete: true
Attachment #8525335 - Attachment is obsolete: true
Attachment #8526205 - Flags: review?(roc)
Attachment #8526205 - Flags: review?(botond)
The idea here is the same as the last version of this patch but the implementation has shifted around a bunch to deal with the review comments and the new part 2.
Attachment #8523935 - Attachment is obsolete: true
Attachment #8523935 - Flags: review?(botond)
Attachment #8526207 - Flags: review?(botond)
Attachment #8523936 - Attachment is obsolete: true
Attachment #8525338 - Attachment is obsolete: true
Attachment #8525338 - Flags: review?(tnikkel)
Attachment #8526208 - Flags: review?(botond)
I'm going to move part 5 to a separate bug so that it doesn't hold up the rest of this stuff landing. It will only matter once the event-regions pref is turned on, and even then it doesn't get hit a lot in practice. I'd rather get these patches landed first and give them more bake time.

Try push with these 4 patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=07e593c82209
Depends on: 1102427
I moved part 5 to bug 1102427.
No longer depends on: apz-css-trans-stage2
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #61)
> 1) After dispatching the event, the event may have some of its touch points
> removed. In particular, if non-primary touch points land in a different
> document than the primary touch point, they might get removed entirely [1],
> or retargeted to the <iframe> element of the primary touch point's document
> [2]. These behaviours are undesirable with respect to finding target APZCs
> when pinch-zooming. The removal of touch points I could have dealt with but
> the retargeting is not easy to work around without modifying existing
> behaviour or adding a whack of new code.
> 
> 2) Doing the hit-test and dispatching SetTargetAPZC before the event is
> dispatched should result in (on average) sooner processing of the event on
> the APZ side, because it won't have to wait for touch listeners to run. The
> worst-case is still the same, but some of the intermediate cases are made
> strictly better.

Hmm I'm a little confused actually. If a touch listener does preventDefault, shouldn't we then not send SetTargetAPZC?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> Hmm I'm a little confused actually. If a touch listener does preventDefault,
> shouldn't we then not send SetTargetAPZC?

In that case we can still send the SetTargetAPZC. The APZ code won't actually process the input events until it also receives a ContentReceivedTouch notification. In the case that the touch-start is preventDefault'ed, that will happen at [1], and that notification will make the APZ drop those input events.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=cabc3b75da32#2401
Comment on attachment 8526199 [details] [diff] [review]
Part 1 - Update SetTargetAPZC API to take a list of targets

Review of attachment 8526199 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +405,5 @@
>    AsyncPanZoomController* FindTargetAPZC(AsyncPanZoomController* aApzc, const ScrollableLayerGuid& aGuid);
>    AsyncPanZoomController* GetAPZCAtPoint(AsyncPanZoomController* aApzc,
>                                           const gfx::Point& aHitTestPoint,
>                                           HitTestResult* aOutHitResult);
> +  already_AddRefed<AsyncPanZoomController> GetMultitouchTarget(AsyncPanZoomController* aApzc1, AsyncPanZoomController* aApzc2);

This method can be const. This will require making CommonAncestor() and RootAPZCForLayersId() const, too, but they should have been to begin with.
Attachment #8526199 - Flags: review?(botond) → review+
Comment on attachment 8526205 [details] [diff] [review]
Part 2 - Dispatch SetTargetAPZC from TabChild (v3)

Review of attachment 8526205 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel qualified to comment on the uses of GetEventCoordinatesRelativeTo and GetFrameForPoint, but I assume :roc evaluated those in his review.

::: dom/ipc/PBrowser.ipdl
@@ +354,5 @@
>      ContentReceivedTouch(ScrollableLayerGuid aGuid, uint64_t aInputBlockId, bool aPreventDefault);
>  
>      /**
> +     * Notifies the APZ code of the results of the gecko hit-test for a
> +     * particular input block.

add "Each target corresponds to one touch point."

::: dom/ipc/TabChild.cpp
@@ +2440,1 @@
>    nsEventStatus status = DispatchWidgetEvent(localEvent);

Can we add a comment to this line saying that this is line that does Gecko hit-testing? It's easy to miss that this line does an important / time-consuming operation.

::: layout/base/nsLayoutUtils.h
@@ +543,4 @@
>      SCROLLABLE_SAME_DOC = 0x01,
> +    /**
> +     * If the SCROLLABLE_INCLUDE_HIDDEN flag is set then we allow
> +     * overflow:hidden scrollframes to be retruned as scrollable frames.

typo
Attachment #8526205 - Flags: review?(botond) → review+
Comment on attachment 8526207 [details] [diff] [review]
Part 3 - Activate scrollframes hit by touchstart (v2)

Review of attachment 8526207 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +2466,5 @@
>      targets.AppendElement(guid);
> +
> +    if (guidIsValid && !nsLayoutUtils::GetDisplayPort(dpElement, nullptr)) {
> +      waitForRefresh |= nsLayoutUtils::CalculateAndSetDisplayPort(
> +        scrollAncestor, nsLayoutUtils::RepaintMode::Repaint);

Say you have a page with an iframe, and you pinch-zoom a page in such a way that both fingers are on the iframe.

IIUC, this will layerize and set a displayport for the iframe unnecessarily.

(Or, if you have two iframes, with one finger on either, they will both be layerized and have displayports drawn unnecessarily.)

Can we avoid this by having some common-ancestor logic here, and only setting one displayport?

::: layout/base/nsLayoutUtils.cpp
@@ +2792,3 @@
>  static FrameMetrics
> +CalculateFrameMetricsForDisplayPort(nsIScrollableFrame* aScrollFrame) {
> +  nsIFrame* frame = do_QueryFrame(aScrollFrame);

We already have the nsIFrame at the call site. Why do an extra do_QueryFrame here?

::: layout/base/nsLayoutUtils.h
@@ +2406,5 @@
> +   * mode provided is passed through to the call to SetDisplayPortMargins.
> +   * The |aScrollFrame| parameter must be non-null and queryable to an nsIFrame.
> +   * @return true iff the call to SetDisplayPortMargins returned true.
> +   */
> +  static bool CalculateAndSetDisplayPort(nsIScrollableFrame* aScrollFrame,

As this function sets the display port margins but not the base, I would prefer it be called CalculateAndSetDisplayPortMargins.
Comment on attachment 8526208 [details] [diff] [review]
Part 4 - Add some logging

Review of attachment 8526208 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +42,5 @@
> +    return true;
> +  }
> +
> +  TBS_LOG("%p replacing unconfirmed target %p with real target %p\n",
> +      this, mTargetApzc.get(), aTargetApzc.get());

This log statement doesn't meet the "enabled by default" criterion I requested in comment 50.

r+ with this fixed
Attachment #8526208 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #70)
> Say you have a page with an iframe, and you pinch-zoom a page in such a way
> that both fingers are on the iframe.
> 
> IIUC, this will layerize and set a displayport for the iframe unnecessarily.
> 
> (Or, if you have two iframes, with one finger on either, they will both be
> layerized and have displayports drawn unnecessarily.)
> 
> Can we avoid this by having some common-ancestor logic here, and only
> setting one displayport?

So generally when you do a pinch, the code will get a touchstart with one touch point, followed by a touchstart with two touch points. At the time we get the first touchstart, we don't know if the user is going to do a pinch so we have to set a displayport on the iframe regardless. So the second touchstart event will incur no extra work (assuming both fingers land on the same iframe). The only case where we do unnecessary work is when both fingers land on separate things that both didn't have displayports. This is going to be unlikely in practice.

In addition, given the design I chose in these patches, the APZCTM needs to be able to convert the ScrollableLayerGuid into a APZC instance, and in order to do that there needs to be a layer with that guid. In order to have the layer we need to set a displayport. I don't see any alternate way to accomplish this without radically changing the design (which I'm open to, but I don't have any ideas for what to do).

Finally I feel like putting common-ancestor logic here would violate the single-source-of-truth principle as we would be duplicating code from APZCTM here, and always assuming that multiple touch points are going to trigger a pinch-zoom behaviour. That may not be the case if, for example, the second touchstart event is prevent-defaulted.

> >  static FrameMetrics
> > +CalculateFrameMetricsForDisplayPort(nsIScrollableFrame* aScrollFrame) {
> > +  nsIFrame* frame = do_QueryFrame(aScrollFrame);
> 
> We already have the nsIFrame at the call site. Why do an extra do_QueryFrame
> here?

Ah, whoops. That got left from one of my intermediate states while working on this. That being said, it felt weird to me to be passing around two pointers to the same object. It's a static method so it's not exposed but still, what happens if somebody accidentally passes in pointers to different objects? The function assumes they are the same so it might be better to enforce that. I don't feel strongly about this though, I can change it back if you prefer.

> ::: layout/base/nsLayoutUtils.h
> > +  static bool CalculateAndSetDisplayPort(nsIScrollableFrame* aScrollFrame,
> 
> As this function sets the display port margins but not the base, I would
> prefer it be called CalculateAndSetDisplayPortMargins.

Sure, I'll change this.
(In reply to Botond Ballo [:botond] from comment #71)
> This log statement doesn't meet the "enabled by default" criterion I
> requested in comment 50.
> 
> r+ with this fixed

Ah I missed that, will fix it up.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #72)
> In addition, given the design I chose in these patches, the APZCTM needs to
> be able to convert the ScrollableLayerGuid into a APZC instance, and in
> order to do that there needs to be a layer with that guid. In order to have
> the layer we need to set a displayport. I don't see any alternate way to
> accomplish this without radically changing the design (which I'm open to,
> but I don't have any ideas for what to do).

If we did the common-ancestor logic on the content side, there would be no reason to send multiple targets to APZC - we could continue send one target (the result of the common-ancestor logic), and we would layerize that target so it would have an APZC.

> Finally I feel like putting common-ancestor logic here would violate the
> single-source-of-truth principle as we would be duplicating code from APZCTM
> here, and always assuming that multiple touch points are going to trigger a
> pinch-zoom behaviour. That may not be the case if, for example, the second
> touchstart event is prevent-defaulted.

If the second touchstart event is prevent-defaulted, wouldn't we discard the entire pinch, because its touch-moves would go into the same touch block as that second touchstart?

That said, I agree that there's value in having a single source of truth. I'm OK with going with this approach as long as we're aware of / have evaluated the alternative.

> > >  static FrameMetrics
> > > +CalculateFrameMetricsForDisplayPort(nsIScrollableFrame* aScrollFrame) {
> > > +  nsIFrame* frame = do_QueryFrame(aScrollFrame);
> > 
> > We already have the nsIFrame at the call site. Why do an extra do_QueryFrame
> > here?
> 
> Ah, whoops. That got left from one of my intermediate states while working
> on this. That being said, it felt weird to me to be passing around two
> pointers to the same object. It's a static method so it's not exposed but
> still, what happens if somebody accidentally passes in pointers to different
> objects? The function assumes they are the same so it might be better to
> enforce that. I don't feel strongly about this though, I can change it back
> if you prefer.

Nah, you convinced me this is better :)
(In reply to Botond Ballo [:botond] from comment #74)
> If we did the common-ancestor logic on the content side, there would be no
> reason to send multiple targets to APZC - we could continue send one target
> (the result of the common-ancestor logic), and we would layerize that target
> so it would have an APZC.

This is true.

> > Finally I feel like putting common-ancestor logic here would violate the
> > single-source-of-truth principle as we would be duplicating code from APZCTM
> > here, and always assuming that multiple touch points are going to trigger a
> > pinch-zoom behaviour. That may not be the case if, for example, the second
> > touchstart event is prevent-defaulted.
> 
> If the second touchstart event is prevent-defaulted, wouldn't we discard the
> entire pinch, because its touch-moves would go into the same touch block as
> that second touchstart?

Also true, given our current behaviour. I think other browsers treat this case differently though - they will allow the first finger to keep panning and prevent the second finger from turning it into a pinch. I'm not saying that we're going to support that but I feel like this design gives us a bit more flexibility if we were to try (and yes, we would need nontrivial other changes as well). There's also the open question about interaction with touch-action properties, and I feel like having extra flexibility in the design at this point will help us when implementing that.

> That said, I agree that there's value in having a single source of truth.
> I'm OK with going with this approach as long as we're aware of / have
> evaluated the alternative.

Ok, I'd like to stick with this for now. If it turns out that we hit this scenario often we can revisit it.

> > Ah, whoops. That got left from one of my intermediate states while working
> > on this. That being said, it felt weird to me to be passing around two
> > pointers to the same object. It's a static method so it's not exposed but
> > still, what happens if somebody accidentally passes in pointers to different
> > objects? The function assumes they are the same so it might be better to
> > enforce that. I don't feel strongly about this though, I can change it back
> > if you prefer.
> 
> Nah, you convinced me this is better :)

Ok, will leave it.
Attachment #8526207 - Flags: review?(botond) → review+
Depends on: 1104282
Depends on: 1109873
You need to log in before you can comment on or make changes to this bug.