856264 - Defect - if context menu visible, tapping and holding another link will not produce new context menu

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P3)

Description

[Kamil Jozwiak [:kjozwiak]]

If a context menu is already being displayed, tapping and holding another link will not display a new context menu

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to mozilla.org
3) Tap and hold on the "Mission" link at the top of the page (You will receive the context menu)
4) While the context menu is displayed, tap and hold the "About" link and you will notice that the context menu is not being displayed

Current Behavior:

- If a context menu is already being displayed, tapping and holding another link will not display a new context menu that is associated with the selected link

Expected Behavior:

- When a user taps and holds another link, a new context menu should be displayed (replacing the one that is currently visible)

Comment 1

[Kamil Jozwiak [:kjozwiak]]

consideration for Iteration #6

Comment 2

[Marco Mucci [:MarcoM]]

Moving to Iteration #6 for consideration as a Defect Story.

Comment 3

[Marco Mucci [:MarcoM]]

Jim's point estimate=1

Comment 4

[Marco Mucci [:MarcoM]]

Leaving the point estimate off until it is assigned to an iteration.

Comment 5

[Jonathan Wilde [:jwilde]]

I tested IE Metro's context menu behavior for comparison.

Mouse:
- If no context menu is showing: right-clicking a link shows the context menu.
- If a context menu is showing: right-clicking (or left-clicking) anywhere, including on a link, closes the existing context menu and does not open a new one. Hover events for the page behind the context menu are not fired while the context menu is open.

Touch:
- If no context menu is showing: longpress causes the light gray square to appear before releasing. When you release from the longpress, the context menu shows up.
- If a context menu is showing: tapping or longpress anywhere does not cause the light gray square to appear and dismisses the context menu on release.

Interestingly, IE doesn't open another context menu with another longpress if an existing one is open.

We currently deviate from what IE does in the following key ways:
- While a context menu is open, we still fire hover events.
- If a user does a longpress while a context menu is open, we don't show the context menu, but show the gray square that indicates that you longpressed.
- If a user does a longpress/rightclick while the context menu is open, we still put focus on the new link and sometimes select some text without showing another context menu.

