1219788 - Show bookmarks toolbar when adding a bookmark to the bookmarks toolbar folder

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Verdi [:verdi] Best to slack me]

If someone imports their bookmarks from another browser on first run and bookmarks are added to the toolbar folder we automatically show the toolbar folder for them. This is nice. 

However, those that don't have the bookmarks toolbar shown and then later file a bookmark in the bookmarks toolbar folder are surprised to find that we don't automatically show the toolbar. In tests, people have found this and the process of showing the toolbar confusing. We should automatically show the bookmarks toolbar if someone adds a bookmark to the toolbar folder and they don't already have the toolbar shown.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8699447 [details] [diff] [review]
patch

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

::: browser/base/content/browser-places.js
@@ +1826,5 @@
>  };
> +
> +var AutoShowBookmarksToolbar = {
> +  init() {
> +    PlacesUtils.bookmarks.addObserver(this, false);

This enforces initialization of the bookmarks service, ideally in the startup path we try to initialize it only when it's strictly needed (like if bookmarks work is about to happen)
You can use PlacesUtils.add/RemoveLazyBookmarkObserver instead

It would also be nice if we could observe only if the toolbar is not visible... each added observer adds a cost to bookmarks notifications. Can we merge this into PlacesToolbarHelper and use its knowledge to observe only when the toolbar was not initialized?

@@ +1832,5 @@
> +  uninit() {
> +    PlacesUtils.bookmarks.removeObserver(this);
> +  },
> +  onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle) {
> +    this._autoshow(aParentId);

I'd prefer if this would use aParentGuid and compare with PlacesUtils.toolbarGUID (ids are sort-of deprecated)

@@ +1840,5 @@
> +  onItemRemoved() {},
> +  onItemChanged() {},
> +  onItemVisited() {},
> +  onItemMoved(aItemId, aOldFolderId, aOldIndex, aNewFolderId, aNewIndex, aItemType) {
> +    this._autoshow(aNewFolderId);

ditto

::: browser/base/content/browser.js
@@ +1472,5 @@
>  
>      PlacesToolbarHelper.uninit();
>  
>      BookmarkingUI.uninit();
> +    AutoShowBookmarksToolbar.uninit();

nit: both of these should actually happen in the } else { branch of onUnload, since they are inited in delayedStartup (

Comment 3

[Marco Bonardo [:mak]]

ehr sorry, PlacesUtils.bookmarks.toolbarGuid

Comment 4

[Dão Gottwald [:dao]]

Attachment: autoshow-toolbarvisibilitychange.diff

This doesn't work reliably... e.g. when I show the toolbar, open a second window, hide the toolbar and add a bookmark, the toolbar is only made visible in the first window and the second one never seems to get the observer callback. Maybe you can spot if I did something wrong, otherwise I guess PlacesUtils.addLazyBookmarkObserver is just broken.

Comment 5

[Dão Gottwald [:dao]]

Attachment: autoshow.diff

This always adds the lazy observer right away. It's simpler and works.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8702876 [details] [diff] [review]
autoshow.diff

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

Considered the most common case should be an hidden toolbar (the default) and considering there are better alternatives to avoid the tiny perf hit (in future we could unify some ui bookmarks observers into a single dispatcher) I think this is fine.

There is only one thing that worries me, the fact this could cause us to show the toolbar in mochitest-browser tests that add a bookmark to the toolbar, next tests could not expect the toolbar. Did you already check this on the Try server?

Comment 7

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #4)
> This doesn't work reliably... e.g. when I show the toolbar, open a second
> window, hide the toolbar and add a bookmark, the toolbar is only made
> visible in the first window and the second one never seems to get the
> observer callback

I'm not sure, but the fact toolbar changes happen in all the windows but each window has an handler and tracks whether it should observe separately sounds like complex enough to cause bugs. So let's just take the simpler path and move on.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] from comment #6)
> There is only one thing that worries me, the fact this could cause us to
> show the toolbar in mochitest-browser tests that add a bookmark to the
> toolbar, next tests could not expect the toolbar. Did you already check this
> on the Try server?

all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f622a0df261

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4e9389db3a5a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e9389db3a5a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Had to back this out after realizing it caused M(JP) LSAN permaleaks that were being starred under the guise of an old intermittent.
https://treeherder.mozilla.org/logviewer.html#?job_id=6386467&repo=fx-team

https://hg.mozilla.org/mozilla-central/rev/ce643acfab14

Comment 12

[Dão Gottwald [:dao]]

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

Looks like moving BookmarkingUI.uninit to the "right" place caused the leak. I'm gonna reland without that change.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/44f79bbe2b51

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44f79bbe2b51

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a640ab7f2457
> 
> Looks like moving BookmarkingUI.uninit to the "right" place caused the leak.
> I'm gonna reland without that change.

Filed bug 1237569 on this.

Comment 16

[Mihai Boldan, Desktop QA [:mboldan]]

I was able to reproduce this issue on Firefox 44.0a1(2015-10-01) and on Windows 7 x64.
This issue is no longer reproducible on Firefox 46.0a1 (2016-01-25) across platforms[1].
[1] - Windows 7 x64, Mac OS X 10.9.5, Ubuntu 12.04 x86

Comment 17

[Kestrel]

The visibility of the bookmarks toolbar can be improved but I don't think this is the right way to go about it.

1. It doesn't respect the user's choice to hide the toolbar.
2. Users that choose to show the toolbar only when necessary will have to fight against it.
3. When the user saves a bookmark they are unlikely to be interested in other bookmarks at that time.
4. Simply rearranging bookmarks in the sidebar or library that belong to the "Bookmarks Toolbar" group is enough to autoshow the toolbar.
5. Extensions that save bookmarks to the "Bookmarks Toolbar" group will cause the toolbar to autoshow.
6. When the toolbar autoshows it moves everything down, if this happens unexpectedly the user can lose their place or perform an action on the wrong item.
7. There is no obvious/easy way to hide the toolbar (eg close button, context menu (Bug 1003364) or hotkey (Bug 403956)), only several indirect methods that are either obscure or require significant mousing (navbar context menu, menubar, bookmarks menu button or Customize). 
8. The only opt-out is to move "Bookmark Toolbar Items" out of the toolbar (which is non-obvious) or not save bookmarks to the toolbar at all which sacrifices its convenience.
9. Chrome users are used to not having a bookmarks toolbar visible by default (they also have a context menu and hotkey to easily hide it) and there seems to be a widespread preference to minimize browser chrome as much as possible.

I think it makes more sense to show the bookmarks toolbar for new tabs (Bug 1244649) as this is where it is most likely needed and doesn't interfere so much with user preference.

Comment 18

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c296a2008eb

Comment 19

[Justin Dolske [:Dolske]]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c296a2008eb
https://hg.mozilla.org/releases/mozilla-aurora/rev/eac694075b3f


Carrying over some discussion from bug 1242756 and IRC with Marco:

Bug 1242756 was filed as a regression involving an obscure JS pref (probably changing some timing of how things run, as a result), and earlier today blassey reported also having his bookmarks toolbar mysteriously reappear on his systems (without that pref).

From discussion with Marco, the patch here isn't actually quite right. The somewhat subtle issue is that we only want to show the bookmark toolbar when the *user* adds a bookmark there, as a result of a user action. The current code will trigger whenever anything using Places adds a bookmark to the toolbar, which may or may not be a result of immediate user action. In particular, Marco says using Sync could trigger this... A sync'd bookmark coming over from another device shouldn't trigger showing the toolbar at some random time.

Bug 1242756 comment 10 suggests handling this in the UI (add bookmark panel), so that when the user adds a bookmark and explicitly files it into the toolbar, the code tells Places to add a bookmark there and separately shows the toolbar.

Comment 20

[Justin Dolske [:Dolske]]

Verdi: can you respond to the issues in comment 17? I think at least some of them will be fixed or adequately addressed by implementing this as intended, but there are some other concerns too.

Comment 21

[Verdi [:verdi] Best to slack me]

(In reply to Kestrel from comment #17)
> The visibility of the bookmarks toolbar can be improved but I don't think
> this is the right way to go about it.
> 
> 1. It doesn't respect the user's choice to hide the toolbar.

We do respect the user's choice. The toolbar is hidden by default (unless you import toolbar bookmarks when doing a new install). If you don't intend to use it and don't file bookmarks there, you'll never see it.

> 2. Users that choose to show the toolbar only when necessary will have to
> fight against it.

I don't believe this is a use-case we're designing for. We have other features that cover this - bookmarks in awesome bar searches and top sites on the new tab (which you can pin).


> 3. When the user saves a bookmark they are unlikely to be interested in
> other bookmarks at that time.
> 4. Simply rearranging bookmarks in the sidebar or library that belong to the
> "Bookmarks Toolbar" group is enough to autoshow the toolbar.

If someone is intentionally saving a bookmark in the bookmarks toolbar, I believe that's a good indication that they intend to use the toolbar. Not showing it in this case is confusing (this has been observed in user testing). And for the case where someone accidentally files a bookmark here, exposing the toolbar makes that obvious and thus easier to correct.


> 5. Extensions that save bookmarks to the "Bookmarks Toolbar" group will
> cause the toolbar to autoshow.

That's fine.

> 6. When the toolbar autoshows it moves everything down, if this happens
> unexpectedly the user can lose their place or perform an action on the wrong
> item.

I don't think this will be a big issue. When you are filing a bookmark you aren't looking at the content so you'll have to relocate your place in a document anyway.

> 7. There is no obvious/easy way to hide the toolbar (eg close button,
> context menu (Bug 1003364) or hotkey (Bug 403956)), only several indirect
> methods that are either obscure or require significant mousing (navbar
> context menu, menubar, bookmarks menu button or Customize). 

That's true though showing and hiding the bookmarks toolbar is not common task.

> 8. The only opt-out is to move "Bookmark Toolbar Items" out of the toolbar
> (which is non-obvious) or not save bookmarks to the toolbar at all which
> sacrifices its convenience.

I think that's fine. If you want to use the toolbar is should be visible. If you don't use (don't have bookmarks in it) it won't be visible. 

> 9. Chrome users are used to not having a bookmarks toolbar visible by
> default (they also have a context menu and hotkey to easily hide it) and
> there seems to be a widespread preference to minimize browser chrome as much
> as possible.
> 

Firefox's bookmarks toolbar is not visible by default. Most people will never see it. It's only when you explicitly file bookmarks there that you'll see it. This solves the problem of filing things there and then having to additionally figure out how to display the toolbar.


> I think it makes more sense to show the bookmarks toolbar for new tabs (Bug
> 1244649) as this is where it is most likely needed and doesn't interfere so
> much with user preference.

This doesn't make sense for the people who don't use the bookmarks toolbar.

Comment 22

[Dão Gottwald [:dao]]

<mak> dao: I think the points where we do new PlacesCreateBookmarkTransaction, both in browser-places.js and in bookmarkProperties.js.. and then there is the case where we move the bookmark to the toolbar (cause when you star it doesn't go there) http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/editBookmarkOverlay.js#739
<mak> dao: so effectively the only way to add to the toolbar is moving from the star panel (editBookmarkOverlay.js) and bookmarkProperties.js... probably there is no need to touch browser-places
<mak> bookmarkProperties is used when you right click on the toolbar )from the menu button) and choose new folder/bookmark and so on
<mak> I don't know if this is a case we care about, cause in this case the user may WANT to keep the toolbar hidden
<mak> so it's likely the only case to handle is editBookmarkOverlay.js moving a bookmark to the toolbar
<mak> alternatively, we could add a parentGuid to gEditItemOverlay, and when the panel closes you could check gEditItemOverlay.parentGuid

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch

the removeObserver call was broken

Comment 25

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b51d32ea41f

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8753355 [details] [diff] [review]
patch

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

It looks good as a first try to do this for UI actions, let's see users reaction.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/54ebf85e8731

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed for many failing mochitests on Linux: https://hg.mozilla.org/integration/fx-team/rev/b7756d054ba8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=54ebf85e87313800ab3b4412861a9d49cef7e2ff

Comment 29

[Dão Gottwald [:dao]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #28)
> Backed for many failing mochitests on Linux:
> https://hg.mozilla.org/integration/fx-team/rev/b7756d054ba8
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=fx-
> team&revision=54ebf85e87313800ab3b4412861a9d49cef7e2ff

That leak was caused by me moving BookmarkingUI.uninit() to the "right" place. I've now removed that from my patch.

https://hg.mozilla.org/integration/fx-team/rev/9caf438f4b91

Comment 30

[Dão Gottwald [:dao]]

... as happened before (comment 12), I just forgot about it.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9caf438f4b91

Comment 32

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Nightly 44.0a1 (2015-10-01) on Windows 7 x64. 

Verified fixed on Windows 7 x64, Ubuntu 14.04 x64 and Mac OSX 10.11.6 using Firefox 49 RC build 3 (buildID: 20160912134115).