986866 - Show All Bookmarks - un-discoverable in Australis

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Now that Australis has moved into a higher group of testing in Aurora I have seen several posts concerning 'Show All Bookmarks', being 'missing', or questions like:  How do I backup my Bookmarks.  Support documents show that you can click on the Bookmarks 'button' and use: Show All Bookmarks to open the Library where manual backups can be made. 

People are clicking the bookmarks button and not seeing Show All bookmarks since it now resides at the very bottom of the bookmarks list. 

It was also pointed out here: 
https://bugzilla.mozilla.org/show_bug.cgi?id=627325#c1 
That in his opinion it would not be good to move it the bottom, yet that's where it wound up.  

An example of user that is confused:
http://forums.mozillazine.org/viewtopic.php?f=38&t=2812901

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=625325 comment #0 for list of ordering in Bookmarks button, Show All Bookmarks is at the top, and that was Alex's first take in the lead in/run-up to getting the Australis ball rolling, I believe.  

The move to put the Show all Bookmarks at the Bottom of the Bookmarks list should be re-considered as to its placement I believe.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

For consistency the same would have to happen for the history button menu item "Show All History" too.

Comment 2

[Marco Bonardo [:mak]]

dupe of bug 985024?

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Marco Bonardo [:mak] from comment #2)
> dupe of bug 985024?

Would likely seem so... I'll let you decide which bug to continue working on.

Comment 4

[:Gijs (he/him)]

Stephen, I know you said in bug 966905 that even in the current case where the footer is in the scrollbox, it should stay at the bottom, but this is proving to be making life complicated for people who have/had a lot of bookmarks in this menu, and now think we just removed the Library entirely. See also bug 985024 comment 0.

Can we either:
- move the item back to the top until we can fix bug 985024;
- duplicate the item to the top, and hide it if the menu isn't scrollable? (I *think* this is possible, but I've not actually tried it...) until we can fix bug 985024?

Comment 5

[Matthew N. [:MattN]]

Bug 985024 is in the backlog so duping there.

Stephen, please leave your comments in that bug.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Matthew N. [:MattN] from comment #5)
> Bug 985024 is in the backlog so duping there.
> 
> Stephen, please leave your comments in that bug.
> 
> *** This bug has been marked as a duplicate of bug 985024 ***

I actually thought we were keeping this bug for a quick workaround that could make Beta/Aurora and bug 985024 for the "proper" fix.

Comment 7

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #6)
> (In reply to Matthew N. [:MattN] from comment #5)
> > Bug 985024 is in the backlog so duping there.
> > 
> > Stephen, please leave your comments in that bug.
> > 
> > *** This bug has been marked as a duplicate of bug 985024 ***
> 
> I actually thought we were keeping this bug for a quick workaround that
> could make Beta/Aurora and bug 985024 for the "proper" fix.

Yes. I think this makes sense, and per last week's Australis meeting, I'm going to undupe this bug and try to workaround here.

Comment 8

[:Gijs (he/him)]

Attachment: duplicate the 'show all bookmarks' item

One option we discussed was duplicating it...

Comment 9

[:Gijs (he/him)]

Attachment: move the 'show all bookmarks' item, ui-

Or we could move it. Needed a slight bit of CSS love.

Comment 10

[:Gijs (he/him)]

Attachment: move the 'show all bookmarks' item

argh, forgot to undo the rename.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8406067 [details] [diff] [review]
move the 'show all bookmarks' item

Stephen, can you indicate which of these two options you prefer?

I did also look at having both present and hiding whichever one was unnecessary, but it looks like that'd be complicated enough that I don't think we should risk it for beta at this point. :-(

Comment 12

[Marco Bonardo [:mak]]

I think duplicating it makes the menu a little bit "funny" when you have just a handful of bookmarks into it... duplicating only when it's scrolled up by a give amount would work better... but it's clearly magic.
Honestly for now I'd just put it back at the top where it was before and leave this menu without a footer, until there's a proper fix.
Btw, let's first see what Stephen thinks.

Comment 13

[Marco Bonardo [:mak]]

Since this was shipped (and I keep seeing users asking why we removed bookmarks features with Australis) may we please workaround this for Firefox 30+ at least?

I think the priority of this bug is under-evaluated.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Gijs, I know you are waiting for an ui review but do you think you could ask the review to someone else to progress on this bug? Thanks

Comment 15

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Gijs, I know you are waiting for an ui review but do you think you could ask
> the review to someone else to progress on this bug? Thanks

No, we need Stephen to reply and decide which of these options to prefer, or if we want to WONTFIX this bug. The other review is waiting on Marco himself. :-)

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track here while awaiting final design decision.

Comment 17

[Stephen Horlander [:shorlander]]

Comment on attachment 8406061 [details] [diff] [review]
duplicate the 'show all bookmarks' item

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

We should just duplicate it for now until we can get the proper fix. This will result in less moving of the item between releases.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8406061 [details] [diff] [review]
duplicate the 'show all bookmarks' item

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

::: browser/base/content/browser.xul
@@ +809,1 @@
>              <menuitem id="BMB_viewBookmarksSidebar"

Since this is just a temporary solution, to avoid making users crazy with continuous changes, I suggest to put this duplicate Show All Bookmarks _after_ View bookmarks sidebar, users were used to have show all bookmarks at the top, now we put the sidebar there, now we'd put again SAB just to remove it in near future.

So, let's keep the sidebar as the first entry and put 2nd Show all bookmarks as the second one.
Also please put a comment above it explaining this is a temporary workaround for bug 985024.

Let's try to bring this up to Beta too.

Comment 19

[:Gijs (he/him)]

Apologies for the delay here.

 https://hg.mozilla.org/integration/fx-team/rev/62a3249c2edd

with the item below the sidebar + a comment. Stephen, let me know if that's not OK (I think Marco's reasons are excellent, so I just landed it :-) ).

