963002 - Adjust height of panel (subview) footer(s)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

According to http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups-/windows8.html the panel footer should have a smaller height and both help and quit buttons should be square.

Comment 1

[u428464]

Note that the new size will match the subview footer height.

Comment 2

[u428464]

So in latest builds the footer is smaller but the styling is broken.

Comment 3

[u428464]

Attachment: Footer difference.png

So now the footer is a bit smaller than the specs and doesn't match the subview footer. A dark line also appears between the footer and the panel.

Comment 4

[u428464]

Attachment: Footer difference.png

The difference between the footers is even bigger than before. The size should be adjusted.

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch v1

Also fixes bug 973248

Comment 7

[:Gijs (he/him)]

Comment on attachment 8395366 [details] [diff] [review]
Patch v1

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

Thanks for the patch. Unfortunately, with this change, the subview footer is still bigger than the main panel footer on OS X, by about 3 pixels.

It is smaller than the main panel footer by about 3 pixels on Windows 7 in classic mode, and by 1px in aero.

This does look fine on Windows 8.

I expect it has to do with the fact that the other footer has icons, and those determine the height of the main panel footer (which was 36px + 1px border everywhere - 2 * 10px padding + 16px for the icon). You should probably set a min-height to 36/37px, verify that the vertical alignment doesn't go wonky (I *think* the existing XUL + CSS takes care of this, but do check) and then look at OS X if you can. If you don't have access to an OS X machine, let me know and I can poke some more after you address the Windows issues.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +441,5 @@
>    background-image: none;
>    border: none;  
>    border-radius: 0;
> +  transition-duration: 150ms;
> +  transition-property: background-color;

This rule was dead and should be removed rather than resurrected. We consciously removed the transitions from the other buttons in the panel, it's just an oversight that they're still there for the combined buttons and the footer, AFAIK. The combined button rules aren't dead yet, so we should probably leave those for a separate bug w/ ui-review. Let's focus on the footer in here.

Comment 8

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8395366 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8395366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. Unfortunately, with this change, the subview footer is
> still bigger than the main panel footer on OS X, by about 3 pixels.
> 
> It is smaller than the main panel footer by about 3 pixels on Windows 7 in
> classic mode, and by 1px in aero.
> 
> This does look fine on Windows 8.
> 
> I expect it has to do with the fact that the other footer has icons, and
> those determine the height of the main panel footer (which was 36px + 1px
> border everywhere - 2 * 10px padding + 16px for the icon). You should
> probably set a min-height to 36/37px, verify that the vertical alignment
> doesn't go wonky (I *think* the existing XUL + CSS takes care of this, but
> do check) and then look at OS X if you can. If you don't have access to an
> OS X machine, let me know and I can poke some more after you address the
> Windows issues.

Can I use line-height like the mockup does ?

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +441,5 @@
> >    background-image: none;
> >    border: none;  
> >    border-radius: 0;
> > +  transition-duration: 150ms;
> > +  transition-property: background-color;
> 
> This rule was dead and should be removed rather than resurrected. We
> consciously removed the transitions from the other buttons in the panel,
> it's just an oversight that they're still there for the combined buttons and
> the footer, AFAIK. The combined button rules aren't dead yet, so we should
> probably leave those for a separate bug w/ ui-review. Let's focus on the
> footer in here.

Sure :)

Comment 9

[:Gijs (he/him)]

That wouldn't work well with non-default font sizes on Windows (hence my suggestion for 'min-height' rather than 'height').

Comment 10

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #9)
> That wouldn't work well with non-default font sizes on Windows (hence my
> suggestion for 'min-height' rather than 'height').

Ok, then. Should I make the Panel footer use the .panel-subview-footer class ?

Comment 11

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > That wouldn't work well with non-default font sizes on Windows (hence my
> > suggestion for 'min-height' rather than 'height').
> 
> Ok, then. Should I make the Panel footer use the .panel-subview-footer class
> ?

*to avoid future inconsistencies.

