912666 - Add support for scroll grabbing

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

As part of the dynamic toolbar implementation for B2G (bug 889883), we need to introduce some special handling of browser iframe elements by APZC(TM). Specifically:

       a. When deciding on the initial target of a scroll, we currently use 
          the APZC for innermost scrollable layer that passes the hit test.
          We'll need to change this so that, if there is a browser iframe
          element on the path from the root scrollable layer to the
          innermost one, the initial target is the APZC for the innermost 
          scrollable layer *outside* of the browser iframe. This will ensure 
          that the toolbar will be scrolled on- or off-screen before 
          anything else.

       b. When deciding which APZC to hand off scrolling to (bug 898478),
          if the initial APZC was the one just outside the browser iframe
          (as per (aa)), the next one should be the one for the innermost
          scrollable layer inside the browser iframe.

In order to do this, APZC needs to be able to tell whether or not its layer corresponds to a browser iframe element.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> In order to do this, APZC needs to be able to tell whether or not its layer
> corresponds to a browser iframe element.

Here is my proposal for how to do this:

  - Add a new field isForBrowserIframe to FrameMetrics.
  - In RecordFrameMetrics, set isForBrowserIframe if aScrollFrame->GetContent() is
    both mozbrowser=true and remote=true. Just mozbrowser=true isn't enough because
    app iframes also have this attribute set.
  - In APZC, treat a layer as corresponding to a browser iframe if its frame
    metrics has isForBrowserIframe = true.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)
> (In reply to Botond Ballo [:botond] from comment #0)
> > In order to do this, APZC needs to be able to tell whether or not its layer
> > corresponds to a browser iframe element.
> 
> Here is my proposal for how to do this:
> 
>   - Add a new field isForBrowserIframe to FrameMetrics.
>   - In RecordFrameMetrics, set isForBrowserIframe if
> aScrollFrame->GetContent() is
>     both mozbrowser=true and remote=true. Just mozbrowser=true isn't enough
> because
>     app iframes also have this attribute set.
>   - In APZC, treat a layer as corresponding to a browser iframe if its frame
>     metrics has isForBrowserIframe = true.

This doesn't quite work because a browser iframe doesn't have its own APZC. tn and I discussed this today in #layout, and came up with the following revised strategy:

  (1) In nsDisplayRemote::BuildLayer(), check whether the nsDisplayRemote's
      mFrame is a browser iframe (as described above). If so, set a flag
      on the layer object that gets built.
  (2) When constructing an APZC, traverse the layer subtree rooted at the
      APZC's layer to see if a layer marked in (1) is present. If so,
      mark the APZC as containing a browser iframe.

It might be good to constrain the traversal done in (2) so that we don't walk a large subtree unnecessarily if we know if can't contain a browser iframe. I'm not sure to what extent this can be constrained.

Comment 3

[Botond Ballo [:botond]]

>   (2) When constructing an APZC, traverse the layer subtree rooted at the
>       APZC's layer to see if a layer marked in (1) is present. If so,
>       mark the APZC as containing a browser iframe.
> 
> It might be good to constrain the traversal done in (2) so that we don't
> walk a large subtree unnecessarily if we know if can't contain a browser
> iframe. I'm not sure to what extent this can be constrained.

In fact this traversal is not necessary. We already traverse the layer tree in APZCTreeManager::UpdatePanZoomControllerTree(), we can use that same traversal to mark APZCs containing a browser iframe.

Comment 4

[Botond Ballo [:botond]]

Attachment: Part 1 - Identify APZCs that contain a browser iframe

This patch implements identification of APZCs that contain browser iframes (directly, i.e. not within a child APZC).

The remaining work for this bug is to use this identification to implement the special cases (a) and (b) in comment #0.

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 1 - Identify APZCs that contain a browser iframe

Updated Part 1 patch to add a getter for AsyncPanZoomController::mContainsBrowserIframe.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 805487 [details] [diff] [review]
Part 1 - Identify APZCs that contain a browser iframe

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

Looks good to me overall but somebody from gfx will also need to review because I don't know if adding an extra attribute like this is ok or not.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +172,5 @@
> +    // If this layer is for a browser iframe, mark the nearest enclosing
> +    // APZC, if any, as containing a browser iframe.
> +    if (aParent && container->AsRefLayer() && container->AsRefLayer()->IsForBrowserIframe()) {
> +      aParent->SetContainsBrowserIframe();
> +    }

As discussed I believe this code is in the wrong place in this function. I think it belongs at the very very top of this function. Otherwise it might end up setting the flag on the APZC corresponding to the ref layer itself, because aParent could be set to apzc a few lines up in this function.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +837,5 @@
>  }
>  
> +void AsyncPanZoomController::SetContainsBrowserIframe() {
> +  mContainsBrowserIframe = true;
> +}

Can we just throw this in the .h file? Seems inconsistent to have the getter in the .h file but not the setter.