Yuan, should we keep the behavior that the context menu is dismissed when you tap outside of it (and provide more indication that it's going to be closed by disabling the grey square that shows up when you longpress and disable hover events), or switch to more desktop-like norms and enable directly opening another context menu when one is already open? Thanks!

Comment 6

[Jonathan Wilde [:jwilde]]

Talked with Yuan. Here's the plan of action for how context menus should work with links.

If there's no context menu open already:
- Tap and holding shows the grey selection square over the link.
- Releasing shows the context menu.
- Tapping outside of the context menu hides the context menu and causes the link text to be selected.

If there's already a context menu open:
- Tap and holding shows the grey selection square over the link.
- Releasing hides the existing context menu and opens the context menu.
- The text for the old link is not selected.

Comment 7

[Jonathan Wilde [:jwilde]]

s/selection square/"simulated right click" square/

This deviates from what IE does, but makes things more consistent with how desktop Firefox works.

Comment 8

[Jonathan Wilde [:jwilde]]

Okay, this is getting complicated. Let's bump this to a p=5.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jonathan Wilde [:jwilde] from comment #6)
> Talked with Yuan. Here's the plan of action for how context menus should
> work with links.
> 
> If there's no context menu open already:
> - Tap and holding shows the grey selection square over the link.
> - Releasing shows the context menu.
> - Tapping outside of the context menu hides the context menu and causes the
> link text to be selected.

We don't control the built-in selection square, not sure how you plan to do this. Windows draws this for us, so I don't think we should mess with it.

As to the original bug, this should fire the context menu event which should get picked up over in Content.js. That would then forward over to BrowserTouchHelper, at which point you could kill menus or selection and trigger the appropriate action at the new spot.

Comment 10

[Jim Mathies [:jimm]]

I agree with the new priority btw, this is not a p1, it's 'polish'.

Comment 11

[Jonathan Wilde [:jwilde]]

(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Jonathan Wilde [:jwilde] from comment #6)
> > Talked with Yuan. Here's the plan of action for how context menus should
> > work with links.
> > 
> > If there's no context menu open already:
> > - Tap and holding shows the grey selection square over the link.
> > - Releasing shows the context menu.
> > - Tapping outside of the context menu hides the context menu and causes the
> > link text to be selected.
> 
> We don't control the built-in selection square, not sure how you plan to do
> this. Windows draws this for us, so I don't think we should mess with it.

Yup! :) I was just noting that we're leaving that in and not trying to mess with it like IE seems to.

> As to the original bug, this should fire the context menu event which should
> get picked up over in Content.js. That would then forward over to
> BrowserTouchHelper, at which point you could kill menus or selection and
> trigger the appropriate action at the new spot.

In my in-progress patch, I've made some tweaks so that the Context UI in chrome reuses the existing context menu in another location. That was pretty straightforward and also dramatically reduces a number of other bits of form <select> jankiness in other bugs.

The bit I'm stuck on is trying to figure out a non-fugly way to do that final selection when the user exits the context menu.

Comment 12

[Jonathan Wilde [:jwilde]]

(In reply to Jonathan Wilde [:jwilde] from comment #11)
> The bit I'm stuck on is trying to figure out a non-fugly way to do that
> final selection when the user exits the context menu.

I've figured out that I can trigger it via a click handler in Content.js, but I'm trying to decide whether that's a sane way to do things.

Comment 13

[Jonathan Wilde [:jwilde]]

Attachment: patch WIP0

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jonathan Wilde [:jwilde] from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > (In reply to Jonathan Wilde [:jwilde] from comment #6)
> > > Talked with Yuan. Here's the plan of action for how context menus should
> > > work with links.
> > > 
> > > If there's no context menu open already:
> > > - Tap and holding shows the grey selection square over the link.
> > > - Releasing shows the context menu.
> > > - Tapping outside of the context menu hides the context menu and causes the
> > > link text to be selected.
> > 
> > We don't control the built-in selection square, not sure how you plan to do
> > this. Windows draws this for us, so I don't think we should mess with it.
> 
> Yup! :) I was just noting that we're leaving that in and not trying to mess
> with it like IE seems to.

I think there's a way to turn this feedback off if we really want to draw our own. I wouldn't suggest working on that for v1 though.

Comment 15

[Jonathan Wilde [:jwilde]]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Jonathan Wilde [:jwilde] from comment #11)
> > (In reply to Jim Mathies [:jimm] from comment #9)
> > > (In reply to Jonathan Wilde [:jwilde] from comment #6)
> > > > Talked with Yuan. Here's the plan of action for how context menus should
> > > > work with links.
> > > > 
> > > > If there's no context menu open already:
> > > > - Tap and holding shows the grey selection square over the link.
> > > > - Releasing shows the context menu.
> > > > - Tapping outside of the context menu hides the context menu and causes the
> > > > link text to be selected.
> > > 
> > > We don't control the built-in selection square, not sure how you plan to do
> > > this. Windows draws this for us, so I don't think we should mess with it.
> > 
> > Yup! :) I was just noting that we're leaving that in and not trying to mess
> > with it like IE seems to.
> 
> I think there's a way to turn this feedback off if we really want to draw
> our own. I wouldn't suggest working on that for v1 though.

We don't need to draw our own. :D I talked with Yuan and we actually want to deviate with the IE behavior (and be more aligned with desktop Firefox), so leaving in the square as it was before is exactly what we end up wanting.

Comment 16

[Jonathan Wilde [:jwilde]]

Attachment: patch WIP1 - fails tests currently

Comment 17

[Jonathan Wilde [:jwilde]]

Attachment: patch v1

Comment 18

[Jonathan Wilde [:jwilde]]

Attachment: patch v1

Sorry for the bugspam. Previous patch was incorrect.

Comment 19

[Jonathan Wilde [:jwilde]]

Attachment: patch v1.1

Cleanup.

Comment 20

[Jonathan Wilde [:jwilde]]

Attachment: patch v1.2

massively simplifying this

Comment 21

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a49288dc2d0

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Asa Dotzler [:asa]]

This doesn't seem to be fixed for me in today's nightly build which contains the patch. When I have one link context menu popped up and  long press on another,  nothing happens -- the first menu just persists.

Comment 24

[Marco Mucci [:MarcoM]]

Hi Juan, please see Comment #23 from Asa.  Can you confirm if the defect has been fixed.  If not, you can reopen this defect as it is part of the current iteration.

Comment 25

[juan becerra [:juanb]]

Tested on 2013-07-17 using latest nightly built from http://hg.mozilla.org/mozilla-central/rev/0888e29c83a3

This defect has not been fixed. If there's an existing context menu open, long press on a different link will not bring up this other context menu. This is different from the behavior that happens when you use the mousepad.

See comment #0 for details.

Comment 26

[Jonathan Wilde [:jwilde]]

Attachment: failing testcase

Comment 27

[Jonathan Wilde [:jwilde]]

Attachment: fix for failing testcase

Comment 28

[Jonathan Wilde [:jwilde]]

Comment on attachment 777584 [details] [diff] [review]
fix for failing testcase

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

::: browser/metro/base/content/helperui/MenuUI.js
@@ +351,5 @@
> +      this._isPopupMoving = this._visible;
> +      yield this.hide();
> +      yield this._show(aPositionOptions);
> +      this._isPopupMoving = false;
> +    }.bind(this));

