873867 - Adjust panelmultiview binding to allow easy swapping of mainviews

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The specification for the bookmarks panel widget (bug 855805) calls for the following behaviour:

1) When the widget is in the main panel, clicking on the widget opens a subview that lists some number of options and bookmarks. Bookmark folders are also visible in this list, but clicking on them opens up that folder in the Library.
2) When the widget is in a toolbar, clicking on the widget opens a panel listing some number of options and bookmarks, but when clicking on a bookmark folder, a subview is opened that displays the contents of that folder. We only go one level deep, so clicking on a folder within the subview opens that folder in a Library.

The panel view that lists the bookmarks and options is common between these two locations, so we'll make use of jaws' patch that opens up subviews within their own panels.

We need, however, to have that panel support subviews, and have it recognize the bookmark panel as the main view. This means making it easy for subviews to become mainviews, and vice versa.

I suggest we drop the notion of panelmainview and panelsubview, and just have panelview tags within a panelmultiview. The mainViewId attribute of the panelmultiview will allow us to set the main view in mark-up, and the setMainView method of the binding will allow us to programmatically swap views in and out of the main view position.

Not entirely sure if this is the best way forward, but it seems to work for the WIP patch I'm putting together for bug 855805. Definitely open to suggestions on this though.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Here's what I'm proposing. Does this look like a reasonable approach, or can we do better?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 751498 [details] [diff] [review]
WIP Patch 1

Review of attachment 751498 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks like it'll work nicely.

::: browser/components/customizableui/content/panelUI.xml
@@ +92,5 @@
>        <property name="showingSubView" readonly="true"
>                  onget="return this._viewStack.getAttribute('view') == 'subview'"/>
> +      <property name="_mainViewId" onget="return this.getAttribute('mainViewId');" onset="this.setAttribute('mainViewId', val); return val;"/>
> +      <property name="_mainView" readonly="true"
> +                onget="return document.getElementById(this._mainViewId);"/>

Need to check this._mainViewId is non-empty/non-null.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 751498 [details] [diff] [review]
WIP Patch 1

>-  <panelmultiview id="PanelUI-multiView">
>-    <panelmainview id="PanelUI-mainView">
>+  <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView">

attribute names are usually lowercase

>+      <property name="_mainViewId" onget="return this.getAttribute('mainViewId');" onset="this.setAttribute('mainViewId', val); return val;"/>
>+      <property name="_mainView" readonly="true"
>+                onget="return document.getElementById(this._mainViewId);"/>

_mainView could just call getAttribute directly, unless you expect the _mainViewId getter to become more complex

>+        this._mainViewId = aNewMainView.id;

and this could just call setAttribute, unless you expect the _mainViewId setter to become more complex

>-#customizationui-widget-panel {
>+#customizationui-widget-panel .panel-mainview {

Should use the child selector here or set an attribute on .panel-mainview to indicate where it's located.

> #PanelUI-contents[type="list"] toolbarbutton > .toolbarbutton-text,
>-panelsubview toolbarbutton > .toolbarbutton-text,
>+.panel-subviews toolbarbutton > .toolbarbutton-text,
> #customizationui-widget-panel toolbarbutton > .toolbarbutton-text {

ditto

