Create Library button and corresponding panel

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
5 months ago
20 days ago

People

(Reporter: mikedeboer, Assigned: Gijs)

Tracking

(Blocks: 3 bugs)

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

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure])

User Story

Morphing this bug. Because we're now keeping the bookmarks menu button, see bug 1365421 for moving that to the palette and replacing it with this button, we will want to write the button separate to the original bookmarks-menu-button. This needs to happen soon so the animation folks can test with this button.

While it would be easiest to create this as a CustomizableUI widget of type=view, I think using a XUL toolbarbutton (that we can hardcode into the navbar) will be better for perf.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (obsolete)
(Reporter)

Updated

5 months ago
Blocks: 1354157
(Assignee)

Updated

5 months ago
No longer blocks: 1354157

Comment 1

4 months ago
Hi Mike, want to confirm if this bug is a Meta?  Thanks.
Flags: needinfo?(mdeboer)
(Reporter)

Updated

4 months ago
Flags: needinfo?(mdeboer)
Keywords: meta

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
(Assignee)

Updated

3 months ago
Blocks: 1365421
(Assignee)

Comment 2

3 months ago
Morphing this, because we're now keeping the bookmarks menu button, see bug 1365421 for moving that to the palette and replacing it with this button.

While it would be easiest to create this as a CustomizableUI widget of type=view, I wonder if using a XUL toolbarbutton (that we can hardcode into the navbar) will be better for perf... we should make sure we've flipped the photon pref and thereby get nightly perf measurements (tpaint etc.) for this change when we land it, so that we don't surprise ourselves when we do end up flipping the pref...
Summary: Show Library panel when the bookmarks dropdown button is clicked → Create Library button and corresponding panel

Updated

3 months ago
Depends on: 1366026

Updated

3 months ago
No longer depends on: 1366026

Updated

3 months ago
Depends on: 1366026
(Assignee)

Updated

3 months ago
Blocks: 1366026
No longer depends on: 1366026

Updated

3 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
(Assignee)

Updated

3 months ago
User Story: (updated)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
These patches create the button but don't add it to the toolbar by default.

I noticed some issues:
- In this case, when we show the library subview inside the main panel we want a header (with back button), but we don't when displaying it as the main view in a separate panel for the library button. As a result, the code needs to deal with showing/hiding the header based on whether we're the mainview. I updated the CSS, but as noted in bug 1364738, we don't correctly set the [mainview] attribute. This messes with styling and with some logic about whether we can go back etc. If you pull (rather than import) these patches, the series will contain a mini-patch extracted from Mike's changes in bug 1364738 which fixes this.
- I noticed the same flashing as in bug 1367970. I haven't attempted to fix that, it seems like a binding issue and at this point I think it makes sense to get this reviewed and landed to unblock the animations team and to get us going on this panel.
- I haven't updated the styling or changed the history view to put the 'restore tab/window' bits into further subviews. We have separate bugs for the history and synced tabs views, and I think the styling updates can happen in there. This is a bit of a halfway house between "empty panel with placeholder text" and "fully functional library panel", but there we are. I think the state is non-terrible enough that it can land without being too offensive.

Note: this soft-conflicts (due to changes in paths) with bug 1367432, so once that lands I need to remember to update the path to the library icon. :-)
(Reporter)

Comment 6

3 months ago
mozreview-review
Comment on attachment 8871813 [details]
Bug 1354155 - use photon panelmultiview for individual subviews,

https://reviewboard.mozilla.org/r/143276/#review147582

Looking good so far and I'm quite happy that & how you solved the destructor/ view caching problem!

I'd like to take a look at the next revision when it's ready.

