If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Adjust panelmultiview binding to allow easy swapping of mainviews

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M6])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 751498 [details] [diff] [review]
WIP Patch 1

Here's what I'm proposing. Does this look like a reasonable approach, or can we do better?
Attachment #751498 - Flags: feedback?(dao)
Attachment #751498 - Flags: feedback?(bmcbride)
Status: NEW → ASSIGNED
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.
Attachment #751498 - Flags: feedback?(bmcbride) → feedback+
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
Attachment #751498 - Flags: feedback?(dao) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 753891 [details] [diff] [review]
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.
Attachment #751498 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 755419 [details] [diff] [review]
WIP Patch 3
Attachment #753891 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 755566 [details] [diff] [review]
Patch v1
Attachment #755419 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 755574 [details] [diff] [review]
Patch v1.1

Swapping out descendent selectors for child selectors.
Attachment #755566 - Attachment is obsolete: true
Attachment #755574 - Flags: review?(jaws)
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.
Attachment #755574 - Flags: review?(jaws)
Hardware: x86 → All
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?
(Assignee)

Comment 12

4 years ago
Created attachment 756039 [details] [diff] [review]
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.
Attachment #755574 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 756045 [details] [diff] [review]
Patch v1.3
Attachment #756039 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 756051 [details] [diff] [review]
Patch v1.4
Attachment #756045 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
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).
Attachment #756051 - Flags: review?(bmcbride)
Attachment #756051 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 16

4 years ago
Landed in UX as https://hg.mozilla.org/projects/ux/rev/aa7f6cd90ff8
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
(Assignee)

Comment 17

4 years ago
Backed out due to introducing orange:

https://hg.mozilla.org/projects/ux/rev/aa7f6cd90ff8
(Assignee)

Comment 18

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

Comment 19

4 years ago
https://hg.mozilla.org/mozilla-central/rev/b1b4efefa241
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.