1090609 - bookmark folder open all does not work with only one url in the folder

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[gr06c206]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141023194920

Steps to reproduce:

I have a lot of bookmark folders, some contain more than one URL and some only contain one URL. The ones with one URL will not open when you do a middle mouse click. When you do a right click the open all in Tabs is grayed out so you can not select it. All of the bookmark folders with more than one URL work the way they are suppose to work. This all started with version 32 or 33 install. Tried on more than one computer. Same results.


Actual results:

Nothing happens. No errors. URL in the folder does not open.


Expected results:

URL should have opened in a new tab.

Comment 1

[:Gijs (away/busy until Mar 10)]

Marco, I wonder if I inadvertently broke this with bug 1006989 - or if this was changed intentionally?

Doing a regression hunt at the minute...

Comment 2

[Marco Bonardo [:mak]]

it should not be shown if there's only one bookmark, so I'd not expect it to work.

I think the problem is now we are showing it?

Comment 3

[:Gijs (away/busy until Mar 10)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> it should not be shown if there's only one bookmark, so I'd not expect it to
> work.
> 
> I think the problem is now we are showing it?

We were always showing it, but it used to be enabled (32) and now it's not (33+).

Comment 4

[Marco Bonardo [:mak]]

ah wait, this is the contextual menu. so it is likely greyed out, not hidden.
I honestly don't remember if before we were allowing that... off-hand sounds like if you middle-click you still don't know if there is 1 or more bookmarks inside, so it should probably work...

Comment 5

[Marco Bonardo [:mak]]

likely something here

177      // Set Open Folder/Links In Tabs items enabled state if they're visible
   2.178 -    if (anyVisible) {
   2.179 +    if (usableItemCount > 0) {
   2.180        var openContainerInTabsItem = document.getElementById("placesContext_openContainer:tabs");
   2.181 -      if (!openContainerInTabsItem.hidden && this._view.selectedNode &&
   2.182 -          PlacesUtils.nodeIsContainer(this._view.selectedNode)) {
   2.183 -        openContainerInTabsItem.disabled =
   2.184 -          !PlacesUtils.hasChildURIs(this._view.selectedNode);
   2.185 -      }
   2.186 -      else {
   2.187 -        // see selectiontype rule in the overlay
   2.188 -        var openLinksInTabsItem = document.getElementById("placesContext_openLinks:tabs");
   2.189 -        openLinksInTabsItem.disabled = openLinksInTabsItem.hidden;
   2.190 +      if (!openContainerInTabsItem.hidden) {
   2.191 +        var containerToUse = this._view.selectedNode || this._view.result.root;
   2.192 +        if (PlacesUtils.nodeIsContainer(containerToUse)) {
   2.193 +          if (!PlacesUtils.hasChildURIs(containerToUse, true)) {
   2.194 +            openContainerInTabsItem.disabled = true;
   2.195 +            // Ensure that we don't display the menu if nothing is enabled:
   2.196 +            usableItemCount--;
   2.197 +          }
   2.198 +        }
   2.199        }
   2.200      }
   2.201  
   2.202 -    return anyVisible;
   2.203 +    return usableItemCount > 0;
   2.204    },

Comment 6

[Marco Bonardo [:mak]]

if (!PlacesUtils.hasChildURIs(containerToUse, true)) 

afaict the second argument is asking for more than one entry now

Comment 7

[:Gijs (away/busy until Mar 10)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1dc6b294800d&tochange=c45f6e6d6005

Comment 8

[:Gijs (away/busy until Mar 10)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> if (!PlacesUtils.hasChildURIs(containerToUse, true)) 
> 
> afaict the second argument is asking for more than one entry now

This sounds plausible. It seems that despite my protests in bug 1006989 comment 22 and/or Mano's reply in 24, I made it work this way. I agree that at least middle click should work. Less sure about "open all in tabs", but I guess we might as well? Not sure if it's very hard to only fix one and not the other, in fact...

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> > if (!PlacesUtils.hasChildURIs(containerToUse, true)) 
> > 
> > afaict the second argument is asking for more than one entry now
> 
> This sounds plausible. It seems that despite my protests in bug 1006989
> comment 22 and/or Mano's reply in 24, I made it work this way. I agree that
> at least middle click should work. Less sure about "open all in tabs", but I
> guess we might as well? Not sure if it's very hard to only fix one and not
> the other, in fact...

Considered that you can open the context menu or middle click on a folder even if its contents are not shown (in the toolbar, in the Library and in the sidebar, at least), I think it's not nice to disable or hide that entry.
The user cannot know if the folder contains 1 or more entries, so the behavior would become unpredictable.
I'd just restore things as they were. The fact this item could be active when there's only one child is not that much problematic.
I'd totally revert the change to hasChildURIs, it was not even related to what Bug 1006989 was aimed to do.

Comment 10

[:Gijs (away/busy until Mar 10)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eddfa10bb5dc

Comment 11

[:Gijs (away/busy until Mar 10)]

Attachment: don't disable open in tabs when there's only one item there,

So like this? Try push above; this seems to work in local testing, at least.

Comment 12

[:Gijs (away/busy until Mar 10)]

(In reply to :Gijs Kruitbosch from comment #10)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eddfa10bb5dc

This is green.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8515927 [details] [diff] [review]
don't disable open in tabs when there's only one item there,

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

It would be nice if we could have a test checking the open all in tabs item is disabled if there's no children or enabled otherwise.

You could maybe modify http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_middleclick.js#107 so that it will only add a single bookmark into the folder, rather than 2. that could be enough to cover this change (please check).

Comment 14

[:Gijs (away/busy until Mar 10)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13)
> Comment on attachment 8515927 [details] [diff] [review]
> don't disable open in tabs when there's only one item there,
> 
> Review of attachment 8515927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be nice if we could have a test checking the open all in tabs item
> is disabled if there's no children or enabled otherwise.
> 
> You could maybe modify
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> tests/browser/browser_library_middleclick.js#107 so that it will only add a
> single bookmark into the folder, rather than 2. that could be enough to
> cover this change (please check).

I tried this, but using an additional test that copies the existing 2-bookmark folder approach and uses only 1 uri passes against current fx-team tip, without the patch.

Do you want me to hold off landing this to create a proper test? (totally fair, but just checking!)

Comment 15

[Marco Bonardo [:mak]]

no, I think it's worth fixing a regression that bites our users first.

Please file a follow-up in Firefox/Bookmarks&History to write a test for this, soon or later.

Comment 16

[:Gijs (away/busy until Mar 10)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/47f5be64e6a3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47f5be64e6a3

Comment 18

[:Gijs (away/busy until Mar 10)]

(note: I backed out a bit of test that verified the behaviour we implemented at the time; this bug needs its own test, which is bug 1094359, so I'm resetting in-testsuite)

Comment 19

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8515927 [details] [diff] [review]
don't disable open in tabs when there's only one item there,

Approval Request Comment
[Feature/regressing bug #]: bug 1006989
[User impact if declined]: can't middle-click / right click -> open all in tabs on bookmark folders with only one item
[Describe test coverage new/current, TBPL]: limited :-(
[Risks and why]: very limited, basically a selective backout of an earlier fix from bug 1006989
[String/UUID change made/needed]: no

Comment 20

[:Gijs (away/busy until Mar 10)]

> This has not been resolved. Does anyone have an idea when it will be fixed?

Firefox 36 (current developer edition), maybe 35 (current beta) if it gets approval to be uplifted.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0cf5aeed832e