::: layout/ipc/RenderFrameParent.cpp
@@ +1081,5 @@
> +  // Check whether this is a browser iframe, and if so annotate the layer
> +  // appropriate. This annotation will be looked at by the APZC to provide
> +  // special scrolling behaviour for this layer.
> +  if (layer->AsRefLayer() && mFrame) {
> +    if (mFrame) {

Second if is redundant

@@ +1085,5 @@
> +    if (mFrame) {
> +      nsIContent* content = mFrame->GetContent();
> +      if (content
> +       && content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, nsGkAtoms::_true, eCaseMatters)
> +       && content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, nsGkAtoms::_true, eCaseMatters)) {

Indent the && lines one more space. Or three more if you don't want it to line up with the body of the condition

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 1 - Identify APZCs that contain a browser iframe

Updated patch to address kats' review comments.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> ::: gfx/layers/composite/APZCTreeManager.cpp
> @@ +172,5 @@
> > +    // If this layer is for a browser iframe, mark the nearest enclosing
> > +    // APZC, if any, as containing a browser iframe.
> > +    if (aParent && container->AsRefLayer() && container->AsRefLayer()->IsForBrowserIframe()) {
> > +      aParent->SetContainsBrowserIframe();
> > +    }
> 
> As discussed I believe this code is in the wrong place in this function. I
> think it belongs at the very very top of this function. Otherwise it might
> end up setting the flag on the APZC corresponding to the ref layer itself,
> because aParent could be set to apzc a few lines up in this function.

I don't think that can actually happen, as setting aParent to apzc only happens if the browser iframe's layer is scrollable, and that shouldn't happen (I added an assertion to be sure). Nevertheless, I think it's clearer to have this code in the place you suggest, so I moved it.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 806253 [details] [diff] [review]
Part 1 - Identify APZCs that contain a browser iframe

I'm withdrawing this patch from review because there are a number of issues with this approach:

  - The patch marks any APZC that contains an iframe with mozbrowser=true
    and remote=true. This is too general - other apps can use such iframes
    but may not want special scrolling behaviour.

  - There aren't many guarantees about the structure of the layer tree
    that is built for an overflow:scroll div containing a browser iframe.
    In particular, the scrollable layer representing the div may be a
    sibling (possibly removed) of the scrollable layer representing the
    iframe, rather than its ancestor.

After discussing this with kats and BenWa, here's an updated proposal:

  - Introduce a new attribute called 'mozscrollgrab' which the browser
    app code would set on the scrollable div. The special scroll
    behaviour would be restricted to divs with this attribute. The
    idea is for this to be an "unofficial" API for now, until we are
    confident that this is the approach we want to take; once that
    happens we can consider making it public.

  - Mark the APZC corresponding to the 'mozscrollgrab' div as being
    special. We should be able to do this in RecordFrameMetrics()
    using the approach outlined in comment #1.

  - Provide special handling of this div as outlined in comment #0.
    Note that there no (longer) a need for APZC to know about the
    browser iframe.

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
>   - Introduce a new attribute called 'mozscrollgrab' which the browser
>     app code would set on the scrollable div. The special scroll
>     behaviour would be restricted to divs with this attribute. The
>     idea is for this to be an "unofficial" API for now, until we are
>     confident that this is the approach we want to take; once that
>     happens we can consider making it public.

I posted on dev-platform to ask for feedback on this. So far there
have been suggestions to make this a CSS property rather than an 
HTML attribute, to make it available to content (and not prefix it
with moz-), and to propose it at www-style.

While we figure out the exact course of action, I will write a
prototype implementation using an HTML attribute named 'mozscrollgrab',
to verify that this approach is viable to begin with.

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'mozscrollgrab' attribute

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 2 - Propagate the 'mozscrollgrab' attribute to FrameMetrics

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 3 - Implement scroll grabbing in APZC

Comment 13

[Botond Ballo [:botond]]

Posted WIP patches for my prototype implementation. They are not working yet - right now they cause pages to not be scrollable at all.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Does this need to be aware of the work in bug 919692?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

I don't think bug 919692 is related.

Comment 16

[Botond Ballo [:botond]]

While implementing the approach in comment #8, I ran into an issue where the structure of the layer tree (and correspondingly of the APZC tree) sometimes isn't conducive to changing hit testing in the way I described.

For the following DOM fragment:

<div style="overflow: scroll">
    <iframe remote="true" mozbrowser="true">
        <html>
           ...
        </html>
    </iframe>
</div>

I initially see the following layer tree structure (with siblings listed from back to front, and thebes layers omitted):

ContainerLayer (non-scrollable)
  ContainerLayer (scroll info layer, corresponding to the <div>)
  RefLayer (for the remote iframe)
    ContainerLayer (scrollable, corresponding to the <html> inside the <iframe>)

Let's call this tree (1).

Then after a while, it turns into:

ContainerLayer (non-scrollable)
  ContainerLayer (scrollable, corresponding to the <div>)
    RefLayer (for the remote iframe)
      ContainerLayer (scrollable, corresponding to the <html> inside the <iframe>)

Let's call this tree (2).

Note the two differences:
  - The scroll info layer has turned into a real scrollable layer.
  - The RefLayer has become a child of this scrollable layer, while it was a sibling of the scroll info layer.

To do hit testing in the APZC, we walk siblings layers from front to back, and return the first one that matches (recursing into it if it has children).

Tree (2) is thus suitable for implementing scroll-grabbing, since we'll encounter the <div> before we encounter the <html>, but tree (1) is not, because we encounter the <html> first.

To resolve this, I would like to modify the layer building code so that the tree (1) instead looks like so:

ContainerLayer (non-scrollable)
  ContainerLayer (scroll info layer, corresponding to the <div>)
    RefLayer (for the remote iframe)
      ContainerLayer (scrollable, corresponding to the <html> inside the <iframe>)

i.e. the scrollable layer corresponding to the <div> is still a scroll info layer, but now it has the RefLayer as its child just like it will when it turns into a real scrollable layer, rather than as its sibling.

More generally, I would like to modify the handling of scroll info layers so that they are located in the layer tree in the same place where they would be located if they would be a real scrollable layer.

Matt, what do you think about this? Is this doable?

Comment 17

[Matt Woodrow (:mattwoodrow)]

tl;dr: Although it looks correct for this particular case, I don't think it's a correct change for the general case.

I didn't write the ScrollInfoLayer stuff, but I'll try explain all that I know :)

In layout we mark each scrollable container (represented by nsGfxScrollFrame) as either active or inactive, with active meaning that we think the scroll position is likely to change in the near future and we should try optimize for that. I think we mark the root scroll container as always active, and other ones get marked active when they are scrolled for the first time, and transition back to inactive after a timeout.

We also define the 'active scrolled root' for every display item as the first active scroll frame ancestor, or the root viewport otherwise.

When building ThebesLayers from a set of display items, we separate items based on their active scrolled root, and create separate ThebesLayers for each set. This separates scrolled and fixed content into different layers, and lets us do minimal invalidation and repainting during scrolling. For content that isn't being scrolled currently, then flattening it into it's surrounding content is an important optimization to reduce the amount of memory used, chances of getting component alpha etc.

ScrollInfoLayer exists for the case where we have content that could be scrolled (but is currently inactive) is occupying the same layer as content that doesn't scroll (for the given scroll container). The ScrollInfoLayer provides extra information to APZC so that it knows that touches within that area can't be handled asynchronously, and we need to tell layout to scroll instead (and probably mark the scroll container as active for any following scrolls).

In this example there would be no single layer, or layer subtree which could be wrapped within the ScrollInfoLayer. In your example, the only content that exists is all scrollable, so it works.

Comment 18

[Botond Ballo [:botond]]

The new Core -> Panning and Zooming component was lonely, so I thought I'd give it its inaugural bug.

Comment 19

[Botond Ballo [:botond]]

I tried to come up with a modified proposal for the placement of scroll info layers in the layer tree that is both suitable for implementing scroll grabbing, and addresses mattwoodrow's concerns, but roc pointed out that any attempt to to give scroll info layers children will disturb internal invariants of the display list building and/or layer building code, and suggested instead that we simply force the creation of a real scrollable layer (not scroll info layer) for scroll frames representing scroll grabbing elements. I will try that.

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'mozscrollgrab' attribute

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 2 - Propagate the 'mozscrollgrab' attribute to FrameMetrics

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement scroll grabbing in APZ

Comment 24

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #19)
> roc pointed out that any
> attempt to to give scroll info layers children will disturb internal
> invariants of the display list building and/or layer building code, and
> suggested instead that we simply force the creation of a real scrollable
> layer (not scroll info layer) for scroll frames representing scroll grabbing
> elements. I will try that.

The updated patches that I've posted implement roc's suggestion, and complete the prototype scrollgrabbing implementation in the APZC.

With these patches + the patch in bug 912657 + the Gaia patches in bug 835152, the dynamic toolbar now mostly works as intended on simple pages. There are a number of things left to do before these patches are ready for review:

  1) The patches cause some regressions that affect the usability of the browser. I will document these in follow-up comments and file bugs for them as appropriate, and fix them.
  2) The patches need a bit of cleanup and documentation.
  3) We need to decide whether we want to land 'scrollgrab' as an HTML attribute or a CSS property (and if the latter, modify the implementation accordingly), and whether we want to expose it to content or not.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