>-panelmainview toolbarbutton,
>-panelsubview toolbarbutton,
>+panelview toolbarbutton,
> #customizationui-widget-panel toolbarbutton,
> .customizationmode-button {

ditto

>-panelmainview toolbarbutton:not([disabled]):not([checked]):not([open]):not(:active):hover,
>-panelsubview toolbarbutton:not([disabled]):not([checked]):not([open]):not(:active):hover,
>+panelview toolbarbutton:not([disabled]):not([checked]):not([open]):not(:active):hover,
> #customizationui-widget-panel toolbarbutton:not([disabled]):not([checked]):not([open]):not(:active):hover,
> #PanelUI-mainView .PanelUI-pageControls toolbarbutton,
> .customizationmode-button,
> #PanelUI-mainView #PanelUI-customize-btn {

ditto

>-panelmainview toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
>-panelsubview toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
>+panelview toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
> .customizationmode-button:hover:active,
> #customizationui-widget-panel toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active) {

ditto

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

This patch made the Help menu items stop working. It looks like the click events were getting fired before the command events, and so we were closing the panel before the command event could be dispatched.

Now we don't listen to click events in the help panel - we listen for commands to bubble up.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

So this is annoying - it looks like if I inject the mainview into the anonymous content, the vbox above the menu panel's footer doesn't flex. :/

I might have to abandon this bug and find an alternative solution.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #5)
> So this is annoying - it looks like if I inject the mainview into the
> anonymous content, the vbox above the menu panel's footer doesn't flex. :/
> 
> I might have to abandon this bug and find an alternative solution.

Found the problem - just forgot to set flex="1" on the menu panel. Should have a new patch for this soon.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Swapping out descendent selectors for child selectors.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 755574 [details] [diff] [review]
Patch v1.1

Review of attachment 755574 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/panelUI.xml
@@ +260,5 @@
>                }
>                break;
>              case "overflow":
>                // Resize the subview on the next tick.
> +              if (this.showingSubView) {

// Resize the view on the next tick.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +52,5 @@
>    overflow: visible;
>  }
>  
> +#PanelUI-popup > .panel-arrowcontainer > .panel-arrowcontent,
> +#customizationui-widget-panel > .panel-arrowcontainer > .panel-arrowcontent {

Why does the customization-widget-panel now have overflow:hidden and padding:0? That doesn't seem to be part of this bug, what issue is it fixing?

@@ +178,5 @@
>    display: none;
>  }
>  
> +panelview > toolbarbutton,
> +panelview > vbox > toolbarbutton,

This change will break the styling in customization mode, when the toolbarbuttons are wrapped in a toolbarpaletteitem.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 755574 [details] [diff] [review]
Patch v1.1

>--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>@@ -1,27 +1,27 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> panelmultiview {
>   -moz-binding: url("chrome://browser/content/customizableui/panelUI.xml#panelmultiview");
> }
> 
>-panelmainview,
>-panelsubview {
>+panelview {
>+  -moz-binding: url("chrome://browser/content/customizableui/panelUI.xml#panelview");

This really doesn't belong in themes/.

>   display: -moz-box;

xul.css already gives you this.

>+panelview > toolbarbutton > .toolbarbutton-text,
>+panelview > vbox > toolbarbutton > .toolbarbutton-text {

What kind of vbox is that? Does it have an ID or class that you can use instead?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2

(In reply to Jared Wein [:jaws] from comment #10)
> Comment on attachment 755574 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 755574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/content/panelUI.xml
> @@ +260,5 @@
> >                }
> >                break;
> >              case "overflow":
> >                // Resize the subview on the next tick.
> > +              if (this.showingSubView) {
> 
> // Resize the view on the next tick.
> 

Fixed.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +52,5 @@
> >    overflow: visible;
> >  }
> >  
> > +#PanelUI-popup > .panel-arrowcontainer > .panel-arrowcontent,
> > +#customizationui-widget-panel > .panel-arrowcontainer > .panel-arrowcontent {
> 
> Why does the customization-widget-panel now have overflow:hidden and
> padding:0? That doesn't seem to be part of this bug, what issue is it fixing?
> 

Good question - I think that was something I may have needed for the bookmarks subview patch. I'll remove it from this one, since it doesn't add much for this particular problem.

> @@ +178,5 @@
> >    display: none;
> >  }
> >  
> > +panelview > toolbarbutton,
> > +panelview > vbox > toolbarbutton,
> 
> This change will break the styling in customization mode, when the
> toolbarbuttons are wrapped in a toolbarpaletteitem.

Good point.

I'm going to revert the removal of the descendant selectors that Dao suggested for this patch, and I'm not going to block on that. I think I'd make more of a mess trying to use child selectors here, and I think we need a more thorough solution. I've filed bug 877697 to do an audit of panelUIOverlay.inc.css and clean up the descendant selectors.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.4

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756051 [details] [diff] [review]
Patch v1.4

Ok, I think that's cleaned it up.

We're definitely going to have to do something about all of these descendant selectors, but as I mentioned earlier, I'd like to tackle that in a separate bug (bug 877697).

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Landed in UX as https://hg.mozilla.org/projects/ux/rev/aa7f6cd90ff8

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Backed out due to introducing orange:

https://hg.mozilla.org/projects/ux/rev/aa7f6cd90ff8

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Re-landed as http://hg.mozilla.org/projects/ux/rev/b1b4efefa241

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/b1b4efefa241