898580 - Contents needs to inform APZC about scroll events on Metro

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

As a result of the fix to bug 859929, APZC needs to be informed about scroll events. This was implemented for Fenec in bug 859929, and for B2G in bug 895905. While I haven't looked into it, it's likely that something will have to be done specifically for Metro as well.

Comment 1

[Botond Ballo [:botond]]

Now that multi-APZC has landed, the fix for this bug needs to take that into account as well. That is, when content informs APZC about a scroll event, it needs to along send along information that identifies the (sub)frame that was scrolled.

Comment 2

[Brian R. Bondy [:bbondy]]

How's this going botond?

Comment 3

[Botond Ballo [:botond]]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> How's this going botond?

Fairly well. I have worked out a strategy for getting the required info to the APZC in the presence of multi-APZC, and I'm starting to implement it now. It will depend on some changes in bug 895905.

One thing that is holding up bug 895905 from landing is that there are some multi-APZC-related regressions in B2G, and as a result I couldn't test my patch for that properly.

I will hopefully have a patch for this bug later today. (Testing that, in turn, may be held up a bit by multi-APZC-related regressions on Metro).

Comment 4

[Brian R. Bondy [:bbondy]]

Sounds great, thanks!

Comment 5

[Botond Ballo [:botond]]

> I have worked out a strategy for getting the required info to
> the APZC in the presence of multi-APZC

Just to summarize this strategy:

  - Modify the "scroll" async message sent in ContentScroll.sendScroll()
    browser/metro/base/content/bindings/browser.js to include
      - the view ID of the element being scrolled
      - the pres shell ID of the pres shell associated with the element
      - the scroll offset
    This info can be gathered in JS code in roughly the same way as it's
    gathered in C++ code in TabChild::HandleEvent() in the patch for
    bug 895905.
  - The handler in browser/metro/base/content/bindings/browser.xml can
    probably ignore "scroll" async messages that do not pertain to the
    root scroll frame.
  - Add a new handler for the "scroll" async message in 
    browser/metro/base/content/apzc.js, and in response to it, forward
    the information to C++ code via the observer mechanism.
  - Handle the message in C++ code (probably in MetroWidget.cpp) by calling
    APZCTreeManager::UpdateScrollOffset().

Any feedback is appreciated.

Comment 6

[Brian R. Bondy [:bbondy]]

That sounds right to me.

Comment 7

[Botond Ballo [:botond]]

Attachment: bug898580.patch

Patch that implements the strategy outlined above.

Comment 8

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

Comment on attachment 788384 [details] [diff] [review]
bug898580.patch

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

r=me with commit message fixed and nits addressed.

