465317 - Today and Navigation Buttons need to be polished

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Christian Jansen]

This bug shall address comment #13 of Bug 454933

[...]
Aside from that, I really don't like the way the navigation buttons ( former "<
o >" ) look like. More specifically, the hover seems a bit strange and "Today"
doesn't really look like a button until hovered. It may be a bit confusing, i.e
user thinks "Today" is in the view (day view, most notably). But I guess this
is up to Christian.
[...]

Comment 1

[Sven Giermann]

I also preferred the 0.9 layout!
Why had it been changed?

Comment 2

[Decathlon]

Attachment: [checked in] toolbarbutton style for navigation buttons

This patch restores the look of navigation buttons, for the 'hover' state, like it is on Calendar 0.9 (see screenshot).
With '-moz-appearance: toolbarbutton', navigation buttons become like the others navigation buttons on minimonth in the left pane and miniday on today pane.
I haven't modified padding and margin in pinstripe theme because I can't verify the look on a mac (reading about bug 465512 it seems slightly different).

About the problem of today button mentioned in comment #0, I've posted in the screenshot how it would look like if it was a normal button (with '-moz-appearance: button' and some adjustments in paddings and margins) but I don't know if this is what you want.

Comment 3

[Decathlon]

Attachment: screenshot

Navigation buttons with and without patch.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 382814 [details] [diff] [review]
[checked in] toolbarbutton style for navigation buttons

Codewise, this patch looks fine. Waiting for a ui-review now. r=philipp

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 382815 [details]
screenshot

toolbar button looks like the obvious winner to me.  Nice work!

Comment 6

