User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10 Well lets see i open the bookmarks menu and go to the livebookmark of slashdot there are 2 news i would like to view but if i press the third mouse buttom(open in tab the link) then the menu will close and i will need to reopen again the live bookmark and press the another link i would like to see, can remain the menu open when i press the open in tab button? Reproducible: Always Steps to Reproduce: 1. 2. 3.

MSIE allows the bookmarks menu to remain open if the Shift key is pressed while clicking on a bookmark. For Firefox, maybe we can have the menu remain open if a bookmark is middle-clicked and the Ctrl key is pressed. ==FILE== jar://./browser.js!/content/browser/bookmarks/bookmarksMenu.js ==OLD CODE== var BookmarksMenu = { ... loadBookmarkMiddleClick: function (aEvent, aDS) { if (aEvent.button != 1) return; // unlike for command events, we have to close the menus manually BookmarksMenuDNDObserver.mCurrentDragOverTarget = null; BookmarksMenuDNDObserver.onDragCloseTarget(); this.loadBookmark(aEvent, aEvent.target, aDS); } } ==== ==NEW CODE== var BookmarksMenu = { ... loadBookmarkMiddleClick: function (aEvent, aDS) { if (aEvent.button != 1) return; if (!aEvent.ctrlKey) { // unlike for command events, we have to close the menus manually BookmarksMenuDNDObserver.mCurrentDragOverTarget = null; BookmarksMenuDNDObserver.onDragCloseTarget(); } this.loadBookmark(aEvent, aEvent.target, aDS); } } ==== I would have made this into a patch, but I don't know how to use CVS with Mozilla's trunks or branches.

*** Bug 306769 has been marked as a duplicate of this bug. ***

*** Bug 314925 has been marked as a duplicate of this bug. ***

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Not a blocker, but if someone wants to take that code and make it into a patch and get it reviewed, I'd consider approving it. It's the little things, after all.

Nominating for blocking‑firefox3.1 now that we have some time.

Still not a blocker.

I really support this request about allowing to change this annoying default behavior.

FWIW - there's an extension called "Stay-Open Menu" that does exactly this: https://addons.mozilla.org/en-US/firefox/addon/6459

allow relevant for bookmark folders

Implements the proposal from comment 1.

Adam, you should ask for a review to a module owner. Ms2ger who should review this patch?

Comment on attachment 570353 [details] [diff] [review] Patch implements behavior to use Ctrl+Middle-click. I assume Gavin can.

We could also leave the bookmark menu open on middle-click without CTRL key pressed, as we do with every link.

Comment on attachment 570353 [details] [diff] [review] Patch implements behavior to use Ctrl+Middle-click. Review of attachment 570353 [details] [diff] [review]: ----------------------------------------------------------------- I think we should do this for any "open in new tab" handling, that is any middleclick combination and ctrl+leftclick. I also think that we should keep closing the menu when middle clicking on containers, since there is a pretty obvious use-case that is expressed in the same code, that is not covering the "you are about to open X tabs" dialog.

This patch adds the attribute 'closemenu="none"' to bookmark menuitems. In onCommand it checks if the event is a command event (ie, not calling onCommand from the click event handler) and will open in a new tab (so it covers both ctrl[+shift]+enter and ctrl[+shift]+click), and it will close the menu in that case. As it only adds the attribute to menuitems and not menus, command events on menus close the menupopup automatically. Click events on menus have their own check that causes the menupopup to close. It also splits out the menupopup closing logic for use by both event handlers.

I'll look at this tomorrow.

