Add a 'Bookmarks' button to the Library panel

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

52 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure], URL)

Attachments

(4 attachments)

Comment hidden (obsolete)
Flags: qe-verify-
Summary: Add a 'Bookmarks' button to the Library panel → [meta] Add a 'Bookmarks' button to the Library panel
(Assignee)

Updated

2 years ago
Keywords: meta
Summary: [meta] Add a 'Bookmarks' button to the Library panel → Add a 'Bookmarks' button to the Library panel
Flags: qe-verify-
(Assignee)

Updated

2 years ago
Blocks: 1354532

Updated

2 years ago
No longer blocks: 1354532

Updated

2 years ago
No longer depends on: 1354157
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Comment 1

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1372946
(Assignee)

Updated

2 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8879259 - Flags: review?(mak77)
Attachment #8879259 - Flags: review?(gijskruitbosch+bugs)
Attachment #8879259 - Flags: feedback?(mak77)
Attachment #8879259 - Flags: feedback?(gijskruitbosch+bugs)

Comment 3

2 years ago
mozreview-review
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?
Attachment #8879259 - Flags: review?(mak77)
(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

2 years ago
mozreview-review
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
Attachment #8879259 - Flags: review?(mak77)
Attachment #8879259 - Flags: feedback?(mak77) → feedback+

Comment 6

2 years ago
mozreview-review
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...

Updated

2 years ago
Attachment #8879259 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 7

2 years ago
(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 hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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. :-)
Attachment #8879259 - Flags: review?(gijskruitbosch+bugs)

Comment 10

2 years ago
mozreview-review-reply
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

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Tests are next on the list - working on 'em.
(Assignee)

Comment 14

2 years ago
Hmmm, I think I might have to cut the patches up in several parts as well.
(Assignee)

Updated

2 years ago
Attachment #8879259 - Flags: review?(mak77)
Attachment #8879259 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
mozreview-review
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.
(Assignee)

Comment 20

2 years ago
mozreview-review
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.
(Assignee)

Comment 21

2 years ago
mozreview-review
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.
(Assignee)

Comment 22

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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?
Attachment #8880292 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 25

2 years ago
mozreview-review
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.
Attachment #8880293 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 26

2 years ago
mozreview-review
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
Attachment #8880294 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 27

2 years ago
mozreview-review
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?
Attachment #8880292 - Flags: review+
(Assignee)

Comment 28

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 29

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

2 years ago
mozreview-review
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`.
Attachment #8880292 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 35

2 years ago
mozreview-review-reply
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.
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Comment 36

2 years ago
mozreview-review
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
Attachment #8879259 - Flags: review?(mak77) → review+

Comment 37

2 years ago
mozreview-review
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
Attachment #8880292 - Flags: review?(mak77)

Comment 38

2 years ago
mozreview-review
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?
Attachment #8880293 - Flags: review?(mak77) → review+

Comment 39

2 years ago
mozreview-review
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.
Attachment #8880294 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Blocks: 1370083

Comment 50

2 years ago
mozreview-review
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
Attachment #8880292 - Flags: review?(mak77) → review+

Comment 51

2 years ago
mozreview-review
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?
Attachment #8880294 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1377365

Comment 56

2 years ago
Pushed by mdeboer@mozilla.com:
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

Updated

2 years ago
Depends on: 1377967

Updated

2 years ago
No longer blocks: 1378016
Depends on: 1378016
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
QA Whiteboard: [bugday-20170712]

Comment 59

2 years ago
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

2 years ago
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Blocks: 1387512
Depends on: 1400669

Updated

2 months ago
Depends on: 1533339
You need to log in before you can comment on or make changes to this bug.