[Martin Schröder [:mschroeder]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/ba45a0f68588>

-> FIXED

The buttons should be fine-tuned for the pinstripe theme in bug 465512!

Comment 7

[Decathlon]

It seems that today button is 1 pixel higher than buttons with arrows.
Can someone confirm?
When I tried the patch it was fine (see screenshot). I think there is the problem of elements with different height when they have icons or text (because when text is used, height depends on font size) like  Bug 453067 or  Bug 456832 for Thunderbird.
In fact the patch (like previous code) sets the padding in the button and not a min or max height.

If it's worth, on my system the following patch would fix the problem(?), but I don't know if something different should be considered (if this is a real problem):

diff --git a/calendar/base/themes/winstripe/calendar-views.css b/calendar/base/themes/winstripe/calendar-views.css
--- a/calendar/base/themes/winstripe/calendar-views.css
+++ b/calendar/base/themes/winstripe/calendar-views.css
@@ -705,17 +705,17 @@ tab[calview] > .tab-middle {
     padding: 4px;
 }
 
 .today-navigation-button {
     -moz-user-focus: normal;
     -moz-appearance: toolbarbutton;
-    margin-top: 2px;
+    margin-top: 3px;
     -moz-margin-start: 2px;
     -moz-margin-end: 2px;
     margin-bottom: 0px;
-    padding: 4px;
+    padding: 3px;
     padding-bottom: 5px;
     color: #2E4E73;
 }
 
 .view-navigation-button > .toolbarbutton-text {
     display: none;

Comment 8

[Martin Schröder [:mschroeder]]

(In reply to comment #7)
> It seems that today button is 1 pixel higher than buttons with arrows.
> Can someone confirm?

Decathlon, at least 1px... but I use Windows XP with Classic theme. Your fix doesn't help in my build and with Classic theme. Bug 465512 surprisingly has a winstripe part, so maybe you want to have a look at it, and we can find a solution over there. ;)

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

What helped for me is to change the alignment of those elements to center (or was it pack?). This way they are different size, but still look aligned correctly.

Comment 10

[Decathlon]

(In reply to comment #8)
> (In reply to comment #7)
> > It seems that today button is 1 pixel higher than buttons with arrows.
> > Can someone confirm?
> 
> Decathlon, at least 1px... but I use Windows XP with Classic theme. Your fix
> doesn't help in my build and with Classic theme. Bug 465512 surprisingly has a
> winstripe part, so maybe you want to have a look at it, and we can find a
> solution over there. ;)

OK, with Philipp' suggestion in comment #9 and changes in comment #7, every problem of font size and buttons height seems solved, even with Win classic theme.
Where have I to attach the patch, here or to the bug 465512? And who should I ask for review?

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Go ahead and attach it here, I'll do the review.

Comment 12

[Decathlon]

Attachment: patch for buttons height related to font size

Added an horizontal box with attribute pack="center" and changed margins and padding for today button.
Here, like the previous patch, I haven't changed margin and padding for pinstripe theme.
Below a screenshot with small and large font size (extreme size) on Win XP with Classic theme.

Comment 13

[Decathlon]

Attachment: Screenshot on Win XP with classic theme and font 6pt - 14pt

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Can't you just set pack=center on the parent box? Or does that break other things?

Comment 15

[Decathlon]

Attachment: Screenshot with pack=center on the parent box

That's what happens with 'pack' attribute on the parent box:

<hbox pack="center" flex="1" class="navigation-inner-box" align="end" >

It's the same result you get without pack attribute.

I'm thinking that since the labels of interval date and calendar week number have a font with height that doesn't change with system font (like the screenshot shows), maybe also text in today button should do the same unless usability reasons (if a user want a bigger text, it want also a bigger button, but this would be true only for that button because e.g. buttons on minimonth, don't change with system font size).

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Right, sorry. I had the wrong thing in mind when I wrote the last comment. Try this:

<hbox flex="1" class="navigation-inner-box" align="center" >

Comment 17

[Decathlon]

Attachment: Screenshot with align=center

(In reply to comment #16)
> Try this:
> 
> <hbox flex="1" class="navigation-inner-box" align="center" >

In this way labels in navigation header keep alignment with arrows icons (or bottom side of 'today' label) i.e. they move to the top when system font size increases, but they lose alignment with navigation tab labels ('Day', 'Week', ...), moreover navigation buttons still have the problem of different height with different system fonts size.

Maybe to get: 
- alignment of buttons with calendar labels in navigation header and
- buttons with the same height when system font change size,
both solutions could be applied: "align=center"  for "navigation-inner-box"  and the hbox with "pack=center" for the buttons like in patch 399711. With both solutions the navigation header is showed in last two images.
Anyway it would need an adjustment in margins and paddings for buttons or labels in navigation header. 
Moreover if request in bug 456745 is valid, it might change again the alignment between button and labels.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 399711 [details] [diff] [review]
patch for buttons height related to font size

(In reply to comment #17)
> Maybe to get: 
> - alignment of buttons with calendar labels in navigation header and
> - buttons with the same height when system font change size,
> both solutions could be applied: "align=center"  for "navigation-inner-box" 
> and the hbox with "pack=center" for the buttons like in patch 399711. With both
> solutions the navigation header is showed in last two images.
> Anyway it would need an adjustment in margins and paddings for buttons or
> labels in navigation header. 
Fine with me.

> Moreover if request in bug 456745 is valid, it might change again the alignment
> between button and labels.
In that case lets go for the solution with the least pain here. I'm fine with whatever you think looks best :)


r- on this patch to get a new patch as proposed. Thanks for taking care!

Comment 19

[Decathlon]

Attachment: New patch screenshot

I have a patch that generates this result on Windows with Classic and Luna themes. Philipp, what about it?

I've also added a screenshot with bigger navigation button if bug 456745 should be take in count. It reduces a bit the horizontal space (as you can see by comparing the fourth and last image). What's your opinion?

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Looks good to me. The CW is not quite centered, but I don't think thats a bad thing. Looking forward to your patch!

Comment 21

[Decathlon]

Attachment: [medium,test] New patch

(In reply to comment #20)
> The CW is not quite centered, but I don't think thats a bad thing

Actually the patch changes only the vertical position of CW and interval description to keep them aligned with navigation buttons.
If you are talking about distance from CW to tabs, it is the same distance from navigation button's right arrow to interval description.
With the button in hover status the distance from the right side of the *button* is shorter, but I think it should be evaluate with the button in normal status (i.e. the distance from the icon inside the button).

I haven't changed size buttons (please tell me if I have to do it maybe in bug 456745) and I haven't applied changes for pinstripe theme.

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 414381 [details] [diff] [review]
[medium,test] New patch

I meant vertically centered, w.r.t the tabs. But it is centered with the text to the left of it, so thats fine. Looks fine on linux, patch works as expected.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6ed701e2db34>

-> FIXED