Clicks to close a context-menu in a panel are also consumed by the panel

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: djcater+bugzilla, Assigned: mconley)

Tracking

(Blocks: 1 bug, {regression, ux-consistency})

Trunk
mozilla19
regression, ux-consistency
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120418 Firefox/14.0a1

When right-clicking a download in the popup, and the left clicking outside of the context menu (to dismiss it), if that click is on a download then it will activate that download.

This is not the behaviour of any other piece of UI that I have tested in the browser (and in webpages as well). For example, right-click on the toolbar (as if customising) and then click on any button, such as the home button. The context menu is dismissed, but the button is not activated.

The same goes for right-clicking in a web page and then left-clicking on a link. It doesn't follow the link.

Many years (maybe decades?) of right-clicking has left me with the muscle memory that left-clicking with a context menu open won't activate whatever it is that is clicked.

There is also a strange hover issue with the context menu open. Sometimes the underlying item has the hover effect and sometimes it doesn't. A good way to reproduce this is to right-click, then right-click again on a different part of the item. Then moving the mouse doesn't always activate the hover effect.
Component: General → Downloads Panel
QA Contact: general → downloads.panel

Comment 1

6 years ago
(In reply to Daniel Cater from comment #0)
> When right-clicking a download in the popup, and the left clicking outside
> of the context menu (to dismiss it), if that click is on a download then it
> will activate that download.
> 
> This is not the behaviour of any other piece of UI that I have tested in the
> browser (and in webpages as well). For example, right-click on the toolbar
> (as if customising) and then click on any button, such as the home button.
> The context menu is dismissed, but the button is not activated.

In general, the behavior I'm observing in the user interface is not consistent,
with regard to whether a click made outside of an active popup menu or panel is
"eaten" or not. The most common case, for me, is that the click is not eaten, for
example opening the page's context menu and the clicking on a link in the page
before closing the menu activates the link for me.

If you open a panel and then click on a link on the current page, instead, the
click is eaten and the link is not activated. I'm on Linux as well.

So, this user interface issue is probably not specific to the Downloads Panel.
(Reporter)

Comment 2

6 years ago
(In reply to Paolo Amadini [:paolo] from comment #1)
> (In reply to Daniel Cater from comment #0)
> > When right-clicking a download in the popup, and the left clicking outside
> > of the context menu (to dismiss it), if that click is on a download then it
> > will activate that download.
> > 
> > This is not the behaviour of any other piece of UI that I have tested in the
> > browser (and in webpages as well). For example, right-click on the toolbar
> > (as if customising) and then click on any button, such as the home button.
> > The context menu is dismissed, but the button is not activated.
> 
> In general, the behavior I'm observing in the user interface is not
> consistent,
> with regard to whether a click made outside of an active popup menu or panel
> is
> "eaten" or not. The most common case, for me, is that the click is not
> eaten, for
> example opening the page's context menu and the clicking on a link in the
> page
> before closing the menu activates the link for me.
> 
> If you open a panel and then click on a link on the current page, instead,
> the
> click is eaten and the link is not activated. I'm on Linux as well.
> 
> So, this user interface issue is probably not specific to the Downloads
> Panel.

Interesting, clicking a link with the context menu open doesn't active the link for me, nor has it ever done as far as I can remember. I haven't yet found another piece of UI, either within Firefox or within the rest of Ubuntu, that behaves like the downloads panel does.

Which distro and desktop environment are you using?

Comment 3

6 years ago
(In reply to Daniel Cater from comment #2)
> Which distro and desktop environment are you using?

At present, Linux Mint (Ubuntu based) with Gnome Classic.

Updated

6 years ago
Blocks: 773273
Blocks: 747422
(Assignee)

Comment 4

6 years ago
I'm able to reproduce this with a different panel in Firefox on both Ubuntu and Windows 7.

STR:

1) For any webpage, click on the bookmark star in the Awesome Bar
2) Click on the star again to bring up the bookmark details panel.
3) Right-click on the tags text input to bring up the Cut/Copy/Paste context menu.
4) Without closing the context menu, position your mouse over "Remove Bookmark" and click.

What happens?

The click falls through, the button is clicked, and the bookmark is removed.

What is expected?

Like on OSX, I expect the first click to close the context menu.
(Assignee)

Comment 5

6 years ago
I do believe this is a platform bug.
Component: Downloads Panel → Widget: Win32
Product: Firefox → Core
Summary: Downloads popup has strange behaviour with context menu → Clicks to close a context-menu in a panel are also consumed by the panel
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
OS: Linux → Windows 7
(Assignee)

Comment 6

6 years ago
I just grabbed a copy of Minefield from Jan 15, 2010 - sometime around when arrow panels were introduced.

