1354159 - Add a 'Bookmarks' button to the Library panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Mike de Boer [:mikedeboer]]

This button opens a subview when clicked, which contains the same items as the current 'Bookmarks' dropdown panel.

The title of the subview reads 'Bookmarks', the same as the button label.

Comment 1

[:Gijs (he/him)]

This submenu has now been updated, and only contains some toplevel items that open various other bits of UI, and recent bookmarks. It doesn't contain all bookmarks.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

This patch updates the structure of the current Bookmarks panel and initializes
it with a list of Recent Bookmarks below a number of static toolbar buttons.
To break free from legacy .bookmark-item styles as used inside menus, I introduce
the .subview-bookmark-item class-name.
This patch also addresses the panel footer styling to match the Photon spec,
introduces a 'subview-header' class that may be used to style sub-headings inside
a list of toolbarbuttons inside a panel and a new 'Search Bookmarks' item.

Review commit: https://reviewboard.mozilla.org/r/150548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150548/

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review155542

off-hand it looks ok.

There's a more general discussion probably regarding the features these views should have. I'd expect this to work as a current Places menupopup view, thus supporting D&D, a full contextual menu and so on. I know that originally we couldn't do that for recently bookmarked because we only support one places view per panel, and the menu panel already contained the bookmarks menu view.
When the menus will be split, we could have a full-fledged view here, and we could stop having a bookmarks observer and all the duplicated stuff, it would just be a view like the bookmarks menu, just with a different places: query.

