858067 - Make the panel in customization mode look more like the panel when opened.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Currently when entering Australis customization mode, the menu panel appears as a plain rectangle to the right of the palette.

The spec calls for the panel to resemble its appearance for when it's opened in normal mode.

Comment 1

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

Blake:

You want to look at browser/base/content/browser.xul - particularly, the customization-panelHolder hbox.

What happens is that when we enter customization mode, we slurp out the main view in the panel, and plop it into that holder.

Style changes (if any) should go into browser/themes/windows/browser.css, browser/themes/linux/browser.css, browser/themes/osx/browser.css. Like we were taking though, if we could somehow share the panels' style (which are defined in /toolkit/themes/osx/global/popup.css, /toolkit/themes/windows/global/popup.css and /toolkit/themes/linux/global/popup.css, that'd probably be best).

Comment 2

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

Whoops - I made a mistake. I forgot to mention that customization-panelHolder is in an include - browser/base/content/customization.inc. This is included during pre-processing into browser.xul.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch 1.

So, this seems like a direction that could work…  What do you think, Mike?  Worth pursuing further?

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at this patch.

Comments:

I moved the "Move the mainView in the panel" code earlier, because we want the whole populated menu to look as if it's animating in, and moving the things after transitionend looked really weird.

I have only fixed the padding for osx (in browser/themes/osx/browser.css), because I haven't tried building on other platforms.  I _suspect_ something similar will be needed there, but I'll do that when I fix the other problems you uncover with this patch.  :)

Thanks,
Blake.

Comment 5

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

Comment on attachment 734156 [details] [diff] [review]
The first cut at this patch.

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

This looks really, really good - and I'm pleased with how neatly we can just take the panel styling.

What is the padding for? Can you give me a screenshot showing what it fixes on OSX? I don't think we need it for Windows...

::: browser/modules/CustomizeMode.jsm
@@ +91,5 @@
> +    // Move the mainView in the panel to the holder so that we can see it
> +    // while customizing.
> +    let panelHolder = document.getElementById("customization-panelHolder");
> +    panelHolder.appendChild(window.PanelUI.mainView);
> +

Please get rid of this extra newline.

::: browser/themes/osx/browser.css
@@ +3616,5 @@
>    box-shadow: inset 0 0 3px blue;
>    border-radius: 2px;
>  }
>  
> +#customization-panelWrapper {

Can you show me a screenshot with this padding?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

https://dl.dropbox.com/u/2301433/Screenshots/Padding.png

Comment 7

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

(In reply to Blake Winton (:bwinton) from comment #6)
> https://dl.dropbox.com/u/2301433/Screenshots/Padding.png

Oh, ok - I hadn't noticed that this was wanted (space between the right-side of the window and the panel).

Then yes, I think we should bring this padding over to the other platforms.

Comment 8

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

Looks good, can we experiment with positioning the panel to the right more so that the arrow still lines up with the button (overlapping some of the blue)?

Comment 9

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

(In reply to Jared Wein [:jaws] from comment #8)
> Looks good, can we experiment with positioning the panel to the right more
> so that the arrow still lines up with the button (overlapping some of the
> blue)?

That'd be removing the padding. I'm fine with trying this, so long as UX is too.

Comment 10

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

(In reply to Mike Conley (:mconley) from comment #9)
> (In reply to Jared Wein [:jaws] from comment #8)
> > Looks good, can we experiment with positioning the panel to the right more
> > so that the arrow still lines up with the button (overlapping some of the
> > blue)?
> 
> That'd be removing the padding. I'm fine with trying this, so long as UX is
> too.

Is it really? The 'without padding' screenshot doesn't have the panel lined up well. Note that the panel would also have to be moved vertically up also.

Comment 11

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

(In reply to Jared Wein [:jaws] from comment #10)
> (In reply to Mike Conley (:mconley) from comment #9)
> > (In reply to Jared Wein [:jaws] from comment #8)
> > > Looks good, can we experiment with positioning the panel to the right more
> > > so that the arrow still lines up with the button (overlapping some of the
> > > blue)?
> > 
> > That'd be removing the padding. I'm fine with trying this, so long as UX is
> > too.
> 
> Is it really? The 'without padding' screenshot doesn't have the panel lined
> up well. Note that the panel would also have to be moved vertically up also.

Ohhhhh - I see - you want the panel over the grid.

No, I'm not opposed to trying that either, although this isn't part of the original prototype...

I don't think I feel too strongly either way - but if I was forced to choose, I'd choose to follow the prototype unless I hear otherwise from the UX team.

Comment 12

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

Attachment: Patch v2

Adds padding to Linux and Windows themes.

Comment 13

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

I have a feeling we're not going to hear from the UX team, since they're at a work week.

I'm just going to land this, and we can file follow-up's if this really isn't exactly the way we want to roll.

Comment 14

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

Landed on jamun: https://hg.mozilla.org/projects/jamun/rev/ef52c9b57900

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 735548 [details] [diff] [review]
Patch v2

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul

>+<?xml-stylesheet href="chrome://global/skin/popup.css" type="text/css"?>

This is a pretty nasty and fragile hack. This stylesheet is a scoped one and supposed to be used in anonymous content.

>--- a/browser/base/content/customize.inc
>+++ b/browser/base/content/customize.inc

>+            <box anonid="arrowbox" class="panel-arrowbox">
>+              <box flex="1"/>
>+              <image anonid="arrow" class="panel-arrow" side="top"/>
>+            </box>
>+            <box class="panel-arrowcontent" flex="1">
>+              <hbox id="customization-panelHolder"/>
>+              <box class="panel-inner-arrowcontentfooter" hidden="true"/>
>+            </box>

The anonid attributes are pointless.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 735548 [details] [diff] [review]
Patch v2

Try this:

<vbox id="customization-panelWrapper">
  <html:style html:type="text/html" scoped="scoped">
    @import url(chrome://global/skin/popup.css);
  </html:style>
  ...

Comment 17

[Dão Gottwald [:dao]]

(you need to set xmlns:html on <window> first)

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Attachment: Followup patch, v1.

A patch implementing Dao's suggestions.  They look like they work for me, on Mac.

Comment 19

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

Follow-up landed on jamun as https://hg.mozilla.org/projects/jamun/rev/27d9eb03de12

Comment 20

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

Landed on UX as https://hg.mozilla.org/projects/ux/rev/ef52c9b57900

Follow-up landed on UX as https://hg.mozilla.org/projects/ux/rev/27d9eb03de12

Comment 21

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #16)
>   <html:style html:type="text/html" scoped="scoped">
>     @import url(chrome://global/skin/popup.css);
>   </html:style>

Dão, what's the significance of 'html:type="text/html"'? Should that have been text/css or is this a trick to preload the CSS?

Comment 22

[Dão Gottwald [:dao]]

That was probably just a typo.

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/895fbf25a82f
https://hg.mozilla.org/mozilla-central/rev/acd3f21b21b3