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

RESOLVED FIXED in Firefox 26

Status

defect
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kjozwiak, Assigned: jwilde)

Tracking

unspecified
Firefox 26
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5)

Attachments

(3 attachments, 8 obsolete attachments)

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)
consideration for Iteration #6
Flags: needinfo?(mmucci)
Moving to Iteration #6 for consideration as a Defect Story.
Flags: needinfo?(mmucci)
Priority: -- → P1
QA Contact: jbecerra
Summary: if context menu visible, tapping and holding another link will not produce new context menu → Defect - if context menu visible, tapping and holding another link will not produce new context menu
Whiteboard: feature=defect c=context_menus u=metro_firefox_user p=0
Blocks: metrov1planning
No longer blocks: metrov1it6
Blocks: metrov1defect&change
No longer blocks: metrov1planning
Whiteboard: feature=defect c=context_menus u=metro_firefox_user p=0 → feature=defect c=context_menus u=metro_firefox_user p=1
Jim's point estimate=1
Whiteboard: feature=defect c=context_menus u=metro_firefox_user p=1 → feature=defect c=context_menus u=metro_firefox_user p=0
Priority: P1 → --
Priority: -- → P3
Whiteboard: feature=defect c=context_menus u=metro_firefox_user p=0 → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=1
Leaving the point estimate off until it is assigned to an iteration.
Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=1 → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=0
Assignee: nobody → jwilde
Status: NEW → ASSIGNED
Blocks: metrov1it9
No longer blocks: metrov1defect&change
Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=0 → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=1
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!
Flags: needinfo?(ywang)
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.
Flags: needinfo?(ywang)
s/selection square/"simulated right click" square/

This deviates from what IE does, but makes things more consistent with how desktop Firefox works.
Blocks: metrov1it10
No longer blocks: metrov1it9
Okay, this is getting complicated. Let's bump this to a p=5.
Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=1 → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5
(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.
I agree with the new priority btw, this is not a p1, it's 'polish'.
(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.
(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.
Posted patch patch WIP0 (obsolete) — Splinter Review
(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.
(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.
Blocks: 888521
Attachment #767980 - Attachment is obsolete: true
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #775062 - Attachment is obsolete: true
Attachment #775063 - Attachment is patch: true
Attachment #775063 - Attachment mime type: text/x-patch → text/plain
Posted patch patch v1 (obsolete) — Splinter Review
Sorry for the bugspam. Previous patch was incorrect.
Attachment #775063 - Attachment is obsolete: true
Posted patch patch v1.1 (obsolete) — Splinter Review
Cleanup.
Attachment #775064 - Attachment is obsolete: true
Blocks: metrov1it11
No longer blocks: metrov1it10
Posted patch patch v1.2Splinter Review
massively simplifying this
Attachment #775065 - Attachment is obsolete: true
Attachment #775069 - Flags: review?(jmathies)
Attachment #775069 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/4a49288dc2d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
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.
Flags: needinfo?(jbecerra)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(jbecerra)
Resolution: FIXED → ---
Posted patch failing testcase (obsolete) — Splinter Review
Attachment #777582 - Flags: review?(jmathies)
Posted patch fix for failing testcase (obsolete) — Splinter Review
Attachment #777584 - Flags: review?(jmathies)
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.
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.
Attachment #777582 - Flags: review?(jmathies) → review+
Blocks: metrov1it12
No longer blocks: metrov1it11
Status: REOPENED → ASSIGNED
Posted patch menufix-fix-v3.0 (obsolete) — Splinter Review
Attachment #777584 - Attachment is obsolete: true
Attachment #777584 - Flags: review?(jmathies)
Attachment #784667 - Flags: review?(jmathies)
Attachment #784667 - Flags: review?(jmathies)
Attachment #777582 - Attachment is obsolete: true
Attachment #784672 - Flags: review?(jmathies)
Attachment #784667 - Attachment is obsolete: true
Attachment #784674 - Flags: review?(jmathies)
(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 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. :)
Attachment #784672 - Flags: review?(jmathies) → review+
Attachment #784674 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/fx-team/rev/79659e804439
https://hg.mozilla.org/integration/fx-team/rev/b3c93b2281fc
Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5 → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/79659e804439
https://hg.mozilla.org/mozilla-central/rev/b3c93b2281fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5[fixed-in-fx-team] → [shovel-ready] feature=defect c=context_menus u=metro_firefox_user p=5
Target Milestone: --- → Firefox 26
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
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
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
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.