::: browser/base/content/browser-places.js:1916
(Diff revision 1)
>      for (let i = 0, l = staticButtons.length; i < l; ++i)
>        CustomizableUI.addShortcut(staticButtons[i]);
>      // Setup the Places view.
> +    if (gPhotonStructure) {
> +      this._initRecentBookmarks(document.getElementById("panelMenu_recentBookmarks"),
> +        "subviewbutton", 42);

what's this magic number?
Isn't this too many for the avg screen and bigger density? I see 10 recent bookmarks in the mock-up. Maybe you meant 12 instead of 42? Can we make this settable through a pref?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #3)
> When the menus will be split, we could have a full-fledged view here, and we
> could stop having a bookmarks observer and all the duplicated stuff, it
> would just be a view like the bookmarks menu, just with a different places:
> query.

Though, this remembered me we'll always have the native legacy menu, that won't be able to use separate views.
So either we make this view a completely separate one (but at this point it's likely not worth it) or we keep having this half-duplicate effort, that is required for the legacy menu. The best solution would be to have multiple places views per element, but that sounds like something it may never happen based on available resources.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review155548

Comment 6

[:Gijs (he/him)]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review155560

This looks pretty OK to me, besides Marco's comments, here are a few minor ones from me... note that I haven't tested this yet.

::: browser/base/content/browser-places.js:1558
(Diff revision 1)
> -      item.setAttribute("class", "menuitem-iconic menuitem-with-favicon bookmark-item " +
> -                                 aExtraCSSClass);
> +      item.classList.add(...(inPhotonPanel ? ["subviewbutton", "subviewbutton-iconic",
> +        "subview-bookmark-item"] : ["menuitem-iconic", "menuitem-with-favicon",
> +        "bookmark-item"]));

Nit: this would be more readable when split out. Something like:

```js
let classes;
if (inPhotonPanel) {
  classes = ["subviewbutton", "subviewbutton-iconic", "subview-bookmark-item"];
} else {
  classes = ["menuitem-iconic", "menuitem-with-favicon", "bookmark-item"];
}
if (aExtraCSSClass)
  classes.push(aExtraCSSClass);

item.classList.add(...classes);
```

::: browser/components/customizableui/content/panelUI.inc.xul:271
(Diff revision 1)
> +#ifndef MOZ_PHOTON_THEME
>            <!-- bookmarks menu items will go here -->
> +#else
> +          <label id="panelMenu_recentBookmarks"
> +                 value="&recentBookmarks.label;"
> +                 class="subview-header"/>

I would confuse this with panel-subview-header, which is another class we use. Can we call this 'subview-subheader' or something? :-)

::: browser/components/places/PlacesUIUtils.jsm:1018
(Diff revision 1)
>     * @param   aView
>     *          The controller associated with aNode.
>     */
>    openNodeWithEvent:
>    function PUIU_openNodeWithEvent(aNode, aEvent, aView) {
> -    let window = aView.ownerWindow;
> +    let window = aView ? aView.ownerWindow : aEvent.target.ownerGlobal;

Is there a reason not to just always use aEvent.target.ownerGlobal? Seems sensible to me...

::: browser/themes/shared/customizableui/panelUI.inc.css:1257
(Diff revision 1)
>  
>  photonpanelmultiview .subviewbutton:focus {
>    outline: 0;
>  }
>  
> -photonpanelmultiview .subviewbutton:not(.subviewbutton-back) > .toolbarbutton-text {
> +photonpanelmultiview .subviewbutton:not(.subviewbutton-back):not(.panel-subview-footer) > .toolbarbutton-text {

I think I bitrotted you here...

::: browser/themes/shared/customizableui/panelUI.inc.css:1261
(Diff revision 1)
>  
> -photonpanelmultiview .subviewbutton:not(.subviewbutton-back) > .toolbarbutton-text {
> +photonpanelmultiview .subviewbutton:not(.subviewbutton-back):not(.panel-subview-footer) > .toolbarbutton-text {
>    padding-inline-start: 24px; /* This is 16px for the icon + 8px for the padding as defined above. */
>  }
>  
> -photonpanelmultiview .subviewbutton-iconic:not(.subviewbutton-back) > .toolbarbutton-text,
> +photonpanelmultiview .subviewbutton-iconic:not(.subviewbutton-back):not(.panel-subview-footer) > .toolbarbutton-text,

... but I also think I forgot to remove :not(.subviewbutton-back) from this selector, which we might as well do now?

::: browser/themes/shared/menupanel.inc.css:277
(Diff revision 1)
> +#panelMenuBookmarkThisPage {
> +  list-style-image: url("chrome://browser/skin/bookmark-hollow.svg");

I think we need an extra rule to make this use the filled-in bookmark for bookmarked pages?

::: browser/themes/shared/toolbarbuttons.inc.css:471
(Diff revision 1)
>  /* Force the display of the label for bookmarks */
> -.bookmark-item > .toolbarbutton-text,
> +.bookmark-item > .toolbarbutton-icon,
> +.subview-bookmark-item > .toolbarbutton-icon,
>  #personal-bookmarks[cui-areatype="toolbar"] > #bookmarks-toolbar-placeholder > .toolbarbutton-text {

This change looks wrong, and certainly no longer matches the comment...

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #3)
> what's this magic number?
> Isn't this too many for the avg screen and bigger density? I see 10 recent
> bookmarks in the mock-up. Maybe you meant 12 instead of 42? Can we make this
> settable through a pref?

It's something Bryan and I discussed off-bug; the idea is that this should be a scrollable list, not too many and not too few items.
Bryan was thinking about 50 items, I added a whimsy factor and 42 is the result. I'll push it into a pref, but how useful is that, really? Who do you see changing this value? Wouldn't it be something driven by UX and updated per release?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/1-2/

Comment 9

[:Gijs (he/him)]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review156126

At least on OS X, I get no context menu at all for the recently bookmarked items, which seems worse than what we do in the bookmarks menu button. Dragging the items in the menu also doesn't seem to work.

::: browser/themes/shared/customizableui/panelUI.inc.css:1244
(Diff revision 2)
> -photonpanelmultiview .PanelUI-subView .subviewbutton:not(.panel-subview-footer) {
> +photonpanelmultiview .PanelUI-subView .subviewbutton,
> +photonpanelmultiview .subview-subheader {

At least on OS X, the margins in this rule are now overridden by https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1237-1240 which results in items in the library panel having margins again (and hover style not going to the edge of the panel).

::: browser/themes/shared/customizableui/panelUI.inc.css:1347
(Diff revision 2)
> +photonpanelmultiview .subview-subheader {
> +  color: GrayText;
> +}
> +
> +photonpanelmultiview .subview-subheader,
> +photonpanelmultiview .panel-subview-footer {

There's still a margin under the footer on OS X, that looks like it's about 2px. This matches up with the issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1370083 where someone's pointed out that we indiscriminately apply 6px top+bottom padding, and the footer gets a -4px bottom margin right now because of https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1380-1385 .

I think the 4px number is because of the default padding on panel-arrowcontent, which we override to 0 for photon panels.

Of course, the negative-margin-based repositioning of the footer we do now means there's 4px (and, if we fix this by simply repositioning the footer further down, then 6px) space between the footer and the scrollable area of bookmarks/history. That doesn't seem desirable.

We already override the top padding for views with a header (ie all subviews).

I *think* the correct solution is to give the panel-subview-body 6px margins instead (and to 0 out the negative margin on the footer in photon panels). It seems like this would mean:
- add-on panels are no longer affected
- we can remove:
- - the margin bottom on panel headers
- - padding on the panelviews
- - padding override on panelviews with headers


so I *think* that that works. Do check whether I'm right. :-)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review155560

> I think we need an extra rule to make this use the filled-in bookmark for bookmarked pages?

This looks like it's still an issue. To be clear, this is only for the 'edit this bookmark' entry in the menu, not for the 'bookmarks' entry in the library menu/panel.

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs from comment #9)
> Of course, the negative-margin-based repositioning of the footer we do now
> means there's 4px (and, if we fix this by simply repositioning the footer
> further down, then 6px) space between the footer and the scrollable area of
> bookmarks/history. That doesn't seem desirable.

Err, I meant to delete this - this is what we do in the pre-photon panels as well, so I guess we should keep it that way.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/2-3/

Comment 13

[Mike de Boer [:mikedeboer]]

Tests are next on the list - working on 'em.

Comment 14

[Mike de Boer [:mikedeboer]]

Hmmm, I think I might have to cut the patches up in several parts as well.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review commit: https://reviewboard.mozilla.org/r/151654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151654/

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

This patch changes the Recent Bookmarks flow in browser-places.js to use the new
PlacesPanelview to provide a rich view that support drag and drop and context
actions.
A new button 'Search Bookmarks' was added, which prefills the urlbar with '* ',
which is a shortcut for searching bookmarks using the Awesomebar.

Review commit: https://reviewboard.mozilla.org/r/151656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151656/

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review commit: https://reviewboard.mozilla.org/r/151658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151658/

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/3-4/

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review156624

::: browser/base/content/browser-places.js:1542
(Diff revision 4)
>        item.setAttribute("label", title || uri);
>        item.setAttribute("targetURI", uri);
>        item.setAttribute("simulated-places-node", true);
>        item.setAttribute("class", "menuitem-iconic menuitem-with-favicon bookmark-item " +
>                                   aExtraCSSClass);
> +

nit: will remove this newline addition.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review156626

::: browser/components/places/content/browserPlacesViews.js:22
(Diff revision 1)
> +    this._rootElt = aOptions.rootElt;
> +  if ("viewElt" in aOptions)
> +    this._viewElt = aOptions.viewElt;
>    this.options = aOptions;
>    this._controller = new PlacesController(this);
> +  this.place = aPlace;

A propos: this fixes bug 912433!

::: browser/components/places/content/browserPlacesViews.js:1976
(Diff revision 1)
> +class PlacesPanelview extends PlacesViewBase {
> +  constructor(container, panelview, place, options = {}) {
> +    options.rootElt = container;
> +    options.viewElt = panelview;
> +    super(place, options);
> +    this._viewElt._placesView = this;// this.panelMultiView._placesView = this;

nit: whoops, left over.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

https://reviewboard.mozilla.org/r/151656/#review156630

::: browser/base/content/browser-places.js:1837
(Diff revision 1)
>        this.dropmarkerNotifier.style.transform = "";
>        this.notifier.style.transform = "";
>      }, 1000);
>    },
>  
> -  _showSubview() {
> +  showSubView(anchor) {

nit: didn't notice the casing update here before, will change.

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

https://reviewboard.mozilla.org/r/151658/#review156636

::: browser/themes/shared/customizableui/panelUI.inc.css:359
(Diff revision 1)
>  #customizationui-widget-multiview panelview:not([extension]) {
>    min-width: @menuPanelWidth@;
>  }
>  
> -photonpanelmultiview panelview[title] {
> -  padding-top: 0;
> +photonpanelmultiview .panel-subview-body {
> +  margin: 4px 0;

I chose 4 pixels margin, not 6, because it just looks so much better and in balance IRL. (Prolly subjective, please feel free to fiddle with it and see for yourself ;) )

::: browser/themes/shared/customizableui/panelUI.inc.css:2112
(Diff revision 1)
>    overflow-x: visible;
>    overflow-y: visible;
>  }
>  
> +photonpanelmultiview #panelMenu_pocket {
> +  display: none;

To update the addon or not to update the addon? Eventually we will have to, of course. I was thinking another bug, though.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

https://reviewboard.mozilla.org/r/151658/#review156640

::: browser/themes/shared/customizableui/panelUI.inc.css:359
(Diff revision 1)
>  #customizationui-widget-multiview panelview:not([extension]) {
>    min-width: @menuPanelWidth@;
>  }
>  
> -photonpanelmultiview panelview[title] {
> -  padding-top: 0;
> +photonpanelmultiview .panel-subview-body {
> +  margin: 4px 0;

wfm but please get confirmation from ux. Probably easiest if you give them screenshots to compare.

::: browser/themes/shared/customizableui/panelUI.inc.css:2112
(Diff revision 1)
>    overflow-x: visible;
>    overflow-y: visible;
>  }
>  
> +photonpanelmultiview #panelMenu_pocket {
> +  display: none;

Yeah, I think leaving this is fine. Would even be fine with just shipping this like this in 57.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review156634

This is mostly a skim-over review with some minor comments, I trust Marco to review this more properly.

::: browser/components/places/content/browserPlacesViews.js:16
(Diff revision 1)
> -  this.place = aPlace;
> +  if ("rootElt" in aOptions)
> +    this._rootElt = aOptions.rootElt;
> +  if ("viewElt" in aOptions)
> +    this._viewElt = aOptions.viewElt;

The other extensions of this class seem to set these properties in their own constructors before calling this one. Is there a reason not do to the same?

::: browser/components/places/content/browserPlacesViews.js:240
(Diff revision 1)
>    destroyContextMenu: function PVB_destroyContextMenu(aPopup) {
>      this._contextMenuShown = null;
>    },
>  
>    _cleanPopup: function PVB_cleanPopup(aPopup, aDelay) {
> +    this._ensureMarkers(aPopup);

I don't really understand this addition, and the code in `_ensureMarkers` seems hardcoded to use a menuseparator, which would be wrong for our panels, right? Can you clarify?

::: browser/components/places/content/browserPlacesViews.js:724
(Diff revision 1)
>              this._getDOMNodeForPlacesNode(child).removeAttribute("visited");
>          }
>        }, Components.utils.reportError);
>    },
>  
> +  _isPopupOpen(elt) {

Please add a comment about why this is abstracted into a method (because we override it).

::: browser/components/places/content/browserPlacesViews.js:2069
(Diff revision 1)
> +      } catch (ex) {
> +        return;
> +      }
> +      this.panelMultiView.showSubView(panelview, button);
> +    } else {
> +      BookmarksEventHandler.onCommand(event, this);

Is this the right thing for any places view?

::: browser/components/places/content/browserPlacesViews.js:2138
(Diff revision 1)
> +              if (AppConstants.platform === "macosx") {
> +                // OS X native menubar doesn't track list-style-images since
> +                // it doesn't have a frame (bug 733415).  Thus enforce updating.
> +                element.setAttribute("image", "");
> +                element.removeAttribute("image");
> +              }

This doesn't look relevant here - copy-paste mistake?

Comment 25

[:Gijs (he/him)]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

https://reviewboard.mozilla.org/r/151656/#review156650

::: browser/base/content/browser-places.js:1911
(Diff revision 1)
> +        document.getElementById("panelMenu_bookmarksMenu"), panelview,
> +        "place:queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS +
> +        "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING +

Nit: can we put the query in a tmp var to make it a bit more readable?

::: browser/base/content/browser-places.js:1914
(Diff revision 1)
> +    if (gPhotonStructure) {
> +      this._panelMenuView = new PlacesPanelview(
> +        document.getElementById("panelMenu_bookmarksMenu"), panelview,
> +        "place:queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS +
> +        "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING +
> +        "&maxResults=42&excludeQueries=1");

Does this exclude folders and livemarks or not? If not, please get confirmation from UX that having them appear as sub-subviews is OK.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

https://reviewboard.mozilla.org/r/151658/#review156666

Comment 27

[:Gijs (he/him)]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review156668

If I:

- open nightly with the library button already in the toolbar
- open the bookmarks view
- right click a bookmark, or try to drag it

all the items in the context menu are greyed out, and dragging it does nothing. If I then close it and open it a second time, things start working. Seems like there must be a race condition of sorts somewhere?

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review156634

> The other extensions of this class seem to set these properties in their own constructors before calling this one. Is there a reason not do to the same?

Yes, this is because the `this.place = ...` below there expects them to be set and this is not supported by class constructors, even though they're inheriting from a proto like this. So it's either this or structure things like the others (using __proto__ and friends).

> I don't really understand this addition, and the code in `_ensureMarkers` seems hardcoded to use a menuseparator, which would be wrong for our panels, right? Can you clarify?

The markers are necessary for `_cleanPopup` to work and also others. Since they're hidden anyways, I tought using a menuseparator would be just fine. I could also introduce an option, like `markerTagName`, but that wouldn't change something functionally.

> Is this the right thing for any places view?

It's the right things for any _panelview_ places view, yes.

Comment 29

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

https://reviewboard.mozilla.org/r/151656/#review156650

> Does this exclude folders and livemarks or not? If not, please get confirmation from UX that having them appear as sub-subviews is OK.

This excludes folders and livemarks. It showed them before while I was working on this and compared the other 'Recent Bookmarks' query below this code AND asked Marco on IRC, both yielded the same answer :) ; I forgot the 'excludeQueries=1' clause.

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/4-5/

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151654/diff/1-2/

Comment 32

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151656/diff/1-2/

Comment 33

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151658/diff/1-2/

Comment 34

[:Gijs (he/him)]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review157218

::: browser/components/places/content/browserPlacesViews.js:2238
(Diff revisions 1 - 2)
> +      return;
> +
> +    // Because PanelMultiView reparents the panelview internally, the controller
> +    // may get lost. In that case we'll append it again, because we certainly
> +    // need it later!
> +    if (!this.controllers.getControllerCount() && this._controller)

Huh, can you file a followup to update the bookmarks menu button popup to do something similar, if this is sufficient? It could get rid of the customizableui listener (see BookmarkingUI) and simplify a lot of code.

::: browser/components/places/content/browserPlacesViews.js:2007
(Diff revision 2)
> +  }
> +
> +  get panelViewCache() {
> +    if (this._panelViewCache)
> +      return this._panelViewCache;
> +    return this._panelViewCache = document.getElementById("appMenu-viewCache");

There's an attribute for this, we should use that or use `this._viewElt.panelMultiView._panelViewCache`.

Comment 35

[:Gijs (he/him)]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review156634

> It's the right things for any _panelview_ places view, yes.

Yes, I kind of meant that we might want it to do something else for the downloads view, but I guess we can change it when we get to that.

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

https://reviewboard.mozilla.org/r/150548/#review157820

Comment 37

[Marco Bonardo [:mak]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review157976

::: browser/components/places/content/browserPlacesViews.js:240
(Diff revision 2)
>    destroyContextMenu: function PVB_destroyContextMenu(aPopup) {
>      this._contextMenuShown = null;
>    },
>  
>    _cleanPopup: function PVB_cleanPopup(aPopup, aDelay) {
> +    this._ensureMarkers(aPopup);

This should alreay happen at the fist popupshowing, that is also the first time we populate the popup.
Could you please explain why now we need to do this here?

::: browser/components/places/content/browserPlacesViews.js:1984
(Diff revision 2)
> +  constructor(container, panelview, place, options = {}) {
> +    options.rootElt = container;
> +    options.viewElt = panelview;
> +    super(place, options);
> +    this._viewElt._placesView = this;
> +    this._onPopupShowing({ originalTarget: this._viewElt });

please add a comment about this simulated popupshowing event and the expected flow.

::: browser/components/places/content/browserPlacesViews.js:2065
(Diff revision 2)
> +      }
> +      panelview.setAttribute("place", place);
> +
> +      this.panelViewCache.appendChild(panelview);
> +      this.panelMultiView.showSubView(panelview, button);
> +      BookmarkingUI._initPopup(panelview);

This code is missing, looks like the view implementation here is already trying to handle subiews, that currently we can't have since we only show recent bookmarks, so this code is basically never invoked and we don't hit this mistake.

Regardless, from here we should likely not call into BookmarkingUI that is a browser helper object with completely different reasons to exist. IF there's code reuse it should probably be moved elsewhere (PlacesUIUtils maybe?)

I'd suggest for now to just land a simplified view only handling the root, and we can add subviews support in the future when we actually can have subviews. Otherwise, this should at least be tested with the complete menu view before landing.

::: browser/components/places/content/browserPlacesViews.js:2110
(Diff revision 2)
> +  }
> +
> +  uninit(event) {
> +    this._removeEventListeners(this.panelMultiView, this.events);
> +    this._removeEventListeners(window, ["unload"]);
> +    super.uninit(event);

what about the ViewShowing event listener? Should we remove it as well?

::: browser/components/places/content/browserPlacesViews.js:2113
(Diff revision 2)
> +    this._removeEventListeners(this.panelMultiView, this.events);
> +    this._removeEventListeners(window, ["unload"]);
> +    super.uninit(event);
> +  }
> +
> +  _createMenuItemForPlacesNode(placesNode) {

maybe we should change the name, since it's not creating "menuitem"s

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

https://reviewboard.mozilla.org/r/151656/#review158026

::: browser/base/content/browser-places.js:714
(Diff revision 2)
> +      gURLBar.inputField.dispatchEvent(new KeyboardEvent("keypress", {
> +        keyCode: code,
> +        charCode: code,
> +        bubbles: true
> +      }));
> +    }

Is this by design?
Potentially we could even provide search inline in the panel itself (the result supports a searchTerm proeprty that sort-of works like the awesomebar, so it would just be matter of changing the view location).
On the other side I see the benefit of showing to the user that he can search for bookmarks in the location bar, so that'd still be great.
I was just curious.

::: browser/base/content/browser-places.js:1911
(Diff revision 2)
>        CustomizableUI.addShortcut(staticButtons[i]);
>      // Setup the Places view.
> +    if (gPhotonStructure) {
> +      let query = "place:queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS +
> +        "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING +
> +        "&maxResults=42&excludeQueries=1";

I'm still not sure I understand the maxResults=42?

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

https://reviewboard.mozilla.org/r/151658/#review158052

::: browser/base/content/browser.css:703
(Diff revision 2)
>  .unified-nav-current {
>    font-weight: bold;
>  }
>  
> -.bookmark-item > label {
> +.bookmark-item > label,
> +.subview-bookmark-item > label {

are there many bookmark-item rules that don't apply to the subview? I'm a little bit wary of adding another class and I'd prefer to just keep the existing .bookmark-item (maybe with a stricter selector), but if the differences are substancial, it will be fine. It jsut seems like we are adding a bunch of duplication.

::: browser/themes/linux/browser.css:197
(Diff revision 2)
> -.bookmark-item[container][livemark] {
> +.bookmark-item[container][livemark],
> +.subview-bookmark-item[container][livemark] {
>    list-style-image: url("chrome://browser/skin/feeds/feedIcon16.png");
>  }
>  
>  .bookmark-item[container][livemark] .bookmark-item {

why would this not be needed?
I see that for now we won't show livemarks here, but looks like when we'll do their children won't get an icon.

Comment 40

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/5-6/

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151654/diff/2-3/

Comment 42

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151656/diff/2-3/

Comment 43

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151658/diff/2-3/

Comment 44

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151654/diff/3-4/

Comment 45

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151656/diff/3-4/

Comment 46

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151658/diff/3-4/

Comment 47

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151654/diff/4-5/

Comment 48

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151656/diff/4-5/

Comment 49

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151658/diff/4-5/

Comment 50

[Marco Bonardo [:mak]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

https://reviewboard.mozilla.org/r/151654/#review158604

Comment 51

[Marco Bonardo [:mak]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

https://reviewboard.mozilla.org/r/151658/#review158606

::: browser/themes/shared/toolbarbuttons.inc.css:480
(Diff revision 5)
>  
> +%ifndef MOZ_PHOTON_THEME
>  .bookmark-item > .toolbarbutton-icon[label]:not([label=""]) {
>    margin-inline-end: 5px;
>  }
> -
> +%endif

This looks strange, afaict the scope of this line is to avoid icons in the toolbar to be too close. Maybe you actually want to make the rule more specific so that it only applied to toolbarbuttons on the toolbar, rather than disabling the rule in photon?

Comment 52

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879259 [details]
Bug 1354159 - Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150548/diff/6-7/

Comment 53

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880292 [details]
Bug 1354159 - Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151654/diff/5-6/

Comment 54

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880293 [details]
Bug 1354159 - Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151656/diff/5-6/

Comment 55

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880294 [details]
Bug 1354159 - Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151658/diff/5-6/

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4f311094669
Part 1 - Remove the unneeded third 'view' argument from PlacesUIUtils.openNodeWithEvent(). r=mak
https://hg.mozilla.org/integration/autoland/rev/17a5f1a17cbd
Part 2 - Introduce a new Places view type, PlacesPanelview, which can visualize query results inside panelview nodes. r=Gijs,mak
https://hg.mozilla.org/integration/autoland/rev/28e8a3011a4c
Part 3 - Add a Bookmarks button to the Library panel that shows a subview with a list of most recent bookmarks. r=Gijs,mak
https://hg.mozilla.org/integration/autoland/rev/96d2fe764ef8
Part 4 - Implement the style changes necessary to properly view the new Bookmarks view inside the Library Panel. r=Gijs,mak

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4f311094669
https://hg.mozilla.org/mozilla-central/rev/17a5f1a17cbd
https://hg.mozilla.org/mozilla-central/rev/28e8a3011a4c
https://hg.mozilla.org/mozilla-central/rev/96d2fe764ef8

Comment 58

[Mohammad Maruf Rahman]

This bug was about "adding  a 'Bookmarks' button to the Library panel" and I have seen the feature being implemented with latest Nightly on Ubuntu 16.04 LTS!

This bug's fix is now verified with latest Nightly!

Build ID 	20170717100212
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 59

[Tanvir Rahman]

I can see the feature implemented in Latest Nightly 56.0a1 on Windows 10, 64-bit

Build ID 	20170717030207
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170712]

Comment 60

[u554753]

Verified on Windows, Mac, and Ubuntu.