>   1) The patches cause some regressions that affect the usability of the
> browser. I will document these in follow-up comments and file bugs for them
> as appropriate, and fix them.

Here are some of the things I see

consistently:
  1) When scrolling the toolbar back into view, the portion that is being 
     scrolled into view shows up as a black bar, and is only repainted
     after I stop scrolling. This suggests that either there is no
     displayport being set on the outer scrollable <div>, or one is being
     set but it's too small.
  2) Turning the screen into landscape orientation causes things to be
     really messed up.

intermittently:
  3) About half of the time, after scrolling the toolbar out of view, and
     lifting your finger, a further downward scroll is ignored, and you have
     to scroll up (bringing the toolbar back into view) before you can
     scroll down again.
  4) Occasionally, after scrolling around for a while, the content iframe
     becomes entirely black.

I will look into each of these issues.

Thankfully, an issue I was seeing before where scrolling was sometimes jumpy because some TOUCH_END events were missed and thus some TOUCH_MOVE events were interpreted as being part of the previous pan, is now gone.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

As per our discussion last week, I think some of these issues are probably caused by not having proper APZC support in the root process.

Comment 27

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> As per our discussion last week, I think some of these issues are probably
> caused by not having proper APZC support in the root process.

I thought that was only the case for the issue (not mentioned above) where if you pan _on_ the toolbar it doesn't redraw properly. Which of the other issues are likely to be caused by improper APZC support in the root process?

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Ah, I thought that was the same issue as #1 on your list above. But even so, item #1 on the list sounds related; the WIP I posted for bug 912657 may not be setting the displayport properly for things in the root process. I guess you can debug it and see what's really happening.

Comment 29

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
>   2) Turning the screen into landscape orientation causes things to be
>      really messed up.

This is caused by the fact that no one updates the composition bounds of the APZC corresponding to the <div> grouping the toolbar and the content iframe. That <div> lives in the parent process, so this is a deficiency in the patch for bug 912657. I commented there: https://bugzilla.mozilla.org/show_bug.cgi?id=912657#c7.

Comment 30

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
>   3) About half of the time, after scrolling the toolbar out of view, and
>      lifting your finger, a further downward scroll is ignored, and you have
>      to scroll up (bringing the toolbar back into view) before you can
>      scroll down again.

One of things causing scrolling misbehaviour is bug 932525, which is unrelated to my patches for this bug. There are probably other things going on as well, but I will fix that bug first to reduce the noise it causes during testing.

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
> (In reply to Botond Ballo [:botond] from comment #25)
> >   3) About half of the time, after scrolling the toolbar out of view, and
> >      lifting your finger, a further downward scroll is ignored, and you have
> >      to scroll up (bringing the toolbar back into view) before you can
> >      scroll down again.
> 
> One of things causing scrolling misbehaviour is bug 932525, which is
> unrelated to my patches for this bug. There are probably other things going
> on as well, but I will fix that bug first to reduce the noise it causes
> during testing.

With the patch for bug 932525, this behaviour is mostly gone. It still occurs very occasionally, suggesting that there is another bug somewhere, but I think it's sufficiently rare to not block this from landing.

Comment 32

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
>   4) Occasionally, after scrolling around for a while, the content iframe
>      becomes entirely black.

Whenever see happens I see the following in logcat:

E/memalloc(  143): /dev/pmem: No more pmem available
E/msm7627a.gralloc(  143): gralloc failed err=Out of memor

suggesting that this is caused by bug 923303.

The fact that this happens with my patches and not without suggests that my patches are causing increased memory consumption. I opened bug 933490 to track this.

Comment 33

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
>   1) When scrolling the toolbar back into view, the portion that is being 
>      scrolled into view shows up as a black bar, and is only repainted
>      after I stop scrolling. This suggests that either there is no
>      displayport being set on the outer scrollable <div>, or one is being
>      set but it's too small.