::: browser/metro/base/content/apzc.js
@@ +50,5 @@
>          break;
>        }
>        case 'TabClose': {
>          let browser = aEvent.originalTarget.linkedBrowser;
> +        browser.removeEventListener("pageshow", this, true);

By default I would lean towards putting this fix into another patch, but if brian thinks this is fine then I have no problems with it.

@@ +126,5 @@
>      } else if (aTopic == "apzc-handle-pan-end") {
>        Util.dumpLn("APZC pan-end");
>      }
> +  },
> +  receiveMessage: function(aMessage) {

blank line above receiveMessage

::: browser/metro/base/content/bindings/browser.js
@@ +688,4 @@
>  
> +      if (target == content.document) {
> +        if (this._scrollOffset.x == scrollOffset.x && this._scrollOffset.y == scrollOffset.y)
> +          return;

Braces on the if

@@ +688,5 @@
>  
> +      if (target == content.document) {
> +        if (this._scrollOffset.x == scrollOffset.x && this._scrollOffset.y == scrollOffset.y)
> +          return;
> +        else {

No need for the else after the return

@@ +694,5 @@
> +          isRoot = true;
> +        }
> +      }
> +    }
> +    else {

} else {

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1193,5 @@
>    /**
> +   * Find the view ID for a given element. This is the reverse of
> +   * findElementWithViewId().
> +   */
> +  nsViewID getViewId(in nsIDOMElement aElement);

Whenever making changes to these scriptable IDL files you need to change the uuid at the top of the IDL file. You can generate a new uuid by asking firebot on IRC or using the uuidgen command-line tool if you have that installed.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 788384 [details] [diff] [review]
bug898580.patch

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

This is looking good but I think I still experience the issue this should solve.
If I scroll down the page with the mouse scrollbar, then try to pan with touch, it jumps back up to the top of the page for a second and then back down to continue the pan.

::: browser/metro/base/content/apzc.js
@@ +50,5 @@
>          break;
>        }
>        case 'TabClose': {
>          let browser = aEvent.originalTarget.linkedBrowser;
> +        browser.removeEventListener("pageshow", this, true);

This is fine.

::: dom/base/nsDOMWindowUtils.cpp
@@ +1695,4 @@
>  }
>  
>  NS_IMETHODIMP
> +nsDOMWindowUtils::GetViewId(nsIDOMElement* aElement, nsViewID* aResult)

Please split this out for someone in DOM to review.

::: widget/windows/winrt/MetroWidget.cpp
@@ +1478,5 @@
> +
> +NS_IMETHODIMP
> +MetroWidget::Observe(nsISupports *subject, const char *topic, const PRUnichar *data)
> +{
> +  LogFunction();

nit: this probably isn't too useful of a logged function moving forward

@@ +1492,5 @@
> +                         &presShellId,
> +                         &scrollOffset.x,
> +                         &scrollOffset.y);
> +    if (matched != 4)
> +      return NS_ERROR_UNEXPECTED;

nit: Please log a warning here

Comment 10

[Botond Ballo [:botond]]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> This is looking good but I think I still experience the issue this should
> solve.
> If I scroll down the page with the mouse scrollbar, then try to pan with
> touch, it jumps back up to the top of the page for a second and then back
> down to continue the pan.

I can't reproduce this. Could you give me an example of a website for which you see this?

Comment 11

[Brian R. Bondy [:bbondy]]

I rebuilt incrementally and see this on all websites, maybe it didn't pick something up though. I'm doing a clobber right now with the patch applied though so I'll report back when that's done.

Comment 12

[Botond Ballo [:botond]]

Attachment: bug898580-dom.patch

Split out addition to nsIDOMWindowUtils for separate review.

Comment 13

[Botond Ballo [:botond]]

Attachment: bug898580.patch

Rest of patch, with review comments addressed.

Comment 14

[Botond Ballo [:botond]]

Try server results: https://tbpl.mozilla.org/?tree=Try&rev=f53309798180

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 788957 [details] [diff] [review]
bug898580.patch

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

drive by nits -

::: widget/windows/winrt/MetroWidget.cpp
@@ +847,5 @@
> +
> +    nsresult rv;
> +    nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1", &rv);
> +    if (NS_SUCCEEDED(rv)) {
> +      observerService->AddObserver(this, "scroll-offset-changed", false);

You add this, but you don't remove it. You can remove them in Destroy(). See the patch in bug 901607.

::: widget/windows/winrt/MetroWidget.h
@@ +206,5 @@
>      mMetroInput = aMetroInput;
>    }
>  
> +  // nsIObserver implementation
> +  NS_IMETHOD Observe(nsISupports *subject, const char *topic, const PRUnichar *data);

you can use

NS_DECL_NSIOBSERVER

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 1 - Inform APZC about scroll events on Metro

Updated patch to address :jimm's review comments.

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 789145 [details] [diff] [review]
Part 1 - Inform APZC about scroll events on Metro

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

I'm not sure if there is a problem here or somewhere else but I'm still experiencing the jumping.
I can reproduce it every time and it seems to happen on any site.

Steps to reproduce:
1. Load slashdot.org
2. Scroll by using your mouse left button and dragging the scrollbar down on the right of the window
3. Try to pan with touch

Actual results:
It jumps to the old location before you scrolled with, and then immediately goes back to where you are panning

Expected results:
No jumping.

It would be nice to try and reproduce / fix this during the time that you have to wait anyway for the dependent patch to land.

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 2 - Remove obsoleted code in APZC

Removed the old scroll offset updating code in AsyncPanZoomController::NotifyLayersUpdated. Now that the new mechanism is in place for Fennec, B2G, and Metro, nothing needs it anymore.

Comment 19

[Brian R. Bondy [:bbondy]]

I still have that problem with it jumping up even with the new patch.  Also I get the whole screen turning black after panning issue again.  I don't think the later one is related to this bug though.

Comment 20

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

Comment on attachment 788956 [details] [diff] [review]
bug898580-dom.patch


> NS_IMETHODIMP
>+nsDOMWindowUtils::GetViewId(nsIDOMElement* aElement, nsViewID* aResult)
>+{
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
>+  if (content && nsLayoutUtils::FindIDFor(content, aResult))
>+    return NS_OK;
>+  return NS_ERROR_NOT_AVAILABLE;
>+}
{} with 'if'

Comment 21

[Botond Ballo [:botond]]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> I still have that problem with it jumping up even with the new patch.

What device do you experience this on?

Comment 22

[Brian R. Bondy [:bbondy]]

Sony vaio - normal dpi, although it's windows 8.1 so that could be a factor as well.

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 0 - Add nsIDOMWindowUtils.getViewId(element)

Updated bug898580-dom.patch to address :smaug's comment. Carrying r+.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 790273 [details] [diff] [review]
Part 0 - Add nsIDOMWindowUtils.getViewId(element)

Part 0 - Add nsIDOMWindowUtils.getViewId(element)

Comment 25

[Jim Mathies [:jimm]]

Is this work ready to land? It's holding up bug 901607.

Comment 26

[Botond Ballo [:botond]]

(In reply to Jim Mathies [:jimm] from comment #25)
> Is this work ready to land? It's holding up bug 901607.

Brian, should we take additional steps to reproduce the issue described in comment 19 (for example, try to install windows 8.1 on a test device) before landing this, or can we land this now? The dependent bug has landed, so this is otherwise ready to land.

Comment 27

[Brian R. Bondy [:bbondy]]

As long as it applies cleanly still and passes tests, let's just land it and post issues for followup work.  Thanks for checking though! :)

Comment 28

[Botond Ballo [:botond]]

Attachment: Part 0 - Add nsIDOMWindowUtils.getViewId(element)

Comment 29

[Botond Ballo [:botond]]

Attachment: Part 1 - Inform APZC about scroll events on Metro

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 2 - Remove obsoleted code in APZC

Comment 31

[Botond Ballo [:botond]]

Rebased patches, carrying r+.

Comment 32

[Jim Mathies [:jimm]]

Botond, what are the side effects of this bug not being fixed? Is there be some sort of vidual side effect in current nightlies that would be addressed once these patches land?

Comment 33

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #32)
> Botond, what are the side effects of this bug not being fixed? Is there be
> some sort of vidual side effect in current nightlies that would be addressed
> once these patches land?

*Is there some sort of visual side effect..

Comment 34

[Botond Ballo [:botond]]

(In reply to Jim Mathies [:jimm] from comment #32)
> Botond, what are the side effects of this bug not being fixed? Is there be
> some sort of vidual side effect in current nightlies that would be addressed
> once these patches land?

If content on the page causes a scrollTo(), without these patches the APZC does not get notified of the change in scroll offset. As a result, a subsequent pan will cause a scroll relative to the old scroll offset, causing the page to briefly jump back to the old scroll offset.

By the way, these patches should land later today. Just waiting for a final try server run.

Comment 35

[Botond Ballo [:botond]]

Try server results: https://tbpl.mozilla.org/?tree=Try&rev=e2a52b21ec02

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/842ac636819c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3078301c9b34
https://hg.mozilla.org/integration/mozilla-inbound/rev/1383705460ab

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/842ac636819c
https://hg.mozilla.org/mozilla-central/rev/3078301c9b34
https://hg.mozilla.org/mozilla-central/rev/1383705460ab

Comment 38

[Philip Chee]

I'm getting this error:
c:/t1/hg/comm-central/mozilla/dom/base/nsDOMWindowUtils.cpp(1698) : error C2039: 'GetViewId' : is not a member of 'nsDOMWindowUtils'
        c:\t1\hg\comm-central\mozilla\dom\base\nsDOMWindowUtils.h(18) : see declaration of 'nsDOMWindowUtils'
make.py[4]: Leaving directory 'c:\t1\hg\objdir-sm\mozilla\dom\bindings'

Comment 39

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

Philip: does your repo include the change to nsIDOMWindowUtils.idl from revision 842ac636819c? (first patch on this bug). Do you get this error with a clobber build?

Comment 40

[Philip Chee]

> Philip: does your repo include the change to nsIDOMWindowUtils.idl from revision
> 842ac636819c? (first patch on this bug).
Yes.

> Do you get this error with a clobber build?
I'll try a CLOBBER. Will report back.

Comment 41

[Philip Chee]

CLOBBER fixed, Thanks.