Comment 12

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #11)
> (In reply to Tim Nguyen [:ntim] from comment #10)
> > (In reply to :Gijs Kruitbosch from comment #9)
> > > That wouldn't work well with non-default font sizes on Windows (hence my
> > > suggestion for 'min-height' rather than 'height').
> > 
> > Ok, then. Should I make the Panel footer use the .panel-subview-footer class
> > ?
> 
> *to avoid future inconsistencies.

That probably won't work well because it's a different kind of node. :-(

(besides, a bunch of the panel-subview-footer rules use .subviewbutton.panel-subview-footer, and we can't give the footer the "subviewbutton" class because it'd break a bunch of things, and we might not be able to adjust those selectors because of specificity concerns)

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v2

I only have Windows. Can you test this patch for me on other platforms please ?

Comment 14

[Tim Nguyen :ntim]

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

Canceling the review since I just figured out the same issue will be there.

Comment 15

[Tim Nguyen :ntim]

Giving up.

Comment 16

[Tim Nguyen :ntim]

Attachment: WIP for those who wish to fix this bug

This has a small 1px gap issue.

Comment 18

[u428464]

I saw the discussion on IRC, but I'll explain here. The purpose of this bug is to have the panel subview footer to have the same height as the menu panel footer (Customize one). Current when a subview is opened you can see the difference between them and that's not very nice visually.

Comment 19

[u428464]

So we have had a brief discussion on IRC yesterday about this and Jaws isn't sure it's worth fixing. I tend to think it is, but UX feedback is maybe best here.

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, it's annoying.
It comes down to how much work this would be. If this is a 1-3 (possibly 5) point effort, then it should be worth fixing. Anything above that, we can have more impact by improving other parts of the browser.
I'll leave the estimation to someone more qualified :)

Comment 21

[Marco Mucci [:MarcoM]]

Hi Madhava, based on today's update meeting you will confirm if any design changes will impact this bug and if it should be removed from the current iteration.

Comment 22

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

Attachment: :ntim's patch (rebased)

I tested Tim's patch on Windows 8, Windows 7 + Windows7/Classic and OSX 10.9. It will be a while before I will have my linux build finished.

Gijs, I don't see any footer height mismatches with Tim's patch. The only "subview" that it doesn't work well with is the Developer Tools which doesn't use a standard footer but just has menuitems at the bottom.

Comment 23

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

Also tested on Windows XP Luna Blue and Classic.

Comment 24

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

Tested on Ubuntu 14.04 as well.

Comment 25

[u428464]

Note that if the design of bug 996230 is implemented the work done here may be "undone".

Comment 26

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Created attachment 8438574 [details] [diff] [review]
> :ntim's patch (rebased)
> 
> I tested Tim's patch on Windows 8, Windows 7 + Windows7/Classic and OSX
> 10.9. It will be a while before I will have my linux build finished.
> 
> Gijs, I don't see any footer height mismatches with Tim's patch. The only
> "subview" that it doesn't work well with is the Developer Tools which
> doesn't use a standard footer but just has menuitems at the bottom.

Yeah, it wouldn't work with things without footers, but that's not an issue IMO. I can try to review this either tomorrow or later tonight, depending on how my energy levels go... in any case, I wonder what Tim meant with comment #16, then. Tim? :-)

Comment 27

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #26)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> > Created attachment 8438574 [details] [diff] [review]
> > :ntim's patch (rebased)
> > 
> > I tested Tim's patch on Windows 8, Windows 7 + Windows7/Classic and OSX
> > 10.9. It will be a while before I will have my linux build finished.
> > 
> > Gijs, I don't see any footer height mismatches with Tim's patch. The only
> > "subview" that it doesn't work well with is the Developer Tools which
> > doesn't use a standard footer but just has menuitems at the bottom.
> 
> Yeah, it wouldn't work with things without footers, but that's not an issue
> IMO. I can try to review this either tomorrow or later tonight, depending on
> how my energy levels go... in any case, I wonder what Tim meant with comment
> #16, then. Tim? :-)

The subview footer was 1px higher than the main menu footer (even with pixel perfect adjustments). I'm not sure if it's still the case now though.

Comment 28

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #25)
> Note that if the design of bug 996230 is implemented the work done here may
> be "undone".

I don't really understand this. I expect that the footer size there is just an artifact of how it is now - it's probably just a manipulated screenshot where they didn't mess with the footer size. In other words, if we fix it now, and as long as bug 996230 doesn't break it, we should be fine.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8438574 [details] [diff] [review]
:ntim's patch (rebased)

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

I'm going to trust your due diligence on testing this, rs=me codewise.

Comment 30

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

https://tbpl.mozilla.org/?tree=Try&rev=ffbfeb5c9057

Comment 31

[u428464]

(In reply to :Gijs Kruitbosch from comment #28)
> (In reply to Guillaume C. [:ge3k0s] from comment #25)
> > Note that if the design of bug 996230 is implemented the work done here may
> > be "undone".
> 
> I don't really understand this. I expect that the footer size there is just
> an artifact of how it is now - it's probably just a manipulated screenshot
> where they didn't mess with the footer size. In other words, if we fix it
> now, and as long as bug 996230 doesn't break it, we should be fine.

In fact the adjusted footer should be approximately 38 px on Windows and the new Mike Maslaney spec shows 40 px (https://bug996230.bugzilla.mozilla.org/attachment.cgi?id=8436013)

Comment 32

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #31)
> (In reply to :Gijs Kruitbosch from comment #28)
> > (In reply to Guillaume C. [:ge3k0s] from comment #25)
> > > Note that if the design of bug 996230 is implemented the work done here may
> > > be "undone".
> > 
> > I don't really understand this. I expect that the footer size there is just
> > an artifact of how it is now - it's probably just a manipulated screenshot
> > where they didn't mess with the footer size. In other words, if we fix it
> > now, and as long as bug 996230 doesn't break it, we should be fine.
> 
> In fact the adjusted footer should be approximately 38 px on Windows and the
> new Mike Maslaney spec shows 40 px
> (https://bug996230.bugzilla.mozilla.org/attachment.cgi?id=8436013)

FYI, that spec isn't finished, it still requires some tweaks. Especially in the colors, that are not high contrast oompatible (our current color is a translucent gray, while in his spec, the colors are fully opaque).

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a9c9191dc939

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Tim Nguyen :ntim]

Comment on attachment 8438574 [details] [diff] [review]
:ntim's patch (rebased)

Can we get the patch uplifted to aurora and beta please ?

Comment 36

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

Yeah, this is early enough in the cycle and the change is low-risk enough that an uplif to Aurora and Beta should be fine.

Tim, please request aurora and beta approval on the patch, and make sure that the patch applies and works on Aurora and Beta. If not then we'll need new patches for those and it will make the uplift doubtful.