Comment 20

[Marco Bonardo [:mak]]

hm my fault, I should have stated that there should not be the need for a menuseparator between view bookmarks sidebar and show all bookmarks, they are non disruptive options and both open a viewer, so I don't think there's need to lose vertical space here.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62a3249c2edd

Comment 22

[Tim Nguyen :ntim]

I seem to have extra left padding on the "normal" menu item.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get an uplift nomination here - and please address what, if anything, will be done about comment 22

Comment 24

[:Gijs (he/him)]

(In reply to Lukas Blakk [:lsblakk] from comment #23)
> Can we get an uplift nomination here - and please address what, if anything,
> will be done about comment 22

This was filed as bug 1008647. I don't think we should uplift without fixing that, which I hope to do tomorrow. I'll request uplift for the combined patch.

Comment 25

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Lukas Blakk [:lsblakk] from comment #23)
> > Can we get an uplift nomination here - and please address what, if anything,
> > will be done about comment 22
> 
> This was filed as bug 1008647. I don't think we should uplift without fixing
> that, which I hope to do tomorrow. I'll request uplift for the combined
> patch.

Also, comment #20, which I will also address in a followup patch.

Comment 26

[:Gijs (he/him)]

Attachment: Patch for aurora/beta

Comment 27

[:Gijs (he/him)]

Comment on attachment 8422539 [details] [diff] [review]
Patch for aurora/beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: some users can't find the 'show all bookmarks' item in the bookmarks menu button's menu
Testing completed (on m-c, etc.): soon all on m-c
Risk to taking this patch (and alternatives if risky): medium-low; adds a duplicate item which might annoy users
String or IDL/UUID changes made by this patch: none

Comment 28

[:Gijs (he/him)]

Still need to get this to beta/aurora and get it verified. Do think it warrants QA attention.

Comment 29

[Francesca Ciceri [:madamezou]]

Hi,

I was able to reproduce it on Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140305030201 CSet: e5b09585215f.

I can confirm that everything is fixed in the latest Nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140514030203 CSet: 2f8af55d6e9a.

Cheers,
Francesca

Comment 30

[Marco Mucci [:MarcoM]]

Thanks Gijs.  Added to iteration and QA notified to add a contact for verification.

Comment 31

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bb67b376349b
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d087200dd934

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d087200dd934

Comment 33

[Camelia Badau [:cbadau], Desktop QA]

* Verified fixed on Windows 7 64bit,32bit, Ubuntu 13.10 32bit and Mac OS X 10.7.5 using:
#latest Aurora 31.0a2, build ID: 20140516004003  
#latest Nightly 32.0a1, build ID: 20140516030204
#Fx 30 beta 5, build ID: 20140515140857