Closed Bug 1420749 Opened 6 years ago Closed 6 years ago

Extend browser.bookmarks.openInTabClosesMenu to work with folders/'Open All in Tabs' (containers)

Categories

(Firefox :: Bookmarks & History, enhancement, P5)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: colonel.panic.code, Assigned: tawn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

* set browser.bookmarks.openInTabClosesMenu to false
* create a bookmark folder
* place some bookmarks in it
* on the bookmark toolbar, middle-click the folder


Actual results:

The bookmarks opened and the menu closed


Expected results:

the bookmarks open and the menu remains open
This occurs via the following methods:
* middle-clicking a bookmark folder
* right-clicking the bookmark folder and choosing "Open All in Tabs"
* navigating to the submenu and left-clicking "Open All in Tabs"
Component: Untriaged → Bookmarks & History
AFAICT bug 260611 didn't intend for this to be applied to the bookmarks toolbar, only the bookmarks menu (toolbar button / main menu). Therefore changing the summary to reflect this better.
Blocks: 1400521
Severity: normal → enhancement
Priority: -- → P5
Summary: browser.bookmarks.openInTabClosesMenu not respected when the target is a folder or "Open All in Tabs" → Extend browser.bookmarks.openInTabClosesMenu to work with folders in the bookmarks toolbar as well
(In reply to Mark Banner (:standard8) from comment #2)
> AFAICT bug 260611 didn't intend for this to be applied to the bookmarks
> toolbar, only the bookmarks menu (toolbar button / main menu). Therefore
> changing the summary to reflect this better.

Actually bug 260611 comment #50 specifically excluded 'containers', i.e. folders or anything other than single menuitems. So it's not related to where the folder is, but just the very fact that it's a folder (containing multiple items). From the original code's comments (prior to bugfix) the menu must be closed to prevent hiding the 'opening x tabs will slow down Firefox' dialog, but my experience was that the the dialog itself would cause the menu to be closed (at least on Linux). According to point 3 in the description for Bug 1400521 "Decide what to do with containers (close, not close) based on feedback" a decision on this still needs to be made.
(In reply to custom.firefox.lady [:tawn] from comment #3)
> (In reply to Mark Banner (:standard8) from comment #2)
> > AFAICT bug 260611 didn't intend for this to be applied to the bookmarks
> > toolbar, only the bookmarks menu (toolbar button / main menu). Therefore
> > changing the summary to reflect this better.
> 
> Actually bug 260611 comment #50 specifically excluded 'containers', i.e.
> folders or anything other than single menuitems. So it's not related to
> where the folder is, but just the very fact that it's a folder (containing
> multiple items). From the original code's comments (prior to bugfix) the
> menu must be closed to prevent hiding the 'opening x tabs will slow down
> Firefox' dialog, but my experience was that the the dialog itself would
> cause the menu to be closed (at least on Linux). According to point 3 in the
> description for Bug 1400521 "Decide what to do with containers (close, not
> close) based on feedback" a decision on this still needs to be made.

Container/Folders should stay open if the pref is flipped, frankly is better that way
Chrome already does this to some extent.
This bug is marked P5; does that mean a decision has been made that we do want it? (compare point 3 in the description for Bug 1400521 "Decide what to do with containers (close, not close) based on feedback.) If we do want this, I'd be willing to submit a patch (should require only relatively minor changes).

Also the summary is incorrect; I'd recommend something like "Extend browser.bookmarks.openInTabClosesMenu to work with folders/'Open All in Tabs' (containers)" As I mentioned in comment #3, the issue is not related to where the folder resides.
Flags: needinfo?(mak77)
(In reply to shellye from comment #4)
> Chrome already does this to some extent.

I just tried, and Chrome seems to always close menupopups when opening in a new tab from the toolbar. So I'm not sure I see what you mean.

(In reply to custom.firefox.lady [:tawn] from comment #5)
> This bug is marked P5; does that mean a decision has been made that we do
> want it?

Not exactly since it's unconfirmed.
If the patch is simple enough (so in case of problems we can easily back it out), we could surely consider it and gather feedback.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mak77)
Summary: Extend browser.bookmarks.openInTabClosesMenu to work with folders in the bookmarks toolbar as well → Extend browser.bookmarks.openInTabClosesMenu to work with folders/'Open All in Tabs' (containers)
(In reply to Marco Bonardo [::mak] from comment #6)
> (In reply to shellye from comment #4)
> > Chrome already does this to some extent.
> 
> I just tried, and Chrome seems to always close menupopups when opening in a
> new tab from the toolbar. So I'm not sure I see what you mean.


it's a experimental flag(keep the menu open always)
I see, I'm fine to keep chrome-parity on the feature.
Attachment #8949625 - Flags: review?(mak77)
Comment on attachment 8949625 [details]
Bug 1420749 - Allow browser.bookmarks.openInTabClosesMenu to work with containers as well as single menuitems

https://reviewboard.mozilla.org/r/218978/#review225522
Attachment #8949625 - Flags: review?(mak77) → review+
Assignee: nobody → stayopenmenu
Status: NEW → ASSIGNED
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/f26d2eac74fb
Allow browser.bookmarks.openInTabClosesMenu to work with containers as well as single menuitems r=mak
https://hg.mozilla.org/mozilla-central/rev/f26d2eac74fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
custom.firefox.lady 
Marco Bonardo and everyone else involved
big thank you for this and hoping the other bug 1400521 is fixed soon too.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.