Closed Bug 980544 Opened 10 years ago Closed 10 years ago

Add hitregion and dispatchtocontentregion to scrollinfo layer instances

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: kats, Assigned: tnikkel)

References

Details

I was thinking about how to go about using the hitregion and dispatchtocontentregion for hit-testing in the APZC code and ran into some problematic cases when I discussed this with botond and tn. Currently the regions are put on each layer. So for example, given a layer tree like this:

- ContainerLayer A
  - ThebesLayer B
  - ContainerLayer C
    - ThebesLayer D
    - ThebesLayer E
  - ThebesLayer F

the individual ThebesLayer instance may have regions set on them. If there is an APZC instance attached to layer C, it has to combine the regions from layers D and E to get the hit-test region needed for that APZC.

This gets more complicated if layer A also has an APZC attached, because now we want the regions from F - C + B for that layer (i.e. because of z-ordering we have to subtract regions as well).

However the real kicker is if we have a scroll info layer, because the content for that layer will be part of a ThebesLayer somewhere else in the tree. If the hit test data is not on the ContainerLayer that is the scrollinfo layer, it becomes impossible to send the input events for that scroll info layer to the APZC attached to it because we don't know what the hit region is.

Is it possible for FrameLayerBuilder to combine these regions and just put them on ContainerLayer instances (ideally just on ContainerLayer instances with scrollable metrics)?
Flags: needinfo?(roc)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> However the real kicker is if we have a scroll info layer, because the
> content for that layer will be part of a ThebesLayer somewhere else in the
> tree. If the hit test data is not on the ContainerLayer that is the
> scrollinfo layer, it becomes impossible to send the input events for that
> scroll info layer to the APZC attached to it because we don't know what the
> hit region is.

I think we can fix that by putting hit-test data on the scrollinfo layer.

We can't just put hit-test data on ContainerLayers. In your example, we need to be able to distinguish the content of A that covers C (F) from the content that doesn't (B).
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> We can't just put hit-test data on ContainerLayers. In your example, we need
> to be able to distinguish the content of A that covers C (F) from the
> content that doesn't (B).

Right, but this could be done in FrameLayerBuilder instead of the APZCTreeManager, right? I'm not sure which place is better.

We can repurpose this bug to just put hit-test data on the scrollinfo layer if that's the way you'd like to tackle this.
Flags: needinfo?(roc)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> > We can't just put hit-test data on ContainerLayers. In your example, we need
> > to be able to distinguish the content of A that covers C (F) from the
> > content that doesn't (B).
> 
> Right, but this could be done in FrameLayerBuilder instead of the
> APZCTreeManager, right? I'm not sure which place is better.

I don't think it can, if C is subject to async scrolling or an OMTAnimated transform.

> We can repurpose this bug to just put hit-test data on the scrollinfo layer
> if that's the way you'd like to tackle this.

Yes :-)
Flags: needinfo?(roc)
Summary: Move hitregion and dispatchtocontentregion to ContainerLayer instances → Add hitregion and dispatchtocontentregion to scrollinfo layer instances
Rob, I'm going to assign this to you so that we don't leave a 1.4+ unowned, and you seem to be up to speed as to what is going on.  If you're not the right person, can you reassign?
Assignee: nobody → roc
blocking-b2g: --- → 1.4+
Bug 918288 no longer 1.4 blocker.  Let's talk about this and related bugs during the work week.
blocking-b2g: 1.4+ → ---
If you can take a look at this along with bug 977831 that would be great.
Assignee: roc → tnikkel
Hmm, this gets more tricky when the scroll frame that is going to get a scroll info layer contains display items that will create container layers, ie scrollable layers, transform layers, etc. We'll have to figure out exactly how to handle that.
So what are next steps on this bug? Do you need to discuss this with roc and/or somebody else?
Flags: needinfo?(tnikkel)
Yeah, I'd like to hear if roc has any thoughts.
Flags: needinfo?(tnikkel) → needinfo?(roc)
How about, if a scrollframe has any children which will generate active layers, we make it active as well? That would simplify things.
Flags: needinfo?(roc)
Blocks: 928833
No longer blocks: 918288
Based on a discussion with tn this bug should basically go away once multi-layer-apz is done because we won't have scrollinfo layers anymore. Please correct me if I'm mistaken.
Assignee: tnikkel → nobody
Depends on: multi-layer-apz
At some point roc said that scrollinfo layers are basically going to remain unchanged so this is probably still going to be an issue.
Assignee: nobody → tnikkel
No longer depends on: multi-layer-apz
roc: APZC needs ScrollInfo layers so that touch events in content belonging to those layers trigger scrolling of the inactive scrollframe, instead of being consumed by ancestor scrollable elements that happen to be active. Right? does it need ScrollInfo layers for anything else?
tn: i dont _think_ so
roc: so we don't actually need scrollinfo layers at all, then, when we have event regions working properly. ThebesLayers with content belonging to inactive scrollframes should just add the region of that content to their mDispatchToContentHitRegion.