::: browser/components/customizableui/PanelMultiView.jsm:187
(Diff revision 1)
>        this.node.removeAttribute("transitioning");
>      }
>    }
>  
> +  get _panelViewCache() {
> +    return this.document.getElementById(this.node.getAttribute("viewcacheId"));

Since this attribute may not be set, can you prevent doing a `getElementById(undefined)`? You can coalesce this with other changes I propose below...

::: browser/components/customizableui/PanelMultiView.jsm:288
(Diff revision 1)
>    }
>  
>    destructor() {
>      if (this._mainView) {
> -      this._mainView.removeAttribute("mainview");
> +      let mainView = this._mainView;
> +      if (this._panelViewCache) {

You're doing `getElementById` 6 times here... can optimize that a wee bit? Like, caching the element in the getter, perhaps, and unsetting it in the latter part of the destructor?

::: browser/components/customizableui/PanelMultiView.jsm:290
(Diff revision 1)
>    destructor() {
>      if (this._mainView) {
> -      this._mainView.removeAttribute("mainview");
> +      let mainView = this._mainView;
> +      if (this._panelViewCache) {
> +        this._panelViewCache.appendChild(mainView);
> +      }

nit: superfluous braces.

::: browser/components/customizableui/PanelMultiView.jsm:298
(Diff revision 1)
> +    if (this._subViews && this._panelViewCache) {
> +      for (let subview of this._subViews.childNodes) {
> +        // Hi, XBL. You're annoying.
> +        if (subview.nodeName != "children") {
> +          this._panelViewCache.appendChild(subview);
> +        }

nit: superfluous braces.

::: browser/components/customizableui/PanelMultiView.jsm:306
(Diff revision 1)
>      if (this.panelViews) {
> +      if (this._panelViewCache) {
> +        for (let subview of this._viewStack.childNodes) {
> +          if (subview.nodeName != "children") {
> +            this._panelViewCache.appendChild(subview);
> +          }

nit: superfluous braces.

::: browser/components/customizableui/PanelMultiView.jsm:335
(Diff revision 1)
>     *
>     * @param  {panelview} view View to check, defaults to the currently active view.
>     * @return {Boolean}
>     */
>    _canGoBack(view = this._currentSubView) {
> -    return !!view.getAttribute("title");
> +    return view == this._mainView;

ITYM `view != this._mainView`? You also need to update the comment above this method when you change it.

::: browser/components/customizableui/content/panelUI.inc.xul:502
(Diff revision 1)
>         hidden="true"
>         flip="slide"
>         position="bottomcenter topright"
>         noautofocus="true">
> -  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView">
> +  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView"
> +                        viewcacheId="appMenu-viewCache">

nit: Even though I'm against camelCase for (X/HT)ML attributes, can you make this 'viewCacheId'?
Attachment #8871813 - Flags: review?(mdeboer) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
I think I addressed all the comments. I also realized there was a loop-iteration-and-removal issue, ie this:

for (let x of y.childNodes) {
  z.appendChild(x);
}

will only append every other x. Would be nice if eslint caught those... I'll file a bug for that. In the meantime, fixed by cloning y.childNodes into a separate array so it doesn't get modified by the removals. Then I stuck that in a helper because otherwise I'd have to duplicate it for the _subViews and _panelViews cases.

Updated

3 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(Reporter)

Comment 10

3 months ago
mozreview-review
Comment on attachment 8871813 [details]
Bug 1354155 - use photon panelmultiview for individual subviews,

https://reviewboard.mozilla.org/r/143276/#review147844

This somehow seems to break the Pocket panel, which'll use the same mechanism. (Try opening it more than once.)

This may need to be solved in follow-ups, but can you investigate what's going on? Thanks!
Attachment #8871813 - Flags: review?(mdeboer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8872654 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 18

3 months ago
mozreview-review
Comment on attachment 8871813 [details]
Bug 1354155 - use photon panelmultiview for individual subviews,

https://reviewboard.mozilla.org/r/143276/#review148298

LGTM! Nice work :)

::: browser/components/customizableui/PanelMultiView.jsm:322
(Diff revision 4)
>      this._panel.removeEventListener("popuphidden", this);
>      this.node = this._clickCapturer = this._viewContainer = this._mainViewContainer =
> -      this._subViews = this._viewStack = this.__dwu = null;
> +      this._subViews = this._viewStack = this.__dwu = this._panelViewCache = null;
> +  }
> +
> +  _moveOutKids(viewNodeContainer) {

Please add a JSDoc comment for this method.
Attachment #8871813 - Flags: review?(mdeboer) → review+
(Reporter)

Comment 19

3 months ago
mozreview-review
Comment on attachment 8872655 [details]
Bug 1354155 - fix pocket button's checks for whether it is the mainview in a panelmultiview,

https://reviewboard.mozilla.org/r/144194/#review148302
Attachment #8872655 - Flags: review?(mdeboer) → review+

Comment 20

3 months ago
mozreview-review
Comment on attachment 8871814 [details]
Bug 1354155 - create library button with initial history and synced tabs views,

https://reviewboard.mozilla.org/r/143278/#review148432
Attachment #8871814 - Flags: review?(bgrinstead) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

3 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e4b71135b63c -d 1941e3304049: rebasing 399252:e4b71135b63c "Bug 1354155 - use photon panelmultiview for individual subviews, r=mikedeboer"
merging browser/components/customizableui/content/panelUI.inc.xul
warning: conflicts while merging browser/components/customizableui/content/panelUI.inc.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

3 months ago
hg error in cmd: hg pull gecko -r df8f7b7a1b0bd6d53da55951c788327d8bb39bc1: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error

Comment 29

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/27e39a0dead1
use photon panelmultiview for individual subviews, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/718fc2e88bb1
create library button with initial history and synced tabs views, r=bgrins
https://hg.mozilla.org/integration/autoland/rev/d8118603a0bd
fix pocket button's checks for whether it is the mainview in a panelmultiview, r=mikedeboer

Comment 30

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27e39a0dead1
https://hg.mozilla.org/mozilla-central/rev/718fc2e88bb1
https://hg.mozilla.org/mozilla-central/rev/d8118603a0bd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Updated

3 months ago
Depends on: 1369659

Updated

3 months ago
Depends on: 1370078

Updated

3 months ago
Depends on: 1370080

Updated

3 months ago
Depends on: 1370082

Updated

3 months ago
Depends on: 1370083
(Assignee)

Updated

3 months ago
No longer depends on: 1370083
Depends on: 1369564
(Assignee)

Updated

3 months ago
No longer depends on: 1369564
(Reporter)

Updated

3 months ago
Depends on: 1370580
When I move the Library button to the toolbar on Windows/Ubuntu and select an option, the animation flickers and is not smooth.  Is this related to the second bullet point mentioned in Comment 5? Asking for clarification.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 32

3 months ago
(In reply to Grover Wimberly IV [:Grover-QA] from comment #31)
> When I move the Library button to the toolbar on Windows/Ubuntu and select
> an option, the animation flickers and is not smooth.  Is this related to the
> second bullet point mentioned in Comment 5? Asking for clarification.

I think the animation issues are covered by bug 1370580.
Flags: needinfo?(gijskruitbosch+bugs)
In that case, verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
No longer depends on: 1370082
No longer depends on: 1370078
Depends on: 1369729
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.