(In reply to Mano from comment #21) > I'll look at this tomorrow. Any news?

1. I'm not sure I agree with Marco's comment about generalizing this feature to any-new-tab-operation. This is definitely going to annoy *some* users (well, me). Of course, if I choose Open in New Tab from the context menu for an item, the only menu that should close is the context menu itself (as we do for Cut/Copy). However, for "implicit" open in new tab request, there's no clear cut, in my opinion. *For some reason*, I tend to think that Ctrl/Cmd+click should keep the menu open, but that middle click cannot be so different in functionality from regular click. Maybe it's just me. Anyway, it's not my decision to make, at all. I suggest you ask for ui-review here before continuing the technical work. You can ask for ui-review either on the patch itself, or by setting the uiwatned keyword on the bug. 2. The Original Code isn't so great (that is, the fact that we used to manually close menus...), but I think this change actually makes things even worse. The closemenu attribute kind-of-defines the *default* behavior, whereas in your patch it's used to define the special case, forcing us to manually close the popup even for regular clicks. With all that said, I'm not saying there's better solution. You should test to see if using preventDefault (in the command event (or maybe in the click event) does the job of preventing the menu hiding. I would test it for you, but right now I'm on OS X, and the behavior of mouse clicks within menu is somewhat different here. 3. Please make sure that when using the context menu commands, the menu doesn't hide either. 4. Speaking of OS X, whoever decides what's the desired behavior here should also decide if any of this magic is wanted on OS X (and at the same time decide once and for all about context menu for bookmarks on OS X). Canceling the request for now. I'm sorry this took longer than I planned!

http://www.shadetreeapiary.com/extension/index.htm the functions(all) of this extensions are a must(plus providing all the options provided by the addon in a gui form so that all functionality is easily accessible ) the developer is also happy to to lend a hand(contact page)

We need the opinion of the UI team here to make a decision.

(In reply to magnumarchonbasileus from comment #25) > the developer is also happy to to lend a hand For what reason did you state that? I have never made any such remark.

(In reply to custom.firefox.lady from comment #27) > For what reason did you state that? I have never made any such remark. sorry for bad english i meant the developers are But can you help too?

I am the developer of Stay-Open Menu mentioned in comment #10 and which is reported not to work on the Mac native menubar. Is the patch in this bug known to work on the menubar on Mac OS X? Thanks.

(re: comment #29) Sorry, nevermind, this bug's fix would be pointless on the Mac menubar due to Bug 313718 and Bug 490002.

Who can uireview the patch?

Not sure at this point.

Marking as [qx], since there's some work needed to un-bitrot the patch (doesn't apply cleanly anymore)

Can someone add ctrl-click to the title so that this bug is found by the duplicate check in bug submission.

We still need UX input here for the below review comments(Comment 24). Does anyone know who can uireview the behavior? > 1. I'm not sure I agree with Marco's comment about generalizing this feature > to any-new-tab-operation. This is definitely going to annoy *some* users > (well, me). Of course, if I choose Open in New Tab from the context menu > for an item, the only menu that should close is the context menu itself (as > we do for Cut/Copy). However, for "implicit" open in new tab request, > there's no clear cut, in my opinion. *For some reason*, I tend to think that > Ctrl/Cmd+click should keep the menu open, but that middle click cannot be so > different in functionality from regular click. Maybe it's just me. > > Anyway, it's not my decision to make, at all. I suggest you ask for > ui-review here before continuing the technical work. You can ask for > ui-review either on the patch itself, or by setting the uiwatned keyword on > the bug.

Add this issue into QX list.

:shorlander is probably the right person to ask, but given his plate is pretty full, let's not actively bombard him with needinfo, before we are sure we are going to actively work on this feature.

Sure, got it.

Add into the "Make bookmarks easier to understand" QX cluster.

Move to QX holding area to consider the priority.

After checking in with Philipp Sackl [:phlsa] we agreed that for both, middle-click and Ctrl/Cmd+click the behavior should be the same as on a website - open a tab in the background and leave the user in their current context. Same behavior for same actions. So let's keep bookmark menus open on middle-click and Ctrl/Cmd+click and open a tab in the background.

Markus, thanks for the inputs. I would like to give it a shot.

Not active on this. Unassign myself.

As the dev of the Stay-Open Menu ext (and no way to port it to webextensions), I'd like to attempt a fix for this bug. (It would be my first bugfix.) Ok to just take the (very) old patch and revise? Some reluctance was expressed in comment#24 about the method for implementing Ctrl-click. Are we ok with that method? I don't know of a better method (I don't think we'd want to set el's to dynamically add/remove the 'closemenu' attr.) Would it be ok to just patch for middle-click first (implementation much simpler) and then do ctrl-click separately? Should this new behavior be behind a pref (or nightly only for now)? With the deprecation of xul-based add-ons, we can no longer say "if you don't like it install an add-on".

Let us give people time to try the feature in Nightly before we decided to let it ride the train to release.

(In reply to custom.firefox.lady from comment #47) > Some > reluctance was expressed in comment#24 about the method for implementing > Ctrl-click. Are we ok with that method? I don't know of a better method (I > don't think we'd want to set el's to dynamically add/remove the 'closemenu' > attr.) Would it be ok to just patch for middle-click first (implementation > much simpler) and then do ctrl-click separately? Should this new behavior be > behind a pref (or nightly only for now)? So now we have clear UX: any action that opens in a new tab (ctrl+click, middle-click, context menu) should leave the menu open. All of them should be implemented and should have a mochitest-browser test. OS X native menubar may be more problematic, we'll check its status once the other consumers are fixed. Regarding the implementation details, I agree with comment 24 that the brute-force approach of making all the menus stay open and then having to close them manually sounds a bit scary, and could not play well with add-ons overriding some of our methods. Comment 24 suggested investigating the usage of preventDefault() to obtain a similar result. Another alternative may be to have popuphiding handler that can somehow intercept the reason for closing and again preventDefault(). If you're interested into testing these possible alternatives that'd be very welcome. Regarding the pref, sounds like comment 48 gave an answer, we should have a pref for now (defineLazyPreferenceGetter is probably the way to go).

(In reply to Marco Bonardo [::mak] from comment #49) > So now we have clear UX: any action that opens in a new tab (ctrl+click, > middle-click, context menu) should leave the menu open. Clearly excluded any "container" (query or folder or live bookmark) that are usually menus.

(In reply to Marco Bonardo [::mak] from comment #49) > So now we have clear UX: any action that opens in a new tab (ctrl+click, > middle-click, context menu) should leave the menu open. All of them should > be implemented and should have a mochitest-browser test. Personally I don't think it's a good idea to leave no available method to open a new tab & close the menu, unless we are going to keep the pref permanent. But if UX wants to try that, ok. (This will only affect bookmarks menus though, not the history menu, or locationbar dropdown, or things in the newer PanelUI - making the UI somewhat inconsistent, IMO.) I am not familiar with Mochitest; will focus on code changes first. > OS X native menubar may be more problematic, we'll check its status once the > other consumers are fixed. I have no access to OSX; someone else would need to handle that. As I mentioned in comment #30, there are other issues with OSX native menubar. From reports by my ext's users, the result would be that the native menubar will close the menu, but e.g. the items in the bookmarks menu button would keep the menu open. > Regarding the implementation details, I agree with comment 24 that the > brute-force approach of making all the menus stay open and then having to > close them manually sounds a bit scary, and could not play well with add-ons > overriding some of our methods. Comment 24 suggested investigating the usage > of preventDefault() to obtain a similar result. Another alternative may be > to have popuphiding handler that can somehow intercept the reason for > closing and again preventDefault(). If you're interested into testing these > possible alternatives that'd be very welcome. When I first implemented ctrl-click support in my ext, I attempted something like that. Didn't appear to work. From https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/PopupEvents "You also cannot cancel the hiding of a menupopup when a user has made a selection from a menu, as it is already too late to do so. In this situation the command event has already been sent to the selected menuitem and the operation already carried out. This is one special case in which the popuphiding event is fired after the popup has been removed from the screen. The reason for this is if, as is quite common, the menu item's action is to open a modal dialog. Here, the menu would need to be removed first, before the dialog can be opened. Otherwise, the user would have a menu for a parent window that is no longer active. So instead, the popup is removed first. Note that this means that the popuphiding event doesn't fire until after the modal dialog has been closed." The only thing I found possible at that time was to do an el on the command evt, then dynamically write the closemenu attr (removing it afterward). It was a PITA to debug, so I later switched to using the method used in the previous patch. IIRC (and understood correctly) from my previous digging in fox guts, left button clicks trigger the command evt and the actual menu closing occurs in C++ code which checks the closemenu attr. AFAIK nothing's changed since then. Haven't tested, but I would guess doing preventDefault on the command evt would prevent the tab from loading at all. If you still think this is worth exploring further, I'll give it another look, but I don't think it seems promising. > Regarding the pref, sounds like comment 48 gave an answer, we should have a > pref for now (defineLazyPreferenceGetter is probably the way to go). Thanks, will look into that.

(In reply to custom.firefox.lady from comment #51) > Personally I don't think it's a good idea to leave no available method to > open a new tab & close the menu, unless we are going to keep the pref > permanent. Yes, I was suggesting keeping the pref permanently. Regardless, the behavior is something we can re-evaluate after a try and a round of feedback. > > OS X native menubar may be more problematic, we'll check its status once the > > other consumers are fixed. > > I have no access to OSX; someone else would need to handle that. As I > mentioned in comment #30, there are other issues with OSX native menubar. > From reports by my ext's users, the result would be that the native menubar > will close the menu, but e.g. the items in the bookmarks menu button would > keep the menu open. Yes, it's likely. There are already other differences there (context menus, D&D). I can have a look later once we have a patch. > Haven't tested, but I would guess doing preventDefault > on the command evt would prevent the tab from loading at all. If you still > think this is worth exploring further, I'll give it another look, but I > don't think it seems promising. It would be great to have a look, note that you intercept the command, so at that point you can do whatever you wish included loading a tab.

(In reply to Marco Bonardo [::mak] from comment #52) > It would be great to have a look, note that you intercept the command, so at > that point you can do whatever you wish included loading a tab. The problem is that preventDefault *only* prevents the tab from loading; it does not prevent the menu from closing.

Ok, can you better explain the reasons for which debugging a dynamically added closemenu attribute was a nightmare? Sorry for all the questions, I need to be sure all the possible approaches have been analyzed before going the brute-force one.

(In reply to Marco Bonardo [::mak] from comment #55) I'd added a mouseup el to menupopups which checked for ctrl key and added the attr; a corresponding 'command' el (click is too late) removed the attr. Doing Application.console.log (crude debugging; this was before add-on debugger) within this code would cause weird malfunctions. But since the attr is only added after the ctrl key is pressed, it means regular clicks still close the menu automatically. But the el' need to be specifically added to each desired menu, so won't apply to any bookmarks-type menus added by extensions (another reason I moved away from it - less 'can you make it work with ext x' requests). Another possible variant on the dynamically added option, would be to add the attr (if not already present) to all 'menuitems' on popupshown (removing on popuphidden?) But that prevents automatic menu closing for regular clicks. Dynamic methods would require more code than the 'brute force' method. No need to be sorry, ask and analyze all you want. Appreciate your input on this.

(In reply to custom.firefox.lady from comment #56) > I'd added a mouseup el to menupopups which checked for ctrl key and added > the attr; a corresponding 'command' el (click is too late) removed the attr. On mouseup you add the attribute only if CTRL is pressed or it's middle mouse button. At that point I was assuming always removing the attribute on popuphidden was fine for our scopes. What am I missing? > But the el' need to be specifically > added to each desired menu, so won't apply to any bookmarks-type menus added > by extensions (another reason I moved away from it - less 'can you make it > work with ext x' requests). This is less than a concern now that we are moving to webextensions. > Another possible variant on the dynamically added option, would be to add > the attr (if not already present) to all 'menuitems' on popupshown (removing > on popuphidden?) But that prevents automatic menu closing for regular clicks. This is not better than the current patch. The concern is with moving from a world where all the menus close automatically some don't, to a world where no menu closes automatically most will have to be closed manually. The latter is clearly scarier because in case of unexpected code malfunctions menus would stop closing.

Also, please expand why click would be too late to remove the attribute.

(In reply to Marco Bonardo [::mak] from comment #57 & comment #58) > On mouseup you add the attribute only if CTRL is pressed or it's middle > mouse button. > At that point I was assuming always removing the attribute on popuphidden > was fine for our scopes. > What am I missing? We'd only add attr for ctrl/meta, not for middle-click; fx handles middle-click menu closing differently so closemenu attr is not applicable in that case. correction: click is too late to *add* the el - we can *remove* on click (my previous use of command was unrelated to the scope of this bugfix). I have doubts about waiting until popuphidden; consider this use case: user ctrl-clicks some menuitems (attr's get added), at some point later while menu is still kept open, user regular clicks a previously ctrl-clicked item. Wouldn't the menu remain open, since the attr has not yet been removed? Admittedly not a likely scenario, but behavior would seem broken. All this mouse-y talk reminds me that Ctrl-Enter (which opens in a new tab, but isn't mouse-y) will close the menu with this implementation. Are we ok with that?

Attached WIP patch for feedback. Patch requires browser.bookmarks.conditionalMenuClose manually added and set to true to take effect. Any decision on whether Ctrl-ENTER should keep menu open (should I attempt keypress el) ? I went with a simple removal of attr on click; do we want to save a ref to the menuitem it was added to, and only remove it from there? Additional Qs in code comments. I commented out code (starting in line 77 of patch) and used the closeMenus funct instead. In retrospect, I should have simply deleted the code as it would have made the diff easier to read. Feel free to suggest changes to any var names or pref name.

(In reply to custom.firefox.lady from comment #61) > Any decision on whether Ctrl-ENTER should keep menu open (should I attempt > keypress el) ? there was no feedback/needinfo request to UX specifically for this, but comment 43 stated the behavior should be the same as in web context, and there ctrl+enter leaves the user in the current context. That said, if it complicates things considerably, we can do that apart (separate bug or changeset). > I went with a simple removal of attr on click; do we want to save a ref to > the menuitem it was added to, and only remove it from there? doesn't sound needed, off-hand, but I didn't look at the patch yet.

(In reply to Marco Bonardo [::mak] from comment #62) > there was no feedback/needinfo request to UX specifically for this, but > comment 43 stated the behavior should be the same as in web context, and > there ctrl+enter leaves the user in the current context. I agree, ctrl+enter should follow the same pattern and leave user in current context.

Ctrl-Enter /does/ complicate things since we're not using the brute force method...but found a way to make it work. Updated patch attached for feedback. I know we prefer not to add code to browser.js; can you suggest a better place for that bit portion of code to live? (it needs to run on each window load)

Sorry if it's taking a bit too much, too many priorities lately. Off-hand the approach looks ok but I need some dedicated time to look at it properly.

(In reply to Marco Bonardo [::mak] from comment #66) > Sorry if it's taking a bit too much, too many priorities lately. Off-hand > the approach looks ok but I need some dedicated time to look at it properly. Hi did you get a chance to finish the review? If it lands soon we can test it so that it can make it to the release of FF55

Will this be Uplifted to beta too?

(In reply to Mefoster from comment #68) > Will this be Uplifted to beta too? unlikely.

(In reply to Marco Bonardo [::mak] from comment #69) > (In reply to Mefoster from comment #68) > > Will this be Uplifted to beta too? > > unlikely. Oh ok thanks, hoping it lands in FF55 so gets tested

Is there any reason the same couldn't be done to the places results? If anything its more frustrating because you also lose the search term. Also the search needs to be performed again, and the order of the results might not even be the same.

(In reply to avada from comment #71) > Is there any reason the same couldn't be done to the places results? Who said it couldn't be done? Assuming you mean the awesomebar, it's just totally different code with different requirements, that were no analyzed since this bug is not about those.

(In reply to Marco Bonardo [::mak] from comment #72) > (In reply to avada from comment #71) > > Is there any reason the same couldn't be done to the places results? > > Who said it couldn't be done? Assuming you mean the awesomebar, it's just > totally different code with different requirements, that were no analyzed > since this bug is not about those. Did anyone consider it though?

(In reply to avada from comment #73) > Did anyone consider it though? Maybe? I'm not aware of any thought in the brains of our UX experts :) The best bet is filing a bug apart and getting UX feedback.

> Is there any reason the same couldn't be done to the places results? > If anything its more frustrating because you also lose the search term. Also > the search needs to be performed again, and the order of the results might > not even be the same. That's a very good suggestion. > (In reply to avada from comment #73) > > Did anyone consider it though? > > Maybe? I'm not aware of any thought in the brains of our UX experts :) The > best bet is filing a bug apart and getting UX feedback. It should be done

dupe bug 1370001

Should/Could this be applicable for address bar history drop down also? So we can middle click many of the items in the address bar history without it closing.

(In reply to mdew from comment #78) > Should/Could this be applicable for address bar history drop down also? So > we can middle click many of the items in the address bar history without it > closing. Bug 1364415 was filed for that.

(Getting closer to having a patch with mochitests ready for review.) I'm using 2 functions I found in mozilla-central/browser/components/places/tests/browser/head.js (to toggle on menubar & bookmarks toolbar). Is that directory a suitable location for my mochitest file? (If not, I'll copy out the code from those functs.) Also, are there existing tests I should run locally? I don't think my computer is speedy enough to run the full suite in a semi-reasonable period of time, but could probably do a few directory's of them, if appropriate.

(In reply to custom.firefox.lady from comment #80) > Is that directory a suitable location for my mochitest file? (If not, I'll > copy out the code from those functs.) Yes, it's the right folder. > Also, are there existing tests I should run locally? I don't think my > computer is speedy enough to run the full suite in a semi-reasonable period > of time, but could probably do a few directory's of them, if appropriate. You could apply for Try Server commit access by filing a bug, there should be no problem with that. I don't think we have very specific tests for middle clicking or context clicking menus, if they exist are likely in that same folder you are adding a test. Note that the team is working on new views for Photon, in bug 1354159, and that may change the code a bit, or just require to also handle the new view.

updated patch to make the contextmenu 'open in new tab' implementation respect the pref and to include mochitests

(In reply to Marco Bonardo [::mak] from comment #81) > You could apply for Try Server commit access by filing a bug, there should > be no problem with that. Skipping that for now, but thanks. > I don't think we have very specific tests for middle clicking or context > clicking menus, if they exist are likely in that same folder you are adding > a test. Ran tests in that folder; all pass, including those in my newly added file > Note that the team is working on new views for Photon, in bug 1354159, and > that may change the code a bit, or just require to also handle the new view. Any idea how soon that will land? From a quick glance, I don't expect any conflicts, but agree we should add code to handle the new view. Will test after that bug's patch lands.

Does the patch leave the mouse focus alone? So If you middle click each bookmark the mouse focus isn't lost? See the video from; https://bugzilla.mozilla.org/show_bug.cgi?id=1370001 as example

Now that bug 1354159 has landed, I've had a look re:'Recent bookmarks' on Library Button: (this is in a clean profile on Nightly) 1. Middle-click appears not to be supported (does nothing; menu does not close, bookmark does not load or open tab) 2. Context menu 'Open in a new Tab' already leaves the menu open (intended?) 3. I cannot reach recent bookmarks via keyboard down arrow. The hilite stops at 'Search bookmarks' (not 100% reproducible, but fails more often than not) Are those known issues, or should I file bugs? Can we wait and add stayopen behavior for the Library Button's 'Recent bookmarks' in a separate bug after that subview's functionality is more complete / less buggy ? Mochitests still passing and not seeing any conflicts with this bug's patch. (Of course, as per comment #52, the menubar tests are expected to fail on OSX.)

(In reply to custom.firefox.lady from comment #85) > 1. Middle-click appears not to be supported (does nothing; menu does not > close, bookmark does not load or open tab) I can't test this because my middle mouse button is broken (srsly) :( Please file a bug. Make it depend on bug 1354159 and set [photon-structure] in the whiteboard, so it will be triaged. > 2. Context menu 'Open in a new Tab' already leaves the menu open (intended?) It's not intended, but nice to have! We should investigate what causes it, I'd not want that it's a missing bit of code and we could regress it by fixing that missing code. > 3. I cannot reach recent bookmarks via keyboard down arrow. The hilite stops > at 'Search bookmarks' (not 100% reproducible, but fails more often than not) Please file a bug depending on bug 1354159 and set [photon-structure] in the whiteboard. > Can we wait and add stayopen behavior for the Library Button's 'Recent > bookmarks' in a separate bug after that subview's functionality is more > complete / less buggy ? Sure we can.

(In reply to Marco Bonardo [::mak] from comment #86) > (In reply to custom.firefox.lady from comment #85) > > 1. Middle-click appears not to be supported (does nothing; menu does not > > close, bookmark does not load or open tab) > > I can't test this because my middle mouse button is broken (srsly) :( > Please file a bug. Make it depend on bug 1354159 and set [photon-structure] > in the whiteboard, so it will be triaged. Someone already filed this one: bug 1377967 > > 2. Context menu 'Open in a new Tab' already leaves the menu open (intended?) > > It's not intended, but nice to have! We should investigate what causes it, > I'd not want that it's a missing bit of code and we could regress it by > fixing that missing code. Ok, but this should be consistent regardless of what menu/button users are accessing bookmarks from. My patch makes keeping the menu open for 'Open in a new Tab' dependent upon the pref; the Library Button will keep the menu open regardless of the pref's setting. > > 3. I cannot reach recent bookmarks via keyboard down arrow. The hilite stops > > at 'Search bookmarks' (not 100% reproducible, but fails more often than not) > > Please file a bug depending on bug 1354159 and set [photon-structure] in the > whiteboard. Done: bug 1378016

bug 1354159 also broke SoM in Nightly 56. Please fix it until this lands in firefox.

(In reply to shellye from comment #88) > bug 1354159 also broke SoM in Nightly 56. > Please fix it until this lands in firefox. Please confine issues with the Stay-Open Menu extension to its support forum.

Any idea when you might be able to review my patch? Or someone else I should ask instead?

Comment on attachment 8881338 [details] [diff] [review] leave menu open for individual bookmarks menuitem (added mochitest) Review of attachment 8881338 [details] [diff] [review]: ----------------------------------------------------------------- As you suspected, the biggest problem is the "keydown" handler that is just too broad in browser. ::: browser/base/content/browser-places.js @@ +854,5 @@ > */ > + > + handleCtrlEnter: function (aEvent) { > + // keeps menu open; > + // only called if browser.bookmarks.conditionalMenuClose set Please rephrase @@ +881,5 @@ > + }, > + > + onMouseup: function BEH_onMouseup(aEvent) { > + // handles left-click with modifier if browser.bookmarks.conditionalMenuClose > + if (aEvent.button != 0 || !conditionalClose) { return; } we don't put bodies on the same line, please intend as usual. This is valid also for the other instances where you do the same. @@ +882,5 @@ > + > + onMouseup: function BEH_onMouseup(aEvent) { > + // handles left-click with modifier if browser.bookmarks.conditionalMenuClose > + if (aEvent.button != 0 || !conditionalClose) { return; } > + var target = aEvent.originalTarget; also, please use let instead of var @@ +889,5 @@ > + if (AppConstants.platform == "macosx") { > + modifKey = aEvent.metaKey; > + } else { > + modifKey = aEvent.ctrlKey; > + } let modifKey = AppConstants.platform === "macosx" ? aEvent.metaKey : aEvent.ctrlKey; @@ +890,5 @@ > + modifKey = aEvent.metaKey; > + } else { > + modifKey = aEvent.ctrlKey; > + } > + // don't keep menu open for 'Open all in Tabs' menuitem nit: in comments, please UC first letter and end with a period. @@ +910,5 @@ > return; > > var target = aEvent.originalTarget; > + // middle-clicks (and left clicks on a folder) do not automatically close menu > + // leave menu open for single menuitems with browser.bookmarks.conditionalMenuClose The comment is unclear, maybe result of editing, please rephrase. @@ +913,5 @@ > + // middle-clicks (and left clicks on a folder) do not automatically close menu > + // leave menu open for single menuitems with browser.bookmarks.conditionalMenuClose > + if ((!conditionalClose || > + target.tagName != "menuitem") || > + target.classList.contains("openintabs-menuitem")) { reindent as if ((!conditionalClose || target.tagName != "menuitem") || target.classList.contains("openintabs-menuitem") Afaict, the original code was closing only for "menu" and "menuitem", is there a reason to use target.tagName != "menuitem" instead of target.tagName == "menu"? ::: browser/base/content/browser.js @@ +1716,5 @@ > }); > > + if (conditionalClose) { > + window.addEventListener("keydown", BookmarksEventHandler.handleCtrlEnter, true); > + } Yes, I'm not a big fan of this. AFAICT, we only need to handle keydown on Places views, this is handling keydown for the whole window, that is surely not what we want to do. Can't we someohw modify oncommand to set the attribute before it actually handles the command, and unset it after? Otherwise, you could try moving the keydown handler to a more specific handler in browserPlacesViews.js? ::: browser/components/places/content/placesOverlay.xul @@ +31,5 @@ > "PlacesUIUtils", "resource:///modules/PlacesUIUtils.jsm"); > XPCOMUtils.defineLazyModuleGetter(window, > "PlacesTransactions", "resource://gre/modules/PlacesTransactions.jsm"); > + XPCOMUtils.defineLazyPreferenceGetter(this, "conditionalClose", > + "browser.bookmarks.conditionalMenuClose", false); nit: indent as XPCOMUtils.defineLazyPreferenceGetter(this, "conditionalClose", "browser.bookmarks.conditionalMenuClose", false); ::: browser/components/places/tests/browser/browser_stayopenmenu.js @@ +4,5 @@ > +async function locateTestBookmark(menupopup) { > + let win = menupopup.ownerDocument.defaultView; > + var bookmarkActive = false; > + // find and activate our added bookmark > + for (let i = 0; !bookmarkActive && i < menupopup.childElementCount; i++) { you could also use for...of on menupopup.childNodes. Does it matter that it's actually really active for the test? @@ +20,5 @@ > + > +async function locateBookmarkAndTestCtrlClick(menupopup) { > + var menuitem = null; > + for (let i=0; i < menupopup.childNodes.length; i++) { > + var node = menupopup.childNodes[i]; for...of @@ +49,5 @@ > + return newTab; > +} > + > +add_task(async function testStayopenBookmarksCtrlEnter() { > + SpecialPowers.setBoolPref("browser.bookmarks.conditionalMenuClose", true); use SpecialPowers.pushPrefEnv instead, it will automatically restore the pref at the end of the test. I'd also move that to a test_setup initial test that will run before the others and prepare the test env there, so you don't have to repeat it for each test. @@ +56,5 @@ > + let menubarVisible = isToolbarVisible(menubar); > + if (!menubarVisible) { > + setToolbarVisibility(menubar, true); > + info("menubar made visible"); > + } should be restored using registerCleanupFunction, or in case of failure this may pollute the next tests. This is valid for any changes that may affect the next tests in case of a timeout or failure. @@ +61,5 @@ > + var itemId = PlacesUtils.bookmarks > + .insertBookmark(PlacesUtils.bookmarksMenuFolderId, > + PlacesUtils._uri("http://www.example.com"), > + 0, > + "Test1"); Should use the new async API (await PlacesUtils.bookmarks.insert(... @@ +63,5 @@ > + PlacesUtils._uri("http://www.example.com"), > + 0, > + "Test1"); > + // after setting pref, CtrlEnter only keeps menu open on new window > + // until after browser restart This is not ok, probably the defineLazyPreferenceGetter should grow an aOnUpdate callback and setup what it needs. @@ +65,5 @@ > + "Test1"); > + // after setting pref, CtrlEnter only keeps menu open on new window > + // until after browser restart > + let newWin = await BrowserTestUtils.openNewBrowserWindow(); > + info("opened new window"); So this should not be needed. @@ +67,5 @@ > + // until after browser restart > + let newWin = await BrowserTestUtils.openNewBrowserWindow(); > + info("opened new window"); > + > + // test Ctrl-Enter on bookmarks menu wrong intendation @@ +71,5 @@ > + // test Ctrl-Enter on bookmarks menu > + let BM = newWin.document.getElementById("bookmarksMenu"); > + let BMpopup = newWin.document.getElementById("bookmarksMenuPopup"); > + let promiseEvent = BrowserTestUtils.waitForEvent(BMpopup, "popupshown"); > + EventUtils.synthesizeMouseAtCenter(BM, {}, newWin); does BM.click() work? @@ +88,5 @@ > + await promiseEvent; > + info("closing popup"); > + await BrowserTestUtils.removeTab(newTab); > + > + // test Ctrl-Enter on bookmarks menu button indentation maybe split to a separate task? @@ +93,5 @@ > + let BMB = newWin.document.getElementById("bookmarks-menu-button"); > + let BMBpopup = newWin.document.getElementById("BMB_bookmarksPopup"); > + > + promiseEvent = BrowserTestUtils.waitForEvent(BMBpopup, "popupshown"); > + EventUtils.synthesizeMouseAtCenter(BMB, {}, newWin); probably .click(), but note that bookmarks-menu-button can either be a menu or a menu-button depending on photon, and if it's a menu-button I'm not sure that a click nor a center mouse event will open the menu. You can check browser/components/places/tests/browser/browser_views_liveupdate.js for a method to open the popups that may also work on Mac for the main menubar. @@ +138,5 @@ > + var itemId = PlacesUtils.bookmarks > + .insertBookmark(folderId, > + PlacesUtils._uri("http://www.example.com"), > + 0, > + "Test2"); ditto, please use the new API, and use "let" instead of var, in the whole test. @@ +163,5 @@ > + info("closing popup"); > + await BrowserTestUtils.removeTab(newTab); > + BrowserTestUtils.closeWindow(newWin); > + PlacesUtils.bookmarks.removeItem(itemId); > + PlacesUtils.bookmarks.removeItem(folderId); I think you could just: registerCleanupFunction(async function() { ... await PlacesUtils.bookmarks.eraseEverything(); }); in the setup task, so all the bookmarks will be removed after the test.

(In reply to Marco Bonardo [::mak] from comment #91) > Comment on attachment 8881338 [details] [diff] [review] > leave menu open for individual bookmarks menuitem (added mochitest) > > Review of attachment 8881338 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: browser/base/content/browser-places.js > @@ +854,5 @@ > > */ > > + > > + handleCtrlEnter: function (aEvent) { > > + // keeps menu open; > > + // only called if browser.bookmarks.conditionalMenuClose set > > Please rephrase // Called to prevent menu-closing if browser.bookmarks.conditionalMenuClose. Ok? > @@ +910,5 @@ > > return; > > > > var target = aEvent.originalTarget; > > + // middle-clicks (and left clicks on a folder) do not automatically close menu > > + // leave menu open for single menuitems with browser.bookmarks.conditionalMenuClose > > The comment is unclear, maybe result of editing, please rephrase. // If this event bubbled up from a menu or menuitem, // close the menus if not browser.bookmarks.conditionalMenuClose. Ok? > @@ +913,5 @@ > > + // middle-clicks (and left clicks on a folder) do not automatically close menu > > + // leave menu open for single menuitems with browser.bookmarks.conditionalMenuClose > > + if ((!conditionalClose || > > + target.tagName != "menuitem") || > > + target.classList.contains("openintabs-menuitem")) { > <snip> > Afaict, the original code was closing only for "menu" and "menuitem", is > there a reason to use target.tagName != "menuitem" instead of target.tagName > == "menu"? Correct. I was being overly careful not to keep the menu open when it should close. target.tagName == "menu" will likely be fine, if you prefer that. Actually, we should probably use if ((!conditionalClose && target.tagName == "menuitem") || target.tagName == "menu" || target.classList.contains("openintabs-menuitem")) if we want to only close for menu & (optionally)menuitem & openintabs > ::: browser/base/content/browser.js > @@ +1716,5 @@ > > }); > > > > + if (conditionalClose) { > > + window.addEventListener("keydown", BookmarksEventHandler.handleCtrlEnter, true); > > + } > > Yes, I'm not a big fan of this. > AFAICT, we only need to handle keydown on Places views, this is handling > keydown for the whole window, that is surely not what we want to do. > > Can't we someohw modify oncommand to set the attribute before it actually > handles the command, and unset it after? I don't think so. oncommand seems to be already too late. e.g. window.addEventListener("command", BookmarksEventHandler.handleCtrlEnter, true); doesn't work > Otherwise, you could try moving the keydown handler to a more specific > handler in browserPlacesViews.js? Had made some attempts at that. I'd also tried setting the el at a lower level in the DOM (e.g. the PlacesToolbar instead of 'window'), but keydown evt's don't appear to be sent there at all. However, I have since thought of another possibility. What if we do set the el on 'window', but set it in onpopupshowing of the menus we want to affect and remove el onpopuphiding (or perhaps shown/hidden) ? In effect we'd handle it on the window level, but only when one of our desired places menus is open. (Thus also eliminating changes to browser.js & the issue of pref not taking effect until new win opened.) BTW, I also found a bug in my handleCtrlEnter code (broken in subfolders). Fixed with only a few lines of extra code. > ::: browser/components/places/tests/browser/browser_stayopenmenu.js > @@ +4,5 @@ > > +async function locateTestBookmark(menupopup) { > > + let win = menupopup.ownerDocument.defaultView; > > + var bookmarkActive = false; > > + // find and activate our added bookmark > > + for (let i = 0; !bookmarkActive && i < menupopup.childElementCount; i++) { > > you could also use for...of on menupopup.childNodes. Does it matter that > it's actually really active for the test? Not sure how to find the bookmark and send Ctrl-Enter to it without making it active. > @@ +71,5 @@ > > + // test Ctrl-Enter on bookmarks menu > > + let BM = newWin.document.getElementById("bookmarksMenu"); > > + let BMpopup = newWin.document.getElementById("bookmarksMenuPopup"); > > + let promiseEvent = BrowserTestUtils.waitForEvent(BMpopup, "popupshown"); > > + EventUtils.synthesizeMouseAtCenter(BM, {}, newWin); > > does BM.click() work? I would have thought so, but strangely, no. > @@ +93,5 @@ > > + let BMB = newWin.document.getElementById("bookmarks-menu-button"); > > + let BMBpopup = newWin.document.getElementById("BMB_bookmarksPopup"); > > + > > + promiseEvent = BrowserTestUtils.waitForEvent(BMBpopup, "popupshown"); > > + EventUtils.synthesizeMouseAtCenter(BMB, {}, newWin); > > probably .click(), but note that bookmarks-menu-button can either be a menu > or a menu-button depending on photon, and if it's a menu-button I'm not sure > that a click nor a center mouse event will open the menu. > You can check > browser/components/places/tests/browser/browser_views_liveupdate.js for a > method to open the popups that may also work on Mac for the main menubar. ...and no, .click() doesn't work here either :( Will try the fakeOpenPopup for the menubar.

> (In reply to Marco Bonardo [::mak] from comment #91) > > ::: browser/components/places/tests/browser/browser_stayopenmenu.js > > @@ +4,5 @@ > > > +async function locateTestBookmark(menupopup) { > > > + let win = menupopup.ownerDocument.defaultView; > > > + var bookmarkActive = false; > > > + // find and activate our added bookmark > > > + for (let i = 0; !bookmarkActive && i < menupopup.childElementCount; i++) { > > > > you could also use for...of on menupopup.childNodes. Does it matter that > > it's actually really active for the test? "Not sure how to find the bookmark and send Ctrl-Enter to it without making it active." Edit: If you meant something like the following, I could not get it to work (new tab does not get opened). Perhaps I am doing something incorrectly here? for (let node of BMpopup.childNodes) { if(node.label == "Test1") { info("found test bookmark"); var evt = new KeyboardEvent('keypress', { 'view': newWin, 'bubbles': true, 'cancelable': true, 'ctrlKey':true, 'key': "Enter", 'code': "Enter" }); let promiseTabOpened = BrowserTestUtils.waitForNewTab(newWin.gBrowser, null); node.dispatchEvent(evt); let newTab = await promiseTabOpened; ok(true, "Ctrl-ENTER on bookmark opened new tab"); break; } }

Attached updated patch addressing review issues including new code for Ctrl-Enter. See comment #91 & comment #92.

I don't have time to go through the patch before leaving, but from a quick look it still looks like the whole ctrl+enter handling is a big hack. I'm not saying it's wrong or that in the end it won't be the way we do it, but the more I look at it, the more I think we should split this bug into 2, land the mouse part and move the keyboard part to a separate bug, or we'll keep running in circles forever. Drew lately has worked on the UI code far more than me (I worked mostly on the backend in the last months) and hopefully he will bring some fresh new ideas to the table. In the worst case, I'd suggest to do the above splitting.

(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #95) > I don't have time to go through the patch before leaving, but from a quick > look it still looks like the whole ctrl+enter handling is a big hack. I'm > not saying it's wrong or that in the end it won't be the way we do it, but > the more I look at it, the more I think we should split this bug into 2, > land the mouse part and move the keyboard part to a separate bug, or we'll > keep running in circles forever. > > Drew lately has worked on the UI code far more than me (I worked mostly on > the backend in the last months) and hopefully he will bring some fresh new > ideas to the table. > In the worst case, I'd suggest to do the above splitting. Actually, I was planning to request splitting if this new approach was also problematic. I would like to be able to at least get the mouse part landed for fx57, if possible. If someone can think of a better way to implement Ctrl-Enter, that would be great, but I'm not having much luck. The next month or two will be very busy for me, so I likely won't have much time to work on this for awhile. From comment #91 > note that bookmarks-menu-button can either be a menu or a menu-button depending on photon, and if it's a menu-button > I'm not sure that a click nor a center mouse event will open the menu. > You can check browser/components/places/tests/browser/browser_views_liveupdate.js for a method to open the popups that may > also work on Mac for the main menubar. Works fine in that file, but causes my tests to fail (popups & new tabs don't open). Anyway, I'm not sure I understand what you mean about the bookmarks-menu-button, or what photon has to do with it. If you're thinking of the PanelUI, this bugfix does not keep the panel open as per comment #51. It does provide stayopen behavior for the BMB on the toolbar.

removed code/tests for Ctrl-Enter, also previous patch got broken by Bug 1377967 (browserPlacesViews.js): this.panelMultiView.closest("panel").hidePopup(); updated my patch to make above line depend on pref However, I was unable to determine the purpose of this hidePopup(). Obviously it closes the panel/menu, but I could not locate for what user action the panel/menu does not close appropriately without that code. Gijs, can you inform us of its intended purpose?

(In reply to custom.firefox.lady from comment #97) > Created attachment 8897676 [details] [diff] [review] > Bug260611KeepMenuOpenClicks > > removed code/tests for Ctrl-Enter, also previous patch got broken by Bug > 1377967 (browserPlacesViews.js): > this.panelMultiView.closest("panel").hidePopup(); > > updated my patch to make above line depend on pref > > However, I was unable to determine the purpose of this hidePopup(). > Obviously it closes the panel/menu, but I could not locate for what user > action the panel/menu does not close appropriately without that code. Gijs, > can you inform us of its intended purpose? The recently closed tabs view broke (ie did the wrong thing) when opening multiple items in tabs while keeping the popup open. Because the previous behaviour was to close the popup when items were middle-clicked, I restored that behaviour. If you break this, you will re-introduce the original bug which is that the recently closed tabs view doesn't cope with more than 1 item being clicked while the popup remains open. So please don't do that.

(In reply to :Gijs from comment #98) > The recently closed tabs view broke (ie did the wrong thing) when opening > multiple items in tabs while keeping the popup open. Because the previous > behaviour was to close the popup when items were middle-clicked, I restored > that behaviour. If you break this, you will re-introduce the original bug > which is that the recently closed tabs view doesn't cope with more than 1 > item being clicked while the popup remains open. So please don't do that. My patch does not keep the menu open for Recently Closed Tabs (History Item), only for Bookmark Items. It also does not keep the Library Button or PanelUI (hamburger) panels open. Thanks for the info (now we'll know what to watch for) but I'm not seeing any such issue on my Linux build with my patch.

Marco, might you be able to review this now that you're back? Or should I needinfo Drew? In addition to splitting off Ctrl-Enter and fixing breakage (comment # 97), I also added code to my mochitest to move the bookmarks-menu-button (no longer present by default) into the toolbar.

(In reply to custom.firefox.lady from comment #100) > Marco, might you be able to review this now that you're back? Or should I > needinfo Drew? In addition to splitting off Ctrl-Enter and fixing breakage > (comment # 97), I also added code to my mochitest to move the > bookmarks-menu-button (no longer present by default) into the toolbar. Will this be added in Firefox 57? Not much time till it's merged in beta and no testing or fall out bugs are known until patch lands Was using the extension for this and it's not working in Firefox 56 beta. If this mouse part lands then would switch to Firefox 57 and will wait for KB patch before next ESR59 lands, Sorry to be rude but why is this reviewing process taking so much time? This is an important improvement and chrome already does this, plus a community peer's providing the patch! Why not help him out devs, look what improvements Kevin Jones in Bug 906076 (lazytabs) & custom.firefox.lady is trying to improve Firefox too so please review it and add it.

Comment on attachment 8897676 [details] [diff] [review] Bug260611KeepMenuOpenClicks Looking into this in the next 2 days, promised. Tanks for splitting off the keyboard part.

Comment on attachment 8897676 [details] [diff] [review] Bug260611KeepMenuOpenClicks Review of attachment 8897676 [details] [diff] [review]: ----------------------------------------------------------------- There are various eslint failures: 5:13 error use .ownerGlobal instead of .ownerDocument.defaultView mozilla/use-ownerGlo 37:31 error Unexpected space before function parentheses. space-before-functio 41:54 error Missing space before value for key 'type'. key-spacing (eslint) 41:76 error Missing space before value for key 'button'. key-spacing (eslint) 77:7 error 'itemId' is assigned a value but never used. no-unused-vars (esli 87:11 error Missing space before value for key 'index'. key-spacing (eslint) 124:56 error Missing space before value for key 'button'. key-spacing (eslint) 152:56 error Missing space before value for key 'button'. key-spacing (eslint) 168:7 error 'PT' is assigned a value but never used. no-unused-vars (esli 195:56 error Missing space before value for key 'button'. key-spacing (eslint) you can easily setup eslint using ./mach eslint --setup and a supported text editor, or just use ./mach eslint <path> I also found a bug with the Library / Bookmarks view, looks like when conditionalMenuClose is true, we open 2 tabs instead of one with CTRL+click. This should be fixed since it affects the Photon primary UI. I'd also like the pref to get a more readable name, something like browser.bookmarks.openInNewTabClosesMenu (defaults to true) and we can add it to firefox.js, so users can discover it. In general let's drop the "conditionalClose" name that is too generic and keep a name that somehow recalls menus and newTab. Apart from these blocking issues, I quickly tested it and it seems to be working fine, at least on Windows. Considered the pref will default to the status-quo for now, we'll have some time to fix problems on native menu bars before evaluating flipping it on more broadly, so I won't block on perfect functionality in all the edge-cases until we'll be at a point to consider to ship the new behavior as default. ::: browser/base/content/browser-places.js @@ +9,5 @@ > "PlacesPanelview", "PlacesPanelMenuView"], > "chrome://browser/content/places/browserPlacesViews.js"); > > +XPCOMUtils.defineLazyPreferenceGetter(this, "conditionalClose", > + "browser.bookmarks.conditionalMenuClose", false); May we expose this from PlacesUIUtils instead, and just use PlacesUIUtils.openInNewTabClosesMenus around? So we don't need to add this lazy getter to every window (and likely now we are doing this multiple times). PlacesUIUtils is already imported by placesOverlay. ::: browser/components/places/tests/browser/browser.ini @@ +62,4 @@ > support-files = > bookmark_dummy_1.html > bookmark_dummy_2.html > +[browser_stayopenmenu.js] please keep alphabetical sorting of tests, as far as possible ::: browser/components/places/tests/browser/browser_stayopenmenu.js @@ +1,2 @@ > +// Menus should stay open (if pref is set) after ctrl-click, middle-click, > +// and contextmenu's "Open in a new tab" click. please add a license header first: /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ @@ +1,4 @@ > +// Menus should stay open (if pref is set) after ctrl-click, middle-click, > +// and contextmenu's "Open in a new tab" click. > + > +async function locateTestBookmark(menupopup) { This is currently unused and should be removed @@ +75,5 @@ > + } > + // Create our test bookmarks. > + let itemId = await PlacesUtils.bookmarks.insert({ > + parentGuid: PlacesUtils.bookmarks.menuGuid, > + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, nit: type can be omitted when a url is passed @@ +87,5 @@ > + index:0 > + }); > + itemId = await PlacesUtils.bookmarks.insert({ > + parentGuid: folder.guid, > + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, nit: ditto

(In reply to Marco Bonardo [::mak] from comment #103) > you can easily setup eslint using ./mach eslint --setup and a supported text > editor, or just use ./mach eslint <path> If it were that easy, I'd have done it a long time ago...got errors about nodejs not being installed and issues installing it. So I put it off until later...ok, it's later. Got help on #introduction and got it done. > I also found a bug with the Library / Bookmarks view, looks like when > conditionalMenuClose is true, we open 2 tabs instead of one with CTRL+click. At some point after comment #97, I had issues with hg (now fixed, AFAICT). Perhaps my build was already flaky, or perhaps something else changed, but my changes to browserPlacesViews.js are no longer required, and without them your bug disappeared. > May we expose this from PlacesUIUtils instead, and just use > PlacesUIUtils.openInNewTabClosesMenus around? > So we don't need to add this lazy getter to every window (and likely now we > are doing this multiple times). Certainly, way better! (Other review issues have also been addressed in updated patch.)

Comment on attachment 8904811 [details] [diff] [review] Bug 260611 - Introduce pref browser.bookmarks.openInNewTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. Review of attachment 8904811 [details] [diff] [review]: ----------------------------------------------------------------- I did a Try push of the patch, and it looks good test-wise https://treeherder.mozilla.org/#/jobs?repo=try&revision=07bb9c7b9e02532875ca15ff41550c779da01b59 Thank you for you patience, lately development sped up quite a bit and it was not trivial to keep up with everything. Hopefully, after some testing with the pref, and when we find a clean way to implement keyboard support without, this may become the default. ::: browser/app/profile/firefox.js @@ +501,5 @@ > > pref("browser.bookmarks.showRecentlyBookmarked", true); > > +// Whether menu should close after Ctrl-click, middle-click, etc. > +pref("browser.bookmarks.openInNewTabClosesMenu", true); let's drop "New", it looks redundant, so the pref becomes just: browser.bookmarks.openInTabClosesMenu ::: browser/base/content/browser-places.js @@ +906,5 @@ > * @param aView > * The places view which aEvent should be associated with. > */ > + > + onMouseup: function BEH_onMouseup(aEvent) { nit: just "onMouseup(aEvent) {" also s/onMouseup/onMouseUp/ to respect lowerCamelCase. ::: browser/components/places/PlacesUIUtils.jsm @@ +33,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "Weave", > "resource://services-sync/main.js"); > > +XPCOMUtils.defineLazyPreferenceGetter(this, "pref_openInNewTabClosesMenu", > + "browser.bookmarks.openInNewTabClosesMenu", false); Let's also drop "New" From the PlacesUIUtils property name. Actually, I think you could (please check) directly do: XPCOMUtils.defineLazyPreferenceGetter(PlacesUIUtils, "openInTabClosesMenu", ... ::: browser/components/places/content/placesOverlay.xul @@ +95,5 @@ > <menupopup id="placesContext" > onpopupshowing="this._view = PlacesUIUtils.getViewForNode(document.popupNode); > + if (!PlacesUIUtils.openInNewTabClosesMenu) { > + var cmNewTab = document.getElementById ('placesContext_open:newtab'); > + cmNewTab.setAttribute('closemenu', 'single'); nit: you could avoid the temp var with some indentation: document.getElementById('placesContext_open:newtab') .setAttribute('closemenu', 'single');

will pref browser.bookmarks.openInNewTabClosesMenu also work on folders? eg Folder 1 Ctrl+click hover to Folder 2 Ctrl+click? It would be great(chrome does this by default)

Forgot to ask will this honor the user pref (Disabled by default) browser.tabs.loadBookmarksInBackground browser.tabs.loadInBackground

(In reply to Marco Bonardo [::mak] from comment #105) > ::: browser/base/content/browser-places.js > @@ +906,5 @@ > > * @param aView > > * The places view which aEvent should be associated with. > > */ > > + > > + onMouseup: function BEH_onMouseup(aEvent) { > > nit: just "onMouseup(aEvent) {" Other functions in BookmarksEventHandler are prefixed with BEH_. Why should this one be different? > ::: browser/components/places/PlacesUIUtils.jsm > @@ +33,5 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "Weave", > > "resource://services-sync/main.js"); > > > > +XPCOMUtils.defineLazyPreferenceGetter(this, "pref_openInNewTabClosesMenu", > > + "browser.bookmarks.openInNewTabClosesMenu", false); > Actually, I think you could (please check) directly do: > XPCOMUtils.defineLazyPreferenceGetter(PlacesUIUtils, "openInTabClosesMenu", Using "openInTabClosesMenu" directly works (but you can't sub PlacesUIUtils for <this> - error: "PlacesUIUtils is not defined"). Thanks for all your time & assistance in getting this patch ready. After I attach the modified patch, I'll file follow-up bugs for Ctrl-Enter and the Library button.

(In reply to custom.firefox.lady from comment #108) > Other functions in BookmarksEventHandler are prefixed with BEH_. Why should > this one be different? the double labels were needed time ago when the debugger was broken, now we can use ES6 shorthands that are nicer to read. It's a nit btw, I don't care that much. (In reply to custom.firefox.lady from comment #108) > Using "openInTabClosesMenu" directly works > (but you can't sub PlacesUIUtils for <this> - error: "PlacesUIUtils is not > defined"). Strange we do the same here: XPCOMUtils.defineLazyPreferenceGetter(PlacesUIUtils, "loadBookmarksInBackground", ...

(In reply to Marco Bonardo [::mak] from comment #109) > (In reply to custom.firefox.lady from comment #108) > > Other functions in BookmarksEventHandler are prefixed with BEH_. Why should > > this one be different? > > the double labels were needed time ago when the debugger was broken, now we > can use ES6 shorthands that are nicer to read. It's a nit btw, I don't care > that much. Not being familiar with ES6 shorthands, I was only thinking drop "BEH_" prefix. But you meant I can literally use just "onMouseup(aEvent) {" Sweet! > (In reply to custom.firefox.lady from comment #108) > > Using "openInTabClosesMenu" directly works > > (but you can't sub PlacesUIUtils for <this> - error: "PlacesUIUtils is not > > defined"). > > Strange we do the same here: > XPCOMUtils.defineLazyPreferenceGetter(PlacesUIUtils, > "loadBookmarksInBackground", ... Indeed, I just need to relocate it near that statement (i.e. later in the file, after PlacesUIUtils /is/ defined). Plan to upload updated patch later today. Do you want to re-review or not (think I read somewhere to ask, if any doubt at all)?

no need to re-review, in any case I always check the patches I reviewed when they land. For checkin-needed you'll need a green-ish Try run.

Updated patch; please do Try run, thanks.

Comment on attachment 8906944 [details] Bug 260611 - Introduce pref browser.bookmarks.openInTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. https://reviewboard.mozilla.org/r/178686/#review183688

Comment on attachment 8906876 [details] Bug 260611 - Introduce pref browser.bookmarks.openInTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. I fixed one ESLint error in PUIU.

Looks like the test is now timing out on Mac at await locateBookmarkAndTestCtrlClick

(In reply to Marco Bonardo [::mak] from comment #117) > Looks like the test is now timing out on Mac at await > locateBookmarkAndTestCtrlClick If I'm understanding that correctly, the menu opens but we never get the info msg: "Bookmark ctrl-click opened new tab." so it's failing to even open a new tab (unless there's some issue with the test itself). Does Ctrl-click open a new tab on mac on the bookmarks menu button? (Per Bug 490002 it does not work on the native menubar.) I was only expecting mac to fail on native menubar, but I have no mac access or experience. Previous Try (comment #105) shows mac as "cancelled by user" (so not sure if it did/didn't work previously). From comment #52 you were going to look at this once we had a patch. So where do we go from here? PS: Making myself a big note to remember to actually run eslint, now that I got it working (wishing could automate this; don't think it integrates into Geany.)

I'll look on my Mac as soon as I'm done with my P1s

It's not working on folders unlike SoM which works on Folders too.

(In reply to shellye from comment #120) > It's not working on folders unlike SoM which works on Folders too. Intended behavior: See comment #50

(In reply to custom.firefox.lady from comment #121) > (In reply to shellye from comment #120) > > It's not working on folders unlike SoM which works on Folders too. > > Intended behavior: See comment #50 Bummer as open groups of bookmark folders and its easier...

Comment on attachment 8906944 [details] Bug 260611 - Introduce pref browser.bookmarks.openInTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. https://reviewboard.mozilla.org/r/178686/#review184372 Marco asked me to take a look at the Mac issues on this patch. I found a couple of things that help it work. ::: browser/base/content/browser-places.js:936 (Diff revision 1) > - else if (node.localName != "menu" && > - node.localName != "hbox" && > - node.localName != "vbox" ) > - break; > - } > + } > + // Command already precesssed so remove any closemenu attr set in onMouseUp. > + if (aEvent.button == 0 && > + target.tagName == "menuitem" && > + target.getAttribute("closemenu") == "none") { > + target.removeAttribute("closemenu"); On Mac, this seems to be removing the attribute too early. After discussion with Marco, it looks like we need a setTimeout here of around 500ms to avoid any animations before the click even happens. ::: browser/components/places/tests/browser/browser_stayopenmenu.js:13 (Diff revision 1) > + let menuitem = null; > + for (let node of menupopup.childNodes) { > + if (node.label == "Test1") { > + menuitem = node; > + let promiseTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, null); > + EventUtils.synthesizeMouseAtCenter(menuitem, {ctrlKey: true}); The second parameter needs to be changed here, as on Mac the meta key is used, not the ctrl key: EventUtils.synthesizeMouseAtCenter(menuitem, AppConstants.platform === "macosx" ? {metaKey: true} : {ctrlKey: true});

I've updated the patch for my comments & pushed it to try server. I disabled a lot of the test on Mac due to the way Mac handles menus slightly differently to other platforms.

still timing out, but the try patch looks empty, so I'm not sure if you pushed the latest version...

(In reply to Marco Bonardo [::mak] from comment #126) > still timing out, but the try patch looks empty, so I'm not sure if you > pushed the latest version... Weird, my guess is somehow I got autoland to push the old version.

Comment on attachment 8907658 [details] Bug 260611 - Introduce pref browser.bookmarks.openInTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. https://reviewboard.mozilla.org/r/179330/#review185008 For the curious coming here checking what to expect, this is not complete, there's follow-up work to do in future bugs: 1. enable more part of the test on Mac (at least the toolbar part should be fine) 2. Try to find a better solution to the timeout on Mac 3. Decide what to do with containers (close, not close) based on feedback 4. keyboard support Once these are somehow resolved, we could even evaluate making this the default behavior. So far this can land because it's behing an about:config pref, so the risk is low, while the benefit for users may be high, provided this is just a first step towards the final shape of it.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/53a1b44951cc Introduce pref browser.bookmarks.openInTabClosesMenu to optionally leave menu open for single bookmark menuitem clicks that open in new tab. r=mak