Closed
Bug 1354155
Opened 7 years ago
Closed 7 years ago
Create Library button and corresponding panel
Categories
(Firefox :: Toolbars and Customization, enhancement, P1)
Tracking
()
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)
Comment hidden (obsolete) |
Comment 1•7 years ago
|
||
Hi Mike, want to confirm if this bug is a Meta? Thanks.
Flags: needinfo?(mdeboer)
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Assignee | ||
Comment 2•7 years 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
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years 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•7 years 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•7 years 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•7 years ago
|
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Reporter | ||
Comment 10•7 years 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•7 years ago
|
Attachment #8872654 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 18•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 31•7 years ago
|
||
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•7 years 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)
Comment 33•7 years ago
|
||
In that case, verified on Windows, Mac, and Ubuntu.
You need to log in
before you can comment on or make changes to this bug.