It looks like the APZC is setting the displayport correctly (for example, when the toolbar, which is 50 pixels in height, is fully off-screen, then it sets one starting at y=-50), so the problem must be somewhere else.

Comment 34

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #32)
> (In reply to Botond Ballo [:botond] from comment #25)
> >   4) Occasionally, after scrolling around for a while, the content iframe
> >      becomes entirely black.
> 
> Whenever see happens I see the following in logcat:
> 
> E/memalloc(  143): /dev/pmem: No more pmem available
> E/msm7627a.gralloc(  143): gralloc failed err=Out of memor
> 
> suggesting that this is caused by bug 923303.
> 
> The fact that this happens with my patches and not without suggests that my
> patches are causing increased memory consumption. I opened bug 933490 to
> track this.

After updating b2g to get the fix for bug 905882, this black screen no longer happens. (The underlying pmem out-of-memory still happens, but the fix for that bug makes it fall back to ashmem in such a case.)

Comment 35

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #33)
> (In reply to Botond Ballo [:botond] from comment #25)
> >   1) When scrolling the toolbar back into view, the portion that is being 
> >      scrolled into view shows up as a black bar, and is only repainted
> >      after I stop scrolling. This suggests that either there is no
> >      displayport being set on the outer scrollable <div>, or one is being
> >      set but it's too small.
> 
> It looks like the APZC is setting the displayport correctly (for example,
> when the toolbar, which is 50 pixels in height, is fully off-screen, then it
> sets one starting at y=-50), so the problem must be somewhere else.

After a lot of debugging through first compositor then layout code, and with help from :mattwoodrow and :tn, I finally diagnosed this as a general layout issue, for which I filed bug 936277.

