Closed
Bug 746775
Opened 13 years ago
Closed 12 years ago
Clicks to close a context-menu in a panel are also consumed by the panel
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: djcater+bugzilla, Assigned: mconley)
References
(Blocks 1 open bug)
Details
(Keywords: regression, ux-consistency)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
Component: General → Downloads Panel
Updated•13 years ago
|
QA Contact: general → downloads.panel
Comment 1•13 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.
(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•13 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•12 years ago
|
Blocks: ReleaseDownloadsPane
Assignee | ||
Comment 4•12 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•12 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•12 years ago
|
OS: Linux → Windows 7
Assignee | ||
Comment 6•12 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•12 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•12 years ago
|
||
We have a winner - bug 511010 introduced this regression.
Blocks: 511010
Keywords: regressionwindow-wanted
Comment 9•12 years ago
|
||
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•12 years ago
|
||
Something like this?
This seems to fix the issue on Windows.
Attachment #673919 -
Flags: feedback?(enndeakin)
Assignee | ||
Comment 11•12 years ago
|
||
(I'll note that while this fixes things for Windows, Gtk still appears to be broken)
Comment 12•12 years ago
|
||
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 13•12 years ago
|
||
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•12 years ago
|
||
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 15•12 years ago
|
||
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•12 years ago
|
||
Hey Karl - any idea how we might fix this for Gtk2?
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(karlt)
Comment 17•12 years ago
|
||
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•12 years ago
|
Attachment #674822 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 18•12 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)
Comment 19•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
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)
Comment 23•12 years ago
|
||
(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.
Comment 24•12 years ago
|
||
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)
Attachment #676004 -
Flags: review?(roc) → review+
Assignee | ||
Comment 25•12 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•12 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)
Comment 27•12 years ago
|
||
(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 28•12 years ago
|
||
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•12 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•12 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.
Comment 31•12 years ago
|
||
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•12 years ago
|
||
Windows and Linux fixes landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b933d6790d
Assignee | ||
Comment 33•12 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•12 years ago
|
Attachment #676004 -
Attachment is obsolete: true
Comment 34•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3b933d6790d
Should this have a test?
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 35•12 years ago
|
||
If the widget folks wish it / thought it was worth it, I could spend some time on a test, yes.
Comment 36•12 years ago
|
||
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.
Comment 37•12 years ago
|
||
(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.
Comment 38•12 years ago
|
||
(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
Comment 39•12 years ago
|
||
Updated•11 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.
Description
•