922934 - Defect - Context menu's from links not clearing when scrolling

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: Context Menu not clearing when scrolling

When a user receives a context menu by tapping & holding on a link, scrolling through the website will not clear the context menu

- Attached a screenshot to illustrate the issue (opened that context menu at the top of the Wiki then scrolled near the bottom of the page)

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to wikipedia.org
3) Tap & hold on a link and let go (you should receive the context menu)
4) Scroll through the website by swiping down and you'll notice that the context menu is not being cleared

Current Behavior:

- Context menu from tapping & holding on link will not dismiss when scrolling through the website

Expected Behavior:

- Context menu should be cleared when the user scrolls through the website

Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-01-03-02-04-mozilla-central/

Comment 1

[Matt Brubeck (:mbrubeck)]

This is a regression from enabling async scrolling.  The context menu code listens for a "PanBegin" event which was sent by our old JavaScript panning code, but this is not sent when using AsyncPanZoomController:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/MenuUI.js#520

We can fix this by listening for the apzc-handle-pan-begin notification instead.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Even better, we can just use standard events (mousedown, touchstart, scroll) to dismiss the context menu.  The advantage is that these work in cases like pinch zooming, double-tap zooming, mousewheel scroll, etc. where PanBegin and handle-apzc-pan-begin do not.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: test

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

I noticed a bug in the previous patch: We need to check the target of the "scroll" event too, in case we are scrolling the menu itself (e.g. for a very long <select> menu).

Comment 5

[Marina Samuel [:emtwo]]

Comment on attachment 813179 [details] [diff] [review]
patch v2

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

Looks good! Work well.

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/ca56f76237df

Comment 7

[Matt Brubeck (:mbrubeck)]

Backed out because of test failures:
https://hg.mozilla.org/integration/fx-team/rev/8f1cdf02deb8

Comment 8

[Matt Brubeck (:mbrubeck)]

Found a simple fix for the test failure:
https://tbpl.mozilla.org/?tree=Try&rev=c46b84d3a75b

Re-landed with the fix:
https://hg.mozilla.org/integration/fx-team/rev/4be0c42fc81a

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ca56f76237df

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/4be0c42fc81a

Comment 11

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #16 without issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-16-03-02-02-mozilla-central/

- Went through the original test cases from comment #0 without any issues
- Ensured scrolling will dismiss the context menu (scrolling up & down)
- Ensured that tapping on the screen will dismiss the context menu
- Ensured that the context menu is being dismissed when sliding the Navigation App Bar from the bottom
- Ensured that the context menu is being dismissed when sliding the Tab App Bar from the top
- Ensured that the context menu is being dismissed when tapping the junior style "Back" button
- Ensured that the context menu is being dismissed when tapping on the junior style "+" button
- Ensured that both link & image context menu's are being cleared
- Ensured that all of the above test cases worked in both filled and full views without issues