The problem does not exist there with the Edit Bookmarks panel, so I think we've regressed sometime between then and now. I'll see if I can nail it down.
Keywords: regression, regressionwindow-wanted
(Assignee)

Comment 7

6 years ago
Here's the regression range. My money is on Bug 511010 introducing the regression. Doing a build to make sure.
(Assignee)

Comment 8

6 years ago
We have a winner - bug 511010 introduced this regression.
Blocks: 511010
Keywords: regressionwindow-wanted
A simple fix might be to change:

-                *outResult = MA_NOACTIVATE;
+                *outResult = popupsToRollup != UINT32_MAX ? MA_NOACTIVATEANDEAT : MA_NOACTIVATE;

Although you might be able to simply the code a bit with the later block that also assigns MA_NOACTIVATEANDEAT.
(Assignee)

Comment 10

6 years ago
Created attachment 673919 [details] [diff] [review]
WIP Patch 1

Something like this?

This seems to fix the issue on Windows.
Attachment #673919 - Flags: feedback?(enndeakin)
(Assignee)

Comment 11

6 years ago
(I'll note that while this fixes things for Windows, Gtk still appears to be broken)
Comment on attachment 673919 [details] [diff] [review]
WIP Patch 1

Something like that yes, but the first two are already inside a 'popupsToRollup == UINT32_MAX' conditional.

I haven't tested it though, so I'd want to verify that it works with various popup configurations.
Comment on attachment 673919 [details] [diff] [review]
WIP Patch 1

But the one line change works ok
Attachment #673919 - Flags: feedback?(enndeakin) → feedback+
(Assignee)

Comment 14

6 years ago
Created attachment 674822 [details] [diff] [review]
Fix for Windows

Ok, I can confirm that this fixes the issue for Windows.

Any idea what I have to do to fix this for Gtk2? Something in here? mxr.mozilla.org/comm-central/source/mozilla/widget/gtk2/nsWindow.cpp
Assignee: nobody → mconley
Attachment #673919 - Attachment is obsolete: true
Attachment #674822 - Flags: review?(enndeakin)
Comment on attachment 674822 [details] [diff] [review]
Fix for Windows

Since I suggested the change, I shouldn't review it.
Attachment #674822 - Flags: review?(enndeakin) → review?(jmathies)
(Assignee)

Comment 16

6 years ago
Hey Karl - any idea how we might fix this for Gtk2?
(Assignee)

Updated

6 years ago
Flags: needinfo?(karlt)
It's not consuming the rollup event because check_for_rollup is saying the event didn't cause rollup.  That is due to the popupsToRollup == UINT32_MAX test here:
http://hg.mozilla.org/mozilla-central/rev/74959aad851c#l12.67
Looks like that test shouldn't be there, but check bug 404766 to see why it was added.
Flags: needinfo?(karlt)

Updated

6 years ago
Attachment #674822 - Flags: review?(jmathies) → review+
(Assignee)

Comment 18

6 years ago
(In reply to Karl Tomlinson (:karlt) from comment #17)
> It's not consuming the rollup event because check_for_rollup is saying the
> event didn't cause rollup.  That is due to the popupsToRollup == UINT32_MAX
> test here:
> http://hg.mozilla.org/mozilla-central/rev/74959aad851c#l12.67
> Looks like that test shouldn't be there, but check bug 404766 to see why it
> was added.

I can't seem to find a reason why that test was added.

Neil - you originally wrote that patch. Do you remember?
Flags: needinfo?(enndeakin)
Seems like it could be removed. While you have a menu or panel open, does the same behaviour exist when clicking on another menu? Similar for right-clicking?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 20

6 years ago
(In reply to Neil Deakin from comment #19)
> Seems like it could be removed. While you have a menu or panel open, does
> the same behaviour exist when clicking on another menu? Similar for
> right-clicking?

So, while the change that karlt suggested fixes the case I described for both the Downloads Panel and the Edit Bookmark panel, I still see a problem on Linux with the Bookmarks Panel (triggered by the Bookmarks button, distinct from the Edit Bookmark panel).

STR:

1. Click on Bookmark button, revealing big list of bookmarks
2. Right click on any bookmark revealing context menu
3. Move mouse outside of context menu, and click any bookmark

What happens?

User is taken to bookmark, and both context menu and Bookmarks panel are closed.

What's expected?

Click should have closed context menu.
(Assignee)

Comment 21

6 years ago
Same applies for Windows, even with the attached patch. Not sure if we want to treat that as a separate bug though, since it sounds like a different code path.
(Assignee)

Comment 22

6 years ago
Created attachment 675700 [details] [diff] [review]
Fix for Linux

Asking for review from Enn, since it was karlt who recommended the fix in the first place. Let me know if there's someone more appropriate to direct this to.
Attachment #675700 - Flags: review?(enndeakin)
(In reply to Mike Conley (:mconley) from comment #22)
> since it was karlt who recommended the fix in the first place.

Just clarifying that I didn't really mean to recommend that fix.  Perhaps it is appropriate, but I haven't really worked out the submenu code there.  If Enn feels comfortable that the test can be removed and not adversely affect the intentions in bug 404766, then I'm happy to go with that.
Created attachment 676004 [details] [diff] [review]
log CaptureRollupEvents parameters

I don't know how to make the bookmark button appear to test comment 20, but this patch adds logging to check that aConsumeRollupEvent is set.
Attachment #676004 - Flags: review?(roc)
(Assignee)

Comment 25

6 years ago
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Created attachment 676004 [details] [diff] [review]
> log CaptureRollupEvents parameters
> 
> I don't know how to make the bookmark button appear to test comment 20, but
> this patch adds logging to check that aConsumeRollupEvent is set.

The Bookmark button is in the defaultset, and looks like a book with a star on it. If you're using Ubuntu, it's possible this button is being hidden by the "Global Menubar" integration add-on that ships with Firefox by default when installed via Canonicals package repository. If this is the case, disabling the add-on and restarting Firefox should reveal the button.
(Assignee)

Comment 26

6 years ago
(Note that after you disabling the add-on, you may also have to hide the menubar in order to make the Bookmark button appear)
(In reply to Mike Conley (:mconley) from comment #20)
> STR:
> 
> 1. Click on Bookmark button, revealing big list of bookmarks
> 2. Right click on any bookmark revealing context menu
> 3. Move mouse outside of context menu, and click any bookmark
> 
> What happens?
> 
> User is taken to bookmark, and both context menu and Bookmarks panel are
> closed.
> 
> What's expected?
> 
> Click should have closed context menu.

That seems to be a different issue.  The rollup is happening on button press, but the button release is enough to select the bookmark, even when no button press is sent to the bookmark menu.
Comment on attachment 675700 [details] [diff] [review]
Fix for Linux

This should be good, and is consistent with other platforms.

When Rollup returns true, the event should be consumed. If popupsToRollup it UINT32_MAX means that the click was outside a popup such as on the main window or desktop. When popupsToRollup is not UINT32_MAX but has a value of 'x', then it 'x' number of popups should be rolled up and the click was on a popup/panel of a different type that was higher up. For example, a click on a panel while a menulist or context menu is open.
Attachment #675700 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 29

6 years ago
Cool - thanks Neil.

If it's alright with you all, I'm going to fold the two fixes into a single patch before committing. I'll also commit Karl's logging fix (separately).
(Assignee)

Comment 30

6 years ago
Hrm - building with Karl's patch fails - aConsumeRollupEvent is no longer declared in that function. Perhaps it should be aDoCapture now?

I'll not commit it for now.
The aConsumeRollupEvent argument isn't supplied and cached any more; it is now looked up on demand and returned by the Rollup() method.
(Assignee)

Comment 32

6 years ago
Windows and Linux fixes landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b933d6790d
(Assignee)

Comment 33

6 years ago
Not sure what you want to do about your logging, Karl - I'll leave that to you whether or not you want to take care of that here, or in another bug, or drop it entirely.
(Assignee)

Updated

6 years ago
Attachment #676004 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b3b933d6790d

Should this have a test?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 35

6 years ago
If the widget folks wish it / thought it was worth it, I could spend some time on a test, yes.
We don't currently have any tests for rolling up as it requires injecting native mouse events. nsDOMWindowUtils::SynthesizeNativeMouseEvent might work but it isn't fully implemented on linux.
(In reply to Karl Tomlinson (:karlt) from comment #27)
> (In reply to Mike Conley (:mconley) from comment #20)
> > STR:
> > 
> > 1. Click on Bookmark button, revealing big list of bookmarks
> > 2. Right click on any bookmark revealing context menu
> > 3. Move mouse outside of context menu, and click any bookmark

> That seems to be a different issue.  The rollup is happening on button
> press, but the button release is enough to select the bookmark, even when no
> button press is sent to the bookmark menu.

Other GTK apps roll up on button release.
(In reply to Mike Conley (:mconley) from comment #30)
> Hrm - building with Karl's patch fails - aConsumeRollupEvent is no longer
> declared in that function. Perhaps it should be aDoCapture now?

Just logging aDoCapture:
https://hg.mozilla.org/integration/mozilla-inbound/rev/543b9865a352

Updated

5 years ago
Component: Widget: Win32 → Widget
OS: Windows 7 → All
You need to log in before you can comment on or make changes to this bug.