1372946 - Spike: research feasibility of Places menu using only panelviews

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

At the moment, the bookmarks panel menu displays the contents of its folders and alternative views in menupopups that do not really fit in.
With Photon we emphasize and make a lot more use of the panel subviews to display secondary UI. Perhaps this is also a good time to move the Places views over to be properly part of the Photon panels.

The drag 'n drop functionality is not part of this spike; if we can get the views to populate and work consistently, implementing drag 'n drop is a matter of doing the work and porting much of the places menupanel code over to PanelMultiView.jsm.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1372946 - Experiment with using only panelviews for Places.

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

Comment 2

[Mike de Boer [:mikedeboer]]

You can see it in action here: https://vimeo.com/221608472

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8877649 [details]
Bug 1372946 - Experiment with using only panelviews for Places.

As I said in the other bug, this is a good start.
I'm a bit missing info about the global direction, since this bug is investigating a full view, the other one just recent bookmarks. What are we targeting?

I didn't test the patch directly, things to check in new views are:
1. does it update properly when bookmarks change and the view is open? (this may want a test, for the old menu we have http://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_views_liveupdate.js that is a bit legacy but what matters is the scope of it)
2. is the contextual menu correct on the various areas of the view? For example on the "static" part of the menu (not the user bookmarks) it is different than on bookmarks. In "Recent Tags" it should not be possible to Cut (since queries are read-only), and so on. I can eventually do some manual testing when you're ready for that.
3. Does drag&drop work? Is it possible to reorder items, and is a drop indicator shown in the proper place? is it possible to drop into subfolders or out of subfolder? is it possible to drop outside the menu, and viceversa? Or maybe you decided D&D is polish that will happen after 57.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #3)
> I didn't test the patch directly, things to check in new views are:
> 1. does it update properly when bookmarks change and the view is open? (this
> may want a test, for the old menu we have
> http://searchfox.org/mozilla-central/source/browser/components/places/tests/
> browser/browser_views_liveupdate.js that is a bit legacy but what matters is
> the scope of it)

Didn't check this yet, but I think it works out-of-the-box. I'll add a test for sure when we move to a concrete implementation.

> 2. is the contextual menu correct on the various areas of the view? For
> example on the "static" part of the menu (not the user bookmarks) it is
> different than on bookmarks. In "Recent Tags" it should not be possible to
> Cut (since queries are read-only), and so on. I can eventually do some
> manual testing when you're ready for that.

I think I skipped setting the correct attribute on the nodes atm, but that will be something to ensure later.

> 3. Does drag&drop work? Is it possible to reorder items, and is a drop
> indicator shown in the proper place? is it possible to drop into subfolders
> or out of subfolder? is it possible to drop outside the menu, and viceversa?
> Or maybe you decided D&D is polish that will happen after 57.

No, because I descoped that feature for this spike. I can't see us _not_ having DnD support, so I'll need to (re-)implement that for sure.

Thanks for the feedback!! This'll be super useful once we start moving this and other panels/ views to the sliding views system.

Comment 5

[Mike de Boer [:mikedeboer]]

Spiked has finished (successfully) and part of the code is used for the Bookmarks subview in the Library panel. Thanks again, Marco!