Comment 36

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #35)
> (In reply to Botond Ballo [:botond] from comment #33)
> > (In reply to Botond Ballo [:botond] from comment #25)
> > >   1) When scrolling the toolbar back into view, the portion that is being 
> > >      scrolled into view shows up as a black bar, and is only repainted
> > >      after I stop scrolling. This suggests that either there is no
> > >      displayport being set on the outer scrollable <div>, or one is being
> > >      set but it's too small.
> > 
> > It looks like the APZC is setting the displayport correctly (for example,
> > when the toolbar, which is 50 pixels in height, is fully off-screen, then it
> > sets one starting at y=-50), so the problem must be somewhere else.
> 
> After a lot of debugging through first compositor then layout code, and with
> help from :mattwoodrow and :tn, I finally diagnosed this as a general layout
> issue, for which I filed bug 936277.

And the patch from that bug resolves this issue!

Comment 37

[Botond Ballo [:botond]]

At this point, the only remaining issues with the dynamic toolbar are related to bug 912657. I will look at those next, and then land this bug and bug 912657 together.

Comment 38

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'mozscrollgrab' attribute

Comment 39

[Botond Ballo [:botond]]

Attachment: Part 2 - Propagate the 'mozscrollgrab' attribute to FrameMetrics

Comment 40

[Botond Ballo [:botond]]

Attachment: Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

Comment 41

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement scroll grabbing in APZ

Comment 42

[Botond Ballo [:botond]]

Rebased the patches and cleaned them up a bit, and flagged them for review.

Comment 43

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=81ed3f9493ff

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8334696 [details] [diff] [review]
Part 1 - Introduce a 'mozscrollgrab' attribute

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

::: content/base/src/nsContentUtils.cpp
@@ +5901,5 @@
> +bool
> +nsContentUtils::IsScrollGrabbing(nsIContent* aContent)
> +{
> +  return aContent && aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozscrollgrab,
> +                                           nsGkAtoms::_true, eCaseMatters);

Just use HasAttr. That's how HTML boolean attributes work --- if the attribute is present, it's "true".

::: dom/webidl/HTMLElement.webidl
@@ +98,5 @@
> +// This is likely to be revised.
> +partial interface HTMLElement {
> +  [ChromeOnly,SetterThrows]
> +           attribute boolean mozscrollgrab;
> +};

Don't add this. We would if this was standards-track, but it isn't, and we don't need this.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8334698 [details] [diff] [review]
Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2357,5 @@
>    // CompositorParent can always find the scrollable layer for the root content
>    // document.
> +  // If the element is marked 'mozscrollgrab', also force building of a layer
> +  // so that APZ can implement scroll grabbing.
> +  bool wantRealLayer = usingDisplayport || nsContentUtils::IsScrollGrabbing(mOuter->GetContent());

call this shouldBuildScrollableLayer.

Comment 46

[Botond Ballo [:botond]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44)
> ::: dom/webidl/HTMLElement.webidl
> @@ +98,5 @@
> > +// This is likely to be revised.
> > +partial interface HTMLElement {
> > +  [ChromeOnly,SetterThrows]
> > +           attribute boolean mozscrollgrab;
> > +};
> 
> Don't add this. We would if this was standards-track, but it isn't, and we
> don't need this.

Script in the browser app might want to toggle the mozscrollgrab attribute. Will removing this prevent it from doing so?

Comment 47

[(no longer active)]

I think that it's better for us to use a DOM property which is only exposed to b2g through a pref.  Using a content attribute we lose the ability to control where that attribute is being exposed, which seems worse.

Not exposing a content attribute makes writing code a bit harder since you won't be able to declare this in HTML but I think that is fine in this case.

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8334696 [details] [diff] [review]
Part 1 - Introduce a 'mozscrollgrab' attribute

Unflagging Part 1 since I will be changing it to address Ehsan's suggestion.

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8334697 [details] [diff] [review]
Part 2 - Propagate the 'mozscrollgrab' attribute to FrameMetrics

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

::: layout/base/nsDisplayList.cpp
@@ +766,5 @@
> +  // If the scroll frame's content is marked 'mozscrollgrab', record this
> +  // in the frame metrics, so APZC knows to provide the scroll grabbing
> +  // behaviour.
> +  if (aScrollFrame && nsContentUtils::IsScrollGrabbing(aScrollFrame->GetContent())) {
> +    metrics.mIsScrollGrab = true;

Not sure if I've mentioned this before (or, for that matter, if it's consistently used everywhere) but in layout code functions that start with "Get" (e.g. GetContent) can return null whereas ones that done (e.g. Frame()) do not. So in this case GetContent() can return null. I noticed that in the IsScrollGrabbing implementation you guard against a null param so this is fine but nsContentUtils.h should probably document that it can take a null pointer, or you can check GetContent() for nullness here and omit the check in IsScrollGrabbing. I don't know which approach is the better one.

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8334698 [details] [diff] [review]
Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2391,5 @@
>      // ScrollLayerWrapper must always be created because it initializes the
>      // scroll layer count. The display lists depend on this.
>      ScrollLayerWrapper wrapper(mOuter, mScrolledFrame);
>  
> +    if (wantRealLayer) {

I don't know if this change is right. Do we still want to run through the body of this condition if there is no displayport on the element but mozscrollgrab is set?

I feel like the only code change you should need to this file should be this:

diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
index bb25c12..562a6cc 100644
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2354,7 +2354,7 @@ ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
   // CompositorParent can always find the scrollable layer for the root content
   // document.
   bool shouldBuildLayer = false;
-  if (usingDisplayport) {
+  if (usingDisplayport || nsContentUtils::IsScrollGrabbing(mOuter->GetContent())) {
     shouldBuildLayer = true;
   } else {
     nsRect scrollRange = GetScrollRange();

I'm not sure about this though.

Comment 51

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8334701 [details] [diff] [review]
Part 4 - Implement scroll grabbing in APZ

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

Minusing for now; would like to see the missing clear() added and the GetTargetAPZC code extracted into a new function.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +340,5 @@
>  APZCTreeManager::GetTouchInputBlockAPZC(const WidgetTouchEvent& aEvent,
>                                          ScreenPoint aPoint)
>  {
> +  bool hasSingleTouchPoint = aEvent.touches.Length() == 1;
> +  nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aPoint, hasSingleTouchPoint);

There's some code duplication here that's been bugging me for a while actually. I filed bug 941138 for it.

@@ +416,5 @@
>        mTouchCount = 0;
>      }
>      if (mTouchCount == 0) {
>        mApzcForInputBlock = nullptr;
> +      mOverscrollHandoffChain.clear();

There needs to be another one of these in ReceiveInputEvent(const InputData& ...) in the analogous place.

@@ +636,5 @@
>  
>  bool
>  APZCTreeManager::HitTestAPZC(const ScreenIntPoint& aPoint)
>  {
> +  return GetTargetAPZC(aPoint).get() != nullptr;

This change would be better in a separate patch since it's only tangentially related to the other stuff. The scrollgrab changes don't actually change the return value of this function for any input.

@@ +665,2 @@
>  already_AddRefed<AsyncPanZoomController>
> +APZCTreeManager::GetTargetAPZC(const ScreenPoint& aPoint, bool aAccountForScrollGrab)

I would prefer breaking out the code to account for scroll grab into a separate function rather than piggybacking it onto here. It's only invoked from a couple of places (might even just be one place if we deduplicate that code) so it shouldn't be a big burden. Having it conditionally run inside this function doesn't seem that nice since it's conceptually separate and pretty easy to lift out.

@@ +703,5 @@
> +    // Now adjust the chain to account for scroll grabbing. Sorting is a bit
> +    // of an overkill here, but scroll grabbing will likely be generalized
> +    // to scroll priorities, so we might as well do it this way.
> +    std::stable_sort(mOverscrollHandoffChain.begin(), mOverscrollHandoffChain.end(),
> +                     CompareByScrollPriority());

Doesn't the stable_sort here mean that the innermost scroll-grabbing APZC will get scrolled first? I would have thought that we want the outermost scroll-grabbing APZC to get scrolled first. Doesn't matter in practice since for now we'll only ever have one scroll-grabbing APZC anyway but just want to make sure we're on the same page.

Comment 52

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'mozscrollgrab' DOM property

Updated Part 1 patch to use a DOM property instead of an HTML attribute, as ehsan suggested.

Removed kats as reviewer because he's on PTO; added roc and ehsan because they had comments about this patch.

Comment 53

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'mozscrollgrab' DOM property

Updated Part 1 patch to address Kats' review comments.

Comment 54

[Botond Ballo [:botond]]

Attachment: Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

Updated Part 3 patch to address review comments by kats and roc.

Comment 55

[Botond Ballo [:botond]]

(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from comment #50)
> Comment on attachment 8334698 [details] [diff] [review]
> Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a
> real scrollable layer rather than a scroll info layer
> 
> Review of attachment 8334698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +2391,5 @@
> >      // ScrollLayerWrapper must always be created because it initializes the
> >      // scroll layer count. The display lists depend on this.
> >      ScrollLayerWrapper wrapper(mOuter, mScrolledFrame);
> >  
> > +    if (wantRealLayer) {
> 
> I don't know if this change is right. Do we still want to run through the
> body of this condition if there is no displayport on the element but
> mozscrollgrab is set?
> 
> I feel like the only code change you should need to this file should be this:
> 
> diff --git a/layout/generic/nsGfxScrollFrame.cpp
> b/layout/generic/nsGfxScrollFrame.cpp
> index bb25c12..562a6cc 100644
> --- a/layout/generic/nsGfxScrollFrame.cpp
> +++ b/layout/generic/nsGfxScrollFrame.cpp
> @@ -2354,7 +2354,7 @@
> ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>    // CompositorParent can always find the scrollable layer for the root
> content
>    // document.
>    bool shouldBuildLayer = false;
> -  if (usingDisplayport) {
> +  if (usingDisplayport ||
> nsContentUtils::IsScrollGrabbing(mOuter->GetContent())) {
>      shouldBuildLayer = true;
>    } else {
>      nsRect scrollRange = GetScrollRange();
> 
> I'm not sure about this though.

I think you're right - good catch! I made this change in my updated patch, and asked tn for review to be sure it's right.

Comment 56

[Botond Ballo [:botond]]

(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from comment #51)
> @@ +703,5 @@
> > +    // Now adjust the chain to account for scroll grabbing. Sorting is a bit
> > +    // of an overkill here, but scroll grabbing will likely be generalized
> > +    // to scroll priorities, so we might as well do it this way.
> > +    std::stable_sort(mOverscrollHandoffChain.begin(), mOverscrollHandoffChain.end(),
> > +                     CompareByScrollPriority());
> 
> Doesn't the stable_sort here mean that the innermost scroll-grabbing APZC
> will get scrolled first? I would have thought that we want the outermost
> scroll-grabbing APZC to get scrolled first. Doesn't matter in practice since
> for now we'll only ever have one scroll-grabbing APZC anyway but just want
> to make sure we're on the same page.

My "specification" for scroll priorities in the presence of 'mozscrollgrab' is that:
  - A 'mozscrollgrab' APZC always takes priority over a non-'mozscrollgrab' APZC. 
  - Among non-'mozscrollgrab' APZCs, an inner one takes priority over an outer one.
  - If there are multiple 'mozscrollgrab' APZCs in a chain, their relative priority is unspecified.

The sorting being stable ensures the second criterion.

Comment 57

[(no longer active)]

Comment on attachment 8336344 [details] [diff] [review]
Part 1 - Introduce a 'mozscrollgrab' DOM property

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1816,5 @@
>    return true;
>  }
>  
> +/* static */ bool
> +nsGenericHTMLElement::IsCertifiedApp(JSContext*, JSObject*)

Please rename this method appropriately.  Its implementation does more than what its name suggests.

::: content/html/content/src/nsGenericHTMLElement.h
@@ +235,5 @@
> +  {
> +    mMozscrollgrab = aValue;
> +  }
> +
> +  bool mMozscrollgrab;

Make this private please, and move it alongside with other member variables.

::: dom/webidl/HTMLElement.webidl
@@ +97,5 @@
> +// Extension for scroll-grabbing, used in the B2G dynamic toolbar.
> +// This is likely to be revised.
> +partial interface HTMLElement {
> +  [Func="nsGenericHTMLElement::IsCertifiedApp"]
> +           attribute boolean mozscrollgrab;

Why the moz prefix?

Comment 58

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #55)
> (In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from
> comment #50)
> > Comment on attachment 8334698 [details] [diff] [review]
> > Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a
> > real scrollable layer rather than a scroll info layer
> > 
> > Review of attachment 8334698 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/generic/nsGfxScrollFrame.cpp
> > @@ +2391,5 @@
> > >      // ScrollLayerWrapper must always be created because it initializes the
> > >      // scroll layer count. The display lists depend on this.
> > >      ScrollLayerWrapper wrapper(mOuter, mScrolledFrame);
> > >  
> > > +    if (wantRealLayer) {
> > 
> > I don't know if this change is right. Do we still want to run through the
> > body of this condition if there is no displayport on the element but
> > mozscrollgrab is set?
> > 
> > I feel like the only code change you should need to this file should be this:
> > 
> > diff --git a/layout/generic/nsGfxScrollFrame.cpp
> > b/layout/generic/nsGfxScrollFrame.cpp
> > index bb25c12..562a6cc 100644
> > --- a/layout/generic/nsGfxScrollFrame.cpp
> > +++ b/layout/generic/nsGfxScrollFrame.cpp
> > @@ -2354,7 +2354,7 @@
> > ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
> >    // CompositorParent can always find the scrollable layer for the root
> > content
> >    // document.
> >    bool shouldBuildLayer = false;
> > -  if (usingDisplayport) {
> > +  if (usingDisplayport ||
> > nsContentUtils::IsScrollGrabbing(mOuter->GetContent())) {
> >      shouldBuildLayer = true;
> >    } else {
> >      nsRect scrollRange = GetScrollRange();
> > 
> > I'm not sure about this though.
> 
> I think you're right - good catch! I made this change in my updated patch,
> and asked tn for review to be sure it's right.

Actually, nope. The code that causes an actual scrollable layer to be built is the WrapListsInPlace() call inside that if block. Otherwise only a scroll info layer gets built.

Comment 59

[Botond Ballo [:botond]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #57)
> Comment on attachment 8336344 [details] [diff] [review]
> Part 1 - Introduce a 'mozscrollgrab' DOM property
> 
> Review of attachment 8336344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsGenericHTMLElement.cpp
> @@ +1816,5 @@
> >    return true;
> >  }
> >  
> > +/* static */ bool
> > +nsGenericHTMLElement::IsCertifiedApp(JSContext*, JSObject*)
> 
> Please rename this method appropriately.  Its implementation does more than
> what its name suggests.

IsCertifiedAppOrChrome?

> ::: dom/webidl/HTMLElement.webidl
> @@ +97,5 @@
> > +// Extension for scroll-grabbing, used in the B2G dynamic toolbar.
> > +// This is likely to be revised.
> > +partial interface HTMLElement {
> > +  [Func="nsGenericHTMLElement::IsCertifiedApp"]
> > +           attribute boolean mozscrollgrab;
> 
> Why the moz prefix?

To indicate that it is not something standard? I can remove it if you prefer.

Comment 60

[Botond Ballo [:botond]]

Attachment: Part 3 - Force scroll frames for 'mozscrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

Updated Part 3 patch against to reinstate the original logic.

Comment 61

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement scroll grabbing in APZ

Updated Part 4 patch to address Kats' review comments.

Comment 62

[(no longer active)]

(In reply to comment #59)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #57)
> > Comment on attachment 8336344 [details] [diff] [review]
> > Part 1 - Introduce a 'mozscrollgrab' DOM property
> > 
> > Review of attachment 8336344 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/html/content/src/nsGenericHTMLElement.cpp
> > @@ +1816,5 @@
> > >    return true;
> > >  }
> > >  
> > > +/* static */ bool
> > > +nsGenericHTMLElement::IsCertifiedApp(JSContext*, JSObject*)
> > 
> > Please rename this method appropriately.  Its implementation does more than
> > what its name suggests.
> 
> IsCertifiedAppOrChrome?

I prefer IsScrollGrabEnabled.

> > ::: dom/webidl/HTMLElement.webidl
> > @@ +97,5 @@
> > > +// Extension for scroll-grabbing, used in the B2G dynamic toolbar.
> > > +// This is likely to be revised.
> > > +partial interface HTMLElement {
> > > +  [Func="nsGenericHTMLElement::IsCertifiedApp"]
> > > +           attribute boolean mozscrollgrab;
> > 
> > Why the moz prefix?
> 
> To indicate that it is not something standard? I can remove it if you prefer.

We are trying to move away from vendor prefixes, as they don't really make anything better for anyone, so please drop it here as well.  (But since this won't be exposed to content it doesn't matter a whole lot.)

Comment 63

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'scrollgrab' DOM property

Updated Part 1 patch to address Ehsan's review comments.

Comment 64

[Botond Ballo [:botond]]

Try push with the updated patches: https://tbpl.mozilla.org/?tree=Try&rev=e9697a48c416

Comment 65

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement scroll grabbing in APZ

Updated Part 4 patch to fix a memory management bug.

Comment 66

[Botond Ballo [:botond]]

Retrying: https://tbpl.mozilla.org/?tree=Try&rev=3e073447f38f

Comment 67

[Chris Lord [:cwiiis]]

Comment on attachment 8336387 [details] [diff] [review]
Part 1 - Introduce a 'scrollgrab' DOM property

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

LGTM.

::: content/base/src/nsContentUtils.cpp
@@ +5898,5 @@
>    return !zombieViewer;
>  }
>  
> +bool
> +nsContentUtils::IsScrollGrabbing(nsIContent* aContent)

IsScrollGrabbing sounds like it does more than this - perhaps IsScrollGrabbingEnabled, or HasScrollGrabbing? If others have already advised different names, I bow to to their comments, this isn't my area.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1816,5 @@
>    return true;
>  }
>  
> +/* static */ bool
> +nsGenericHTMLElement::IsScrollGrabEnabled(JSContext*, JSObject*)

Should this be called IsScrollGrabAllowed rather than Enabled?

Comment 68

[Chris Lord [:cwiiis]]

Comment on attachment 8334697 [details] [diff] [review]
Part 2 - Propagate the 'mozscrollgrab' attribute to FrameMetrics

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

LGTM, some nits.

::: gfx/layers/FrameMetrics.h
@@ +255,5 @@
>  
>    uint32_t mPresShellId;
> +
> +  // Whether or not this frame is for an element marked 'mozscrollgrab'.
> +  bool mIsScrollGrab;

I'd prefer mIsScrollGrabbing or mHasScrollGrab. Also, don't forget to update this comment when this property changes to 'scrollgrab'.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +274,5 @@
>     */
>    void AttemptScroll(const ScreenPoint& aStartPoint, const ScreenPoint& aEndPoint);
>  
> +  /**
> +   * Returns whether this APZC is for an element marked with the 'mozscrollgrab'

And this comment.

::: layout/base/nsDisplayList.cpp
@@ +762,5 @@
>    }
>  
>    metrics.mPresShellId = presShell->GetPresShellId();
>  
> +  // If the scroll frame's content is marked 'mozscrollgrab', record this

and this one.

@@ +763,5 @@
>  
>    metrics.mPresShellId = presShell->GetPresShellId();
>  
> +  // If the scroll frame's content is marked 'mozscrollgrab', record this
> +  // in the frame metrics, so APZC knows to provide the scroll grabbing

nit, no need for a comma here. Might be nice to say FrameMetrics instead of 'frame metrics' too, just to be really explicit.

Comment 69

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #67)
> Comment on attachment 8336387 [details] [diff] [review]
> Part 1 - Introduce a 'scrollgrab' DOM property
> 
> Review of attachment 8336387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: content/base/src/nsContentUtils.cpp
> @@ +5898,5 @@
> >    return !zombieViewer;
> >  }
> >  
> > +bool
> > +nsContentUtils::IsScrollGrabbing(nsIContent* aContent)
> 
> IsScrollGrabbing sounds like it does more than this - perhaps
> IsScrollGrabbingEnabled, or HasScrollGrabbing? If others have already
> advised different names, I bow to to their comments, this isn't my area.

It returns whether or not 'aContent' has the 'scrollgrab' property set. Perhaps I could call it IsScrollgrab (to match what the FrameMetrics method is called), or HasScrollgrab?

Also, what more does IsScrollGrabbing suggest?

> ::: content/html/content/src/nsGenericHTMLElement.cpp
> @@ +1816,5 @@
> >    return true;
> >  }
> >  
> > +/* static */ bool
> > +nsGenericHTMLElement::IsScrollGrabEnabled(JSContext*, JSObject*)
> 
> Should this be called IsScrollGrabAllowed rather than Enabled?

Good idea! I'll change that.

Comment 70

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #69)
> (In reply to Chris Lord [:cwiiis] from comment #67)
> > Comment on attachment 8336387 [details] [diff] [review]
> > Part 1 - Introduce a 'scrollgrab' DOM property
> > 
> > Review of attachment 8336387 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > LGTM.
> > 
> > ::: content/base/src/nsContentUtils.cpp
> > @@ +5898,5 @@
> > >    return !zombieViewer;
> > >  }
> > >  
> > > +bool
> > > +nsContentUtils::IsScrollGrabbing(nsIContent* aContent)
> > 
> > IsScrollGrabbing sounds like it does more than this - perhaps
> > IsScrollGrabbingEnabled, or HasScrollGrabbing? If others have already
> > advised different names, I bow to to their comments, this isn't my area.
> 
> It returns whether or not 'aContent' has the 'scrollgrab' property set.
> Perhaps I could call it IsScrollgrab (to match what the FrameMetrics method
> is called), or HasScrollgrab?

HasScrollgrab sounds good to me, but you might want to get someone from content to comment on this.

> Also, what more does IsScrollGrabbing suggest?

The 'Grabbing' part makes it sound like it's actively doing something to me - if I read that without context, I'd assume that IsScrollGrabbing returned true when some kind of scroll grab was occurring, as opposed to the element just having a property set. I don't think it's a big deal though, as long as it's documented.

Comment 71

[Chris Lord [:cwiiis]]

Comment on attachment 8336402 [details] [diff] [review]
Part 4 - Implement scroll grabbing in APZ

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

Looks good!

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +625,2 @@
>  {
> +  ++aOverscrollHandoffChainIndex;

I can see what this is doing, but this is a pretty complexly-worded variable and abstract thing to be doing without comments - let's comment this block.

@@ +717,5 @@
> +  mOverscrollHandoffChain.clear();
> +
> +  // Start with the child -> parent chain.
> +  AsyncPanZoomController* apzc = aInitialTarget;
> +  while (apzc) {

personally, I'd prefer this to be a for loop:

for (AsyncPanZoomController* apzc = aInitialTarget; apzc; apzc = apzc->GetParent()) { ... }

@@ +731,5 @@
> +  // of an overkill here, but scroll grabbing will likely be generalized
> +  // to scroll priorities, so we might as well do it this way.
> +  // The sorting being stable ensures that the relative order between
> +  // non-scrollgrabbing APZCs remains child -> parent.
> +  // The relative order between scrollgrabbing APZCs is unspecified.

It's not though if it's stable? If it's stable, it means the inner-most will scroll first, right? This comment should be updated.

Comment 72

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #71)
> @@ +731,5 @@
> > +  // of an overkill here, but scroll grabbing will likely be generalized
> > +  // to scroll priorities, so we might as well do it this way.
> > +  // The sorting being stable ensures that the relative order between
> > +  // non-scrollgrabbing APZCs remains child -> parent.
> > +  // The relative order between scrollgrabbing APZCs is unspecified.
> 
> It's not though if it's stable? If it's stable, it means the inner-most will
> scroll first, right? This comment should be updated.

I meant unspecified from a specification perspective - while it is the case that with current implementation, an inner scrollgrabbing element will scroll before an outer one, a front-end developer shouldn't rely on that behaviour (whereas they can rely on an inner *non*-scrollgrabbing element scrolling before an outer one). It doesn't really matter all that much as scroll grabbing is not standard, and it's likely to undergo changes if it gets standardized. I will clarify the comment.

Thanks for the reviews! I will update the patches as per your comments.

Comment 73

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce a 'scrollgrab' DOM property

Comment 74

[Botond Ballo [:botond]]

Attachment: Part 2 - Propagate the 'scrollgrab' attribute to FrameMetrics

Comment 75

[Botond Ballo [:botond]]

Attachment: Part 3 - Force scroll frames for 'scrollgrab' elements to always create a real scrollable layer rather than a scroll info layer

Comment 76

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement scroll grabbing in APZ

Comment 77

[Botond Ballo [:botond]]

Updated patches to address Chris's review comments. Carried r+ for all of them.

Comment 78

[Botond Ballo [:botond]]

Latest try run looks clean: https://tbpl.mozilla.org/?tree=Try&rev=3e073447f38f

Comment 79

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f5ea934a19e8
https://hg.mozilla.org/integration/b2g-inbound/rev/6877fced862a
https://hg.mozilla.org/integration/b2g-inbound/rev/e9c7bf84fcfb
https://hg.mozilla.org/integration/b2g-inbound/rev/8cf6021169e2

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5ea934a19e8
https://hg.mozilla.org/mozilla-central/rev/6877fced862a
https://hg.mozilla.org/mozilla-central/rev/e9c7bf84fcfb
https://hg.mozilla.org/mozilla-central/rev/8cf6021169e2

Comment 81

[Botond Ballo [:botond]]

The title of this bug has fallen out of sync with the patches that landed for it, adjusting.

Comment 82

[u279076]

Does this code have/need tests? Also do we need to track this as a feature or relnote it?

Comment 83

[Kartikaya Gupta (email:[e-mail])]

It doesn't have any tests currently. For now I would also say to not track it as a feature or relnote it, as the future is uncertain with respect to this code. It's not currently being used and we may need to modify how it works when it does eventually get used. If reasonable we'll try to get something standardized.

Comment 84

[Olli Pettay [:smaug][[e-mail]]]

Hrm, this added a boolean member variable to nsGenericHTMLElement. Doesn't that end up
increasing memory usage of all the HTMLElements 4 or 8 bytes (depending on the platform)?

Comment 85

[Botond Ballo [:botond]]

(In reply to Olli Pettay [:smaug] from comment #84)
> Hrm, this added a boolean member variable to nsGenericHTMLElement. Doesn't that end up
> increasing memory usage of all the HTMLElements 4 or 8 bytes (depending on the platform)?

It looks like it does, yeah; on my system, it causes sizeof(nsGenericHTMLElement) to increase from 136 to 144.

Is there a better place to store the property?

Comment 86

[Olli Pettay [:smaug][[e-mail]]]

We don't seem to have spare bits in nsINode::mBoolFlags, but
I think we do have some spare bits in NODE_FLAG_BIT, see nsINode.h.

Comment 87

[Botond Ballo [:botond]]

(In reply to Olli Pettay [:smaug] from comment #86)
> We don't seem to have spare bits in nsINode::mBoolFlags, but
> I think we do have some spare bits in NODE_FLAG_BIT, see nsINode.h.

Ok, thanks - I filed bug 1277298 for it.

Comment 88

[(no longer active)]

Botond, is this feature intended to still be developed?  This is currently restricted only to chrome callers and b2g certified apps, the latter of which is no longer a thing...

Comment 89

[Botond Ballo [:botond]]

The primary user of the feature was the B2G dynamic toolbar. However, the browser.html folks have also expressed an interest in using it in their UI.

I'd like to keep the feature around for projects like browser.html experimenting with new browser UIs. If necessary, we can adjust the restriction to be just "chrome only".

Comment 90

[(no longer active)]

Great!  I filed bug 1301126 for that.  Thanks!