So I think this bug should be about making sure the scrollports of inactive scrollframes are added to their ThebesLayer's mDispatchToContentHitRegion (probably via a dedicated display item that covers the scrollport and adds itself to mDispatchToContentHitRegion), and when layer hitregions are enabled, we disable scrollinfo layers altogether. That looks like a very elegant solution.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> roc: APZC needs ScrollInfo layers so that touch events in content belonging
> to those layers trigger scrolling of the inactive scrollframe, instead of
> being consumed by ancestor scrollable elements that happen to be active.
> Right? does it need ScrollInfo layers for anything else?
> tn: i dont _think_ so
> roc: so we don't actually need scrollinfo layers at all, then, when we have
> event regions working properly. ThebesLayers with content belonging to
> inactive scrollframes should just add the region of that content to their
> mDispatchToContentHitRegion.
> 
> So I think this bug should be about making sure the scrollports of inactive
> scrollframes are added to their ThebesLayer's mDispatchToContentHitRegion
> (probably via a dedicated display item that covers the scrollport and adds
> itself to mDispatchToContentHitRegion), and when layer hitregions are
> enabled, we disable scrollinfo layers altogether. That looks like a very
> elegant solution.

Will such thebes layers get a FrameMetrics?
No, not for the inactive scroll frame. The thebeslayer would contain content from the inactive scroll frame as well as content from outside it. Is that a problem?
When the compositor gets a touch event in an inactive scrollable element, the compositor will walk the layer tree from front to back to find which layer has the event in its mHitRegion. If the event is also in that layer's mDispatchToContentHitRegion (which it will be, when the event targets an inactive scrollable element), dispatch the event to the main thread and do nothing else (don't create an APZC for example). Then in the main thread we'll handle the event, make the scrollable element active, do a layer transaction that creates scrollable layers for it, and send the results back to the compositor. I guess we'll also have to tell the compositor to start a scroll gesture for the new scrollable layer(s) using this event.

Of course this means you can't start a scroll gesture for the inactive scrollable element until you've done a round-trip to the main thread. But that's already the case since you can't async scroll an element until the main thread has created layers for it! This is simply the tradeoff we make when we make scrollable elements inactive.
On IRC there was some discussion of why don't we just attach FrameMetrics for inactive scrollframes to ThebesLayers.

That won't always be possible. For example a ThebesLayer might contain an inactive scrollframe wrapped in an inactive transform (e.g. a rotation); FrameMetrics can't express that. Or, the inactive scrollframe might be partially covered by some non-scrolling content; in that case the compositor thread won't be able to tell whether an event actually hit the scrollframe or not. Or, there might be multiple overlapping inactive scrollframes, in which case we'd have to order the FrameMetrics ... but the content of those inactive scrollframes might actually interleave so there's no total order. Etc.

We could add additional metadata to handle more of those cases, but we already know that in the limit we will have to support APZC delegating to the main thread to decide whether a touch-start will start a scroll gesture in an inactive scroll frame (and if so, what will be scrolled). So I think we should cut over to that approach now.
Closed in favor of bug 1082594, which encapsulates the decisions we made in this bug.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.