Closed Bug 898580 Opened 7 years ago Closed 6 years ago

Contents needs to inform APZC about scroll events on Metro

Categories

(Core :: Graphics: Layers, defect)

All
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [preview] [depends on patch for bug 895905])

Attachments

(3 files, 6 obsolete files)

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.
Depends on: 859929, 895905
Whiteboard: [preview]
Assignee: nobody → 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.
How's this going 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).
Sounds great, thanks!
> 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.
That sounds right to me.
Attached patch bug898580.patch (obsolete) — Splinter Review
Patch that implements the strategy outlined above.
Attachment #788384 - Flags: review?(netzen)
Attachment #788384 - Flags: review?(bugmail.mozilla)
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.
Attachment #788384 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #788384 - Flags: review?(netzen) → feedback+
(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?
Flags: needinfo?(netzen)
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.
Flags: needinfo?(netzen)
Attached patch bug898580-dom.patch (obsolete) — Splinter Review
Split out addition to nsIDOMWindowUtils for separate review.
Attachment #788956 - Flags: review?(bugs)
Attached patch bug898580.patch (obsolete) — Splinter Review
Rest of patch, with review comments addressed.
Attachment #788384 - Attachment is obsolete: true
Attachment #788957 - Flags: review?(netzen)
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
Blocks: 901607
OS: Linux → Windows 8 Metro
Whiteboard: [preview] → [preview] [depends on patch for bug 895905]
Updated patch to address :jimm's review comments.
Attachment #788957 - Attachment is obsolete: true
Attachment #788957 - Flags: review?(netzen)
Attachment #789145 - Flags: review?(netzen)
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.
Attachment #789145 - Flags: review?(netzen) → review+
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.
Attachment #789739 - Flags: review?(bugmail.mozilla)
Attachment #789739 - Flags: review?(netzen)
Attachment #789739 - Flags: review?(bugmail.mozilla) → review+
Attachment #789739 - Flags: review?(netzen) → review+
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 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'
Attachment #788956 - Flags: review?(bugs) → review+
(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?
Flags: needinfo?(netzen)
Sony vaio - normal dpi, although it's windows 8.1 so that could be a factor as well.
Flags: needinfo?(netzen)
Updated bug898580-dom.patch to address :smaug's comment. Carrying r+.
Attachment #788956 - Attachment is obsolete: true
Attachment #790273 - Flags: review+
Comment on attachment 790273 [details] [diff] [review]
Part 0 - Add nsIDOMWindowUtils.getViewId(element)

Part 0 - Add nsIDOMWindowUtils.getViewId(element)
Attachment #790273 - Attachment description: bug898580-dom.patch → Part 1 - Add nsIDOMWindowUtils.getViewId(element)
Attachment #790273 - Attachment description: Part 1 - Add nsIDOMWindowUtils.getViewId(element) → Part 0 - Add nsIDOMWindowUtils.getViewId(element)
Attachment #789145 - Attachment description: bug898580.patch → Part 1 - Inform APZC about scroll events on Metro
Attachment #789739 - Attachment description: bug898580-part2.patch → Part 2 - Remove obsoleted code in APZC
Is this work ready to land? It's holding up bug 901607.
(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.
Flags: needinfo?(netzen)
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! :)
Flags: needinfo?(netzen)
Attachment #790273 - Attachment is obsolete: true
Attachment #792839 - Flags: review+
Attachment #789145 - Attachment is obsolete: true
Attachment #792840 - Flags: review+
Attachment #789739 - Attachment is obsolete: true
Attachment #792841 - Flags: review+
Rebased patches, carrying r+.
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?
(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..
(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.
Keywords: checkin-needed
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'
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?
Flags: needinfo?(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.
Flags: needinfo?(philip.chee)
CLOBBER fixed, Thanks.
Depends on: 909482
Depends on: 944362
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.