Since the event listeners were extremely hard to follow. I've decided to switch this over to using promises.

Comment 29

[Jim Mathies [:jimm]]

These two patches fix the problem for me, however when I tap on content, the menu doesn't hide. Appear this was introduced by this work, I can't reproduce without these patches.

Comment 30

[Jonathan Wilde [:jwilde]]

Attachment: menufix-fix-v3.0

Comment 31

[Jonathan Wilde [:jwilde]]

Attachment: menufix-test-v3.1

Comment 32

[Jonathan Wilde [:jwilde]]

Attachment: menufix-fix-v3.1

Comment 33

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #29)
> These two patches fix the problem for me, however when I tap on content, the
> menu doesn't hide. Appear this was introduced by this work, I can't
> reproduce without these patches.

This is fixed in the new rev.

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 784672 [details] [diff] [review]
menufix-test-v3.1

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

::: browser/metro/base/tests/mochitest/browser_context_menu_tests.js
@@ +28,4 @@
>      "Top position is " + aElement.top + ", expected between " + aMinTop + " and " + aMaxTop);
>  }
>  
> +function sendContextMenuMouseClickToElement(aWindow, aElement, aX, aY) {

Would this be useful in head.js?

@@ +37,5 @@
> +  utils.sendMouseEventToWindow("mouseup", coords.x, coords.y, 2, 1, 0);
> +  utils.sendMouseEventToWindow("contextmenu", coords.x, coords.y, 2, 1, 0);
> +}
> +
> +function sendMouseClick(aWindow, aX, aY) {

ditto?

@@ +656,5 @@
> +
> +  try {
> +    yield waitForCondition(function () {
> +      return ContextUI.isVisible;
> +    }, 500, 50);

I think the default timeout/poll values here should suffice.

@@ +669,5 @@
> +  yield promise;
> +  ok(!ContextMenuUI._menuPopup.visible, "popup is actually hidden");
> +
> +  Browser.closeTab(Browser.selectedTab, { forceClose: true });
> +  delete window.r;

no longer needed, mbrubeck found the cause of this in my selection code. :)

Comment 35

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/fx-team/rev/79659e804439
https://hg.mozilla.org/integration/fx-team/rev/b3c93b2281fc

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/79659e804439
https://hg.mozilla.org/mozilla-central/rev/b3c93b2281fc

Comment 37

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #12 without any issues. Used the following issue:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-13-03-02-05-mozilla-central/

- Went through the original test case in comment #0 without any issues
- Tapped on several links in a row (10 or so) and ensured that the context menu appeared every single time without any issues
- Ensured that all the context menu items worked without any issues ("Open link in new Tab", "Copy Link" & "Bookmark Link")
- Ensured that you still receive context menu's when you have text selected and then tap on links ("Copy", "Search Bing..", "Open link in new tab" & "Copy Link). Also ensured those items in the context menu worked without any issues
- Went through the above test cases in Filled View without any issues

Comment 38

[Kamil Jozwiak [:kjozwiak]]

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

- Went through the original test cases from comment #0 without any issues
- Went through all of the test cases added in comment #37 without any issues

Comment 39

[Kamil Jozwiak [:kjozwiak]]

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

- Went through the original test cases from comment #0 without any issues
- Went through all of the test cases added in comment #37 without any issue