Closed Bug 1354155 Opened 7 years ago Closed 7 years ago

Create Library button and corresponding panel

Categories

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

52 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- verified

People

(Reporter: mikedeboer, Assigned: Gijs)

References

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.

Attachments

(3 files, 1 obsolete file)

Blocks: 1354157
No longer blocks: 1354157
Hi Mike, want to confirm if this bug is a Meta?  Thanks.
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Keywords: meta
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Blocks: 1365421
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
Depends on: 1366026
No longer depends on: 1366026
Depends on: 1366026
Blocks: 1366026
No longer depends on: 1366026
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
User Story: (updated)
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. :-)
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-
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.
Iteration: 55.6 - May 29 → 55.7 - Jun 12
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)
Attachment #8872654 - Attachment is obsolete: true
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+
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 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+
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)
hg error in cmd: hg pull gecko -r df8f7b7a1b0bd6d53da55951c788327d8bb39bc1: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1369659
Depends on: 1370078
Depends on: 1370080
Depends on: 1370082
Depends on: 1370083
No longer depends on: 1370083
No longer depends on: 1369564
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)
(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
Flags: qe-verify+
Blocks: 1387512
Depends on: 1398349
Depends on: 1398346
Depends on: 1398667
No longer depends on: 1398667
No longer depends on: 1398346
No longer depends on: 1398349
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: