Closed Bug 986866 Opened 6 years ago Closed 6 years ago

Show All Bookmarks - un-discoverable in Australis

Categories

(Firefox :: Bookmarks & History, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- wontfix
firefox30 + verified
firefox31 + verified
firefox32 + verified

People

(Reporter: jmjjeffery, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: polish, regression, Whiteboard: [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa!] [bugday-20140514])

Attachments

(2 files, 2 obsolete files)

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.
For consistency the same would have to happen for the history button menu item "Show All History" too.
dupe of bug 985024?
(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.
Blocks: 966905
No longer blocks: 939943
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?
Flags: needinfo?(shorlander)
Whiteboard: ? → [Australis:P3-]
Bug 985024 is in the backlog so duping there.

Stephen, please leave your comments in that bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 985024
(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.
(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.
Assignee: nobody → gijskruitbosch+bugs
Status: RESOLVED → REOPENED
Flags: needinfo?(shorlander)
Resolution: DUPLICATE → ---
One option we discussed was duplicating it...
Attachment #8406061 - Flags: review?(mak77)
Or we could move it. Needed a slight bit of CSS love.
Attachment #8406065 - Flags: review?(mak77)
argh, forgot to undo the rename.
Attachment #8406067 - Flags: review?(mak77)
Attachment #8406065 - Attachment is obsolete: true
Attachment #8406065 - Flags: review?(mak77)
Attachment #8406061 - Attachment description: duplicate the 'show all bookmarks' item, ui- → duplicate the 'show all bookmarks' item
Attachment #8406061 - Flags: ui-review?(shorlander)
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. :-(
Attachment #8406067 - Attachment description: move the 'show all bookmarks' item, ui- → move the 'show all bookmarks' item
Attachment #8406067 - Flags: ui-review?(shorlander)
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.
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.
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
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(shorlander)
Will track here while awaiting final design decision.
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.
Attachment #8406061 - Flags: ui-review?(shorlander) → ui-review+
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.
Attachment #8406061 - Flags: review?(mak77) → review+
Attachment #8406067 - Flags: review?(mak77)
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 :-) ).
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.
https://hg.mozilla.org/mozilla-central/rev/62a3249c2edd
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
I seem to have extra left padding on the "normal" menu item.
Depends on: 1008647
Can we get an uplift nomination here - and please address what, if anything, will be done about comment 22
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(shorlander)
Attachment #8406061 - Attachment is obsolete: true
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
Attachment #8422539 - Flags: approval-mozilla-beta?
Attachment #8422539 - Flags: approval-mozilla-aurora?
Attachment #8406061 - Attachment is obsolete: false
Attachment #8406067 - Attachment is obsolete: true
Still need to get this to beta/aurora and get it verified. Do think it warrants QA attention.
Flags: needinfo?(mmucci)
Whiteboard: [Australis:P3-] → [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa+]
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
Whiteboard: [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa+] → [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa+] [bugday-20140514]
Thanks Gijs.  Added to iteration and QA notified to add a contact for verification.
Flags: needinfo?(mmucci) → firefox-backlog+
QA Contact: camelia.badau
Attachment #8422539 - Flags: approval-mozilla-beta?
Attachment #8422539 - Flags: approval-mozilla-beta+
Attachment #8422539 - Flags: approval-mozilla-aurora?
Attachment #8422539 - Flags: approval-mozilla-aurora+
* 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
Whiteboard: [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa+] [bugday-20140514] → [Australis:P3-] p=2 s=it-32c-31a-30b.2 [qa!] [bugday-20140514]
Depends on: 1047891
You need to log in before you can comment on or make changes to this bug.