Closed Bug 1398992 Opened 7 years ago Closed 6 years ago

'Recently Bookmarked' items in Library button's Bookmarks subview should respect pref browser.bookmarks.openInTabClosesMenu

Categories

(Firefox :: Menus, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: tawn, Assigned: tawn, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Follow-up to Bug 260611. Ctrl-click, middle-click, and the contextmenu's 'Open in a New Tab' should keep the menu open for 'Recently Bookmarked' items when accessed from the Library Button, if browser.bookmarks.openInTabClosesMenu is set to false.
Component: Bookmarks & History → Menus
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Blocks: 1400521
Patch makes Ctrl-click & middle-click keep 'Recently Bookmarked' menu/panel open if browser.bookmarks.openInTabClosesMenu set to false. 'Open in a new Tab' on contextmenu leaves panel open regardless of pref (see Bug 260611 comment #86 where Marco says this is "nice to have". I haven't located what causes that behavior nor altered it.)
Pref only applies to bookmarks, so exempting 'Recent Highlights' as those are not necessarily bookmarks.
Flags: needinfo?(jaws)
Attachment #8913019 - Flags: review?(jaws)
Assignee: nobody → stayopenmenu
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Comment on attachment 8913019 [details] [diff] [review]
Bug1398992- Photon panels support for browser.bookmarks.openInTabClosesMenu

Review of attachment 8913019 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine but I mainly have a question on the second part. I'll hold off marking this as r- or r+ until the question is answered.

::: browser/components/places/content/browserPlacesViews.js
@@ +2161,5 @@
> +      if (button.parentNode.id == "panelMenu_bookmarksMenu") {
> +        button.setAttribute("closemenu", "none");
> +      }
> +    } else if (button.hasAttribute("closemenu")) {
> +        button.removeAttribute("closemenu");

We don't need to check if the button has the attribute before removing it. You can just change this to:

} else {
  button.removeAttribute("closemenu");
}

@@ +2167,4 @@
>      PlacesUIUtils.openNodeWithEvent(button._placesNode, event);
> +    // Unlike left-click, middle-click requires manual menu closing.
> +    if (button.parentNode.id != "panelMenu_bookmarksMenu" ||
> +        (event.type == "click" && PlacesUIUtils.openInTabClosesMenu)) {

Do we need to check which button was clicked? The comment here makes it seem that this code should only be necessary for middle clicks. Is that a fair understanding?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)

> ::: browser/components/places/content/browserPlacesViews.js
> @@ +2161,5 @@
> > +      if (button.parentNode.id == "panelMenu_bookmarksMenu") {
> > +        button.setAttribute("closemenu", "none");
> > +      }
> > +    } else if (button.hasAttribute("closemenu")) {
> > +        button.removeAttribute("closemenu");
> 
> We don't need to check if the button has the attribute before removing it.
> You can just change this to:
> 
> } else {
>   button.removeAttribute("closemenu");
> }


Ok, thought it might make code faster, if attr not present

> >      PlacesUIUtils.openNodeWithEvent(button._placesNode, event);
> > +    // Unlike left-click, middle-click requires manual menu closing.
> > +    if (button.parentNode.id != "panelMenu_bookmarksMenu" ||
> > +        (event.type == "click" && PlacesUIUtils.openInTabClosesMenu)) {
> 
> Do we need to check which button was clicked? The comment here makes it seem
> that this code should only be necessary for middle clicks. Is that a fair
> understanding?

Yes, it is only necessary for middle clicks. But see first part of patch:

   handleEvent(event) {
     switch (event.type) {
       case "click":
-        // For left and middle clicks, fall through to the command handler.
-        if (event.button >= 2) {
+        // For middle clicks, fall through to the command handler.
+        if (event.button != 1) {
           break;
         }
       case "command":

So if the event.type == "click" in the second part, it implies that it is a middle click since if it's not, we don't fall through to the command handler at all.
(In reply to custom.firefox.lady from comment #3)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> 
> > ::: browser/components/places/content/browserPlacesViews.js
> > @@ +2161,5 @@
> > > +      if (button.parentNode.id == "panelMenu_bookmarksMenu") {
> > > +        button.setAttribute("closemenu", "none");
> > > +      }
> > > +    } else if (button.hasAttribute("closemenu")) {
> > > +        button.removeAttribute("closemenu");
> > 
> > We don't need to check if the button has the attribute before removing it.
> > You can just change this to:
> > 
> > } else {
> >   button.removeAttribute("closemenu");
> > }
> 
> 
> Ok, thought it might make code faster, if attr not present

Not any measurable difference so we should prefer the simpler code.

> > >      PlacesUIUtils.openNodeWithEvent(button._placesNode, event);
> > > +    // Unlike left-click, middle-click requires manual menu closing.
> > > +    if (button.parentNode.id != "panelMenu_bookmarksMenu" ||
> > > +        (event.type == "click" && PlacesUIUtils.openInTabClosesMenu)) {
> > 
> > Do we need to check which button was clicked? The comment here makes it seem
> > that this code should only be necessary for middle clicks. Is that a fair
> > understanding?
> 
> Yes, it is only necessary for middle clicks. But see first part of patch:
> 
>    handleEvent(event) {
>      switch (event.type) {
>        case "click":
> -        // For left and middle clicks, fall through to the command handler.
> -        if (event.button >= 2) {
> +        // For middle clicks, fall through to the command handler.
> +        if (event.button != 1) {
>            break;
>          }
>        case "command":
> 
> So if the event.type == "click" in the second part, it implies that it is a
> middle click since if it's not, we don't fall through to the command handler
> at all.

Okay, but what code is going to run for left-clicks now?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Okay, but what code is going to run for left-clicks now?

Left clicks get entirely handled by _onCommand when it gets triggered by the command evt. 

When I first tried disabling the menu closing, I found that clicks got handled by _onCommand in the command evt, then re-handled by _onCommand for the click evt. This caused 2 (duplicate) tabs to be opened. My best guess is that explicitly closing the menu at "command" somehow prevented the click evt from being handled, thus preventing the duplicate tab from being opened.

BTW, do you want us to set needinfo on every comment or just on stuff that isn't a reply to your question (which I assume you're already expecting)?
Flags: needinfo?(jaws)
Okay that makes more sense. I'll look at the patch tomorrow after I get a nights rest.

If the bug has a patch with an open review request for myself I'll see the comments without a needinfo request. Otherwise the needinfo request is very helpful.
Flags: needinfo?(jaws)
Comment on attachment 8913019 [details] [diff] [review]
Bug1398992- Photon panels support for browser.bookmarks.openInTabClosesMenu

Review of attachment 8913019 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for answering all of my questions. I'll grant r+ after looking at this one more time. Please don't forget to clean up the `hasAttribute / removeAttribute` lines that I commented on earlier.

::: browser/components/places/content/browserPlacesViews.js
@@ +2167,4 @@
>      PlacesUIUtils.openNodeWithEvent(button._placesNode, event);
> +    // Unlike left-click, middle-click requires manual menu closing.
> +    if (button.parentNode.id != "panelMenu_bookmarksMenu" ||
> +        (event.type == "click" && PlacesUIUtils.openInTabClosesMenu)) {

Okay, I see it now. The only time `event.type == "click"` is true is during middle-clicks since left-clicks generate `event.type == "command"`. This is heavily relying on how the `handleEvent` is implemented. I would prefer if you include an extra check here that `event.button == 1`.
Attachment #8913019 - Flags: review?(jaws) → review-
Comment on attachment 8914590 [details]
Bug 1398992 = Make 'Recently Bookmarked' in Photon panels support browser.bookmarks.openInTabClosesMenu pref.

https://reviewboard.mozilla.org/r/185934/#review190910
Attachment #8914590 - Flags: review?(jaws) → review+
Attempting to add a mochitest for this to browser_stayopenmenu.js. Need help with 2 issues.

diff --git a/browser/components/places/tests/browser/browser_stayopenmenu.js b/browser/components/places/tests/browser/browser_stayopenmenu.js
--- a/browser/components/places/tests/browser/browser_stayopenmenu.js
+++ b/browser/components/places/tests/browser/browser_stayopenmenu.js
@@ -122,6 +122,70 @@ add_task(async function testStayopenBook
   info("Closing menu");
   await BrowserTestUtils.removeTab(newTab);
 
+  // Test Library Button stayopen clicks
+  let libView = document.getElementById("appMenu-libraryView");
+  let libraryBtn = document.getElementById("library-button");
+  let ViewShowingPromise = BrowserTestUtils.waitForEvent(libView, "ViewShowing");
+  libraryBtn.click();
+  await ViewShowingPromise;
+  info("Library button's panel shown.");
+      await new Promise((resolve, reject) => {
+        setTimeout(resolve, 1000);
+      });
+
+  let bookmarks = document.getElementById("appMenu-library-bookmarks-button");
+  let BMview = document.getElementById("PanelUI-bookmarks");
+  ViewShowingPromise = BrowserTestUtils.waitForEvent(BMview, "ViewShowing");
+  bookmarks.click();
+  await ViewShowingPromise;
+  info("Library button's bookmarks panel shown.");
+
+  // Test Library Button's bookmarks panel stayopen clicks: Ctrl-click.
+  let menupopup = BMview.firstChild.lastChild;
+  for (let node of menupopup.childNodes) {
+    if (node.label == "Test1") {
+      info("found test bookmark")
+
+      await new Promise((resolve, reject) => {
+        setTimeout(resolve, 1000);
+      });
+      promiseTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, null);
+      EventUtils.synthesizeMouseAtCenter(node,
+        AppConstants.platform === "macosx" ? {metaKey: true} : {ctrlKey: true});
+      newTab = await promiseTabOpened;
+      ok(true, "Bookmark ctrl-click opened new tab.");
+      await BrowserTestUtils.removeTab(newTab);                                                                                                                                                
+      ok(libraryBtn.open, "Library button's  bookmarks panel should still be open.");                                                                                                          
+                                                                                                                                                                                               
+      // Test Library Button's bookmarks panel stayopen clicks: middle-click.                                                                                                                  
+      promiseTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, null);                                                                                                                       
+      EventUtils.synthesizeMouseAtCenter(node, {button: 1});                                                                                                                                   
+      newTab = await promiseTabOpened;                                                                                                                                                         
+      ok(true, "Bookmark middle-click opened new tab.");                                                                                                                                       
+      await BrowserTestUtils.removeTab(newTab);                                                                                                                                                
+      ok(libraryBtn.open, "Library button's  bookmarks panel should still be open.");                                                                                                          
+                                                                                                                                                                                               
+      // hide the bookmarks subview                                                                                                                                                            
+      BMview.hidden = true;                                                                                                                                                                    
+
+      // Close the Library button's panel
+
+      // (empty panel remains when using either of the following)
+      // libraryBtn.click();
+      // PanelUI.hide();
+
+      // any of the below closes panel, but causes the following failure:
+      // A promise chain failed to handle a rejection: viewNode.panelMultiView is undefined - stack: null
+      // JavaScript error: chrome://browser/content/customizableui/panelUI.js, line 542: TypeError: viewNode.panelMultiView is undefined
+
+      // EventUtils.sendKey("escape");
+      // CustomizableUI.hidePanelForNode(node);
+      // EventUtils.synthesizeMouseAtCenter(node, {button: 0});
+
+      ok(!libraryBtn.open, "Library button's  bookmarks panel should be closed.");
+      break;
+    }
+  }
+
   // Disable the rest of the tests on Mac due to Mac's handling of menus being
   // slightly different to the other platforms.
   if (AppConstants.platform === "macosx") {


Issues:
1. What evt should I be waiting for instead of using the timeouts? I guessed possibly transitionend, but if so, don't know what obj to set it on:
  BrowserTestUtils.waitForEvent(???, "transitionend");


2. Closing the menu when I'm done with the test - see comments in code for what I've tried and their effects. (Effects are the same whether I hide the bookmarks subview first or not.)
Flags: needinfo?(jaws)
Mentor: jaws
Flags: needinfo?(jaws)
(In reply to custom.firefox.lady from comment #10)

I'm so sorry for not landing your patch earlier after I granted r+. Can you include your test as part of your patch that you pushed to this bug?

> Attempting to add a mochitest for this to browser_stayopenmenu.js. Need help
> with 2 issues.

Great!
 
> +  libraryBtn.click();
> +  await ViewShowingPromise;
> +  info("Library button's panel shown.");
> +      await new Promise((resolve, reject) => {
> +        setTimeout(resolve, 1000);
> +      });

Instead of doing a 1-second timeout, you can wait for the "ViewShown" event which is dispatched on the subview node. This is dispatched at http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/browser/components/customizableui/PanelMultiView.jsm#607

> +  for (let node of menupopup.childNodes) {
> +    if (node.label == "Test1") {
> +      info("found test bookmark")

Since you are only looking for this specific child node, you could replace this loop and conditional with the following code:

let testMenuitem = [...menupopup.childNodes].find(node => node.label == "Test1");
ok(testMenuitem, "found test bookmark");

> +      await new Promise((resolve, reject) => {
> +        setTimeout(resolve, 1000);
> +      });

I'm not sure why this setTimeout is needed. How does the test fail without it?

> +      promiseTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, null);
> +      EventUtils.synthesizeMouseAtCenter(node,
> +        AppConstants.platform === "macosx" ? {metaKey: true} : {ctrlKey:
> true});

You should be able to use {accelKey: true} and it will work for both OSX and non-OSX platforms.

> +
> +      // Close the Library button's panel
> +
> +      // (empty panel remains when using either of the following)
> +      // libraryBtn.click();
> +      // PanelUI.hide();

You should get a reference to the <popup> element and then you can call popup.hidePopup();
Attachment #8913019 - Attachment is obsolete: true
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)

> I'm so sorry for not landing your patch earlier after I granted r+. Can you
> include your test as part of your patch that you pushed to this bug?

Yes. (Figured I'd wait until I had test done, before asking for check-in. Since I did tests for the bug this was a follow-up to, I assumed we'd want one for this too.) Should I have waited until both were done before submitting patch for review?

> Instead of doing a 1-second timeout, you can wait for the "ViewShown" event
> which is dispatched on the subview node. This is dispatched at
> http://searchfox.org/mozilla-central/rev/
> 1285ae3e810e2dbf2652c49ee539e3199fcfe820/browser/components/customizableui/

Thanks; I needed to wait for ViewShown instead of ViewShowing in both instances.

> > +  for (let node of menupopup.childNodes) {
> > +    if (node.label == "Test1") {
> > +      info("found test bookmark")
> 
> Since you are only looking for this specific child node, you could replace
> this loop and conditional with the following code:
> 
> let testMenuitem = [...menupopup.childNodes].find(node => node.label ==
> "Test1");
> ok(testMenuitem, "found test bookmark");

Gotta love ES6!

> > +      // Close the Library button's panel
> > +
> > +      // (empty panel remains when using either of the following)
> > +      // libraryBtn.click();
> > +      // PanelUI.hide();
> 
> You should get a reference to the <popup> element and then you can call
> popup.hidePopup();

Except that I couldn't find the Library Button's popup and there was still a timeout I needed a fix for. So I took a slightly different approach and tested this indirectly through the App Menu...easy to close and no timeouts required. Working fine, but getting late; will submit patch when I get back to this, likely within a few days or so.
Flags: needinfo?(jaws)
Pushed patch with mochitest via moz-review; not sure if it's actually showing as needing review; can you take a look?
The changes look good. I've pushed this to tryserver now and will land it once I see that the tests are passing on tryserver. Flagging myself for needinfo so I won't forget it this time.
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
The trypush shows a lot of failures. I'm going to rebase this to the mozilla-central tip and re-push to tryserver.
Flags: needinfo?(jaws)
Here's the trypush,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cd1e69f0a46e64ee573181b89d951f631c703cb

browser/components/customizableui/test/browser_947914_button_history.js failed on all platforms with many timeouts. tawn, would you be able to look at fixing the test failure?
Flags: needinfo?(stayopenmenu)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> would you be able to look
> at fixing the test failure?

Didn't see anything obvious from a quick look; will investigate further when I get time.
Flags: needinfo?(stayopenmenu)
Just requires a minor adjustment to the failing test due to tab now being opened via command instead of click handler. Updated patch and submitted new try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa2b68db9252b366a59597963d48145bd8932f6
Flags: needinfo?(jaws)
Comment on attachment 8914590 [details]
Bug 1398992 = Make 'Recently Bookmarked' in Photon panels support browser.bookmarks.openInTabClosesMenu pref.

https://reviewboard.mozilla.org/r/185934/#review208550

Ah, makes sense! Thanks!
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b80bf7e61354
= Make 'Recently Bookmarked' in Photon panels support browser.bookmarks.openInTabClosesMenu pref. r=jaws
https://hg.mozilla.org/mozilla-central/rev/b80bf7e61354
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.