470824 - messagereader-style buttons in Tasks view are different heights on Windows

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Phil Ringnalda (:philor)]

The Tb3-style buttons that bug 456379 added for Tasks have three different heights for the four buttons on Windows (XP, Luna or Classic, dunno about Vista).

The thing controlling their height is the dropmarker, so for Category and Priority (button type="menu"), the height comes from a 14px dropmarker with 2px of margin and 2px of padding for 18px, inside a button-box with 2px of border and 3px of padding, for 23px for the whole thing, while Mark Completed (button type="menubutton") has the same 14px dropmarker with 2px of margin and 2px of padding  as a direct child of the button, without the intervening button-box, so it's only 18px high, while the trashcan is 16px of image in a button-box with 2px of border and 3px of padding, so it's 21px high.

Comment 1

[dave.r.wilson]

This problem exists in Vista as well, with the Category and Priority buttons being 23px high, the Mark Completed button 18px high and the Delete button 21px high.

Comment 2

[Jim Porter (:squib)]

Attachment: Ugly buttons in task pane

This is especially obnoxious now since (possibly as of 3.1), the "mark completed" button shows up as two nested buttons (see attachment). Ultimately, this is because core doesn't have good styles for non-toolbar menu-buttons, but Thunderbird adds its own style rules for these buttons. However, the CSS is overly specific, so Lightning doesn't get to use it.

Comment 3

[Jim Porter (:squib)]

Attachment: Fix this by turning the buttons into a toolbar

This turns the task buttons into a proper toolbar, which lets it use all the CSS from Thunderbird. Doing it this way does also require that all the buttons have both icons and text, but the icons already exist, so lucky us! (It also looks a lot prettier this way, in my opinion.)

Someone should probably check that the OS X version of this looks decent, since I'm not sure what it should look like there. The icons may be too big (24px square)...

This almost certainly requires my patch from bug 282068, though it'd be easy enough to reverse this dependency (also note that that patch only has the necessary CSS changes under Gnomestripe at the moment).

Other things to consider in this patch for consistency with Thunderbird:
* Change "Delete Task" to "Delete"
* Remove the button tooltips, which aren't that helpful anyway
* Make the button labels all lowercase like Thunderbird (though maybe
  Thunderbird should be the one to change here)

Hopefully, I flagged the right person for review of this. I've contributed to Thunderbird for a while, but this is my first Calendar patch. :)

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 510540 [details] [diff] [review]
Fix this by turning the buttons into a toolbar

Hey Jim,

thank you very much, your first Calendar patch looks very good, codewise I have nothing to complain about. I'm still having a bit of trouble testing it though.

* Any reason you didn't put the delete button in the default set?
* At least on linux, the icons are above the text
* When you right click and customize, the customize toolbar dialog has some errors (buttons are missing icons, "show" dropdown is missing content).
* Accepting changes in the customize dialog makes it no longer possible to click in the main menubar (File,Edit,...).

The button actions are working very well though.

Let me know if you need any assistance and thank you very much for your contribution.
I will test it on Mac on Monday or later when I have things set up.

> Other things to consider in this patch for consistency with Thunderbird:
> * Change "Delete Task" to "Delete"
> * Remove the button tooltips, which aren't that helpful anyway
I don't have a strong opinion here, if you want this changed talk to andreasn for a UI decision.

> * Make the button labels all lowercase like Thunderbird (though maybe
>   Thunderbird should be the one to change here)
bug 620221

r- for now to resolve the testing issues from above, you're going in the right direction though!

Comment 5

[Jim Porter (:squib)]

(In reply to comment #4)
> * Any reason you didn't put the delete button in the default set?
> * At least on linux, the icons are above the text
> * When you right click and customize, the customize toolbar dialog has some
> errors (buttons are missing icons, "show" dropdown is missing content).

Hmm. I probably forgot about these because I immediately customized the toolbar and so I was looking at its persisted state rather than its initial state.

> * Accepting changes in the customize dialog makes it no longer possible to
> click in the main menubar (File,Edit,...).

There seem to be some other modifications that the message header toolbar in Thunderbird does, so I'm going to try to make sure everything is in sync. That should (theoretically) resolve this.

> > Other things to consider in this patch for consistency with Thunderbird:
> > * Change "Delete Task" to "Delete"
> > * Remove the button tooltips, which aren't that helpful anyway
> I don't have a strong opinion here, if you want this changed talk to andreasn
> for a UI decision.

Will do.

Comment 6

[Jim Porter (:squib)]

Attachment: Address issues in comment 4 (by porting changes in bug 519956)

This should resolve the issues in comment 4 as well as a few others. Notably, the customization dialog works like the one for the message header, so "icon and text" is no longer an option, there's no small icons checkbox, and you can't add extra toolbars. This should help keep people from doing anything too strange.

Comment 7

[Jim Porter (:squib)]

:andreasn, do you have any opinions about the following?

> Someone should probably check that the OS X version of this looks decent, since
> I'm not sure what it should look like there. The icons may be too big (24px
> square)...

and

> * Change "Delete Task" to "Delete"
> * Remove the button tooltips, which aren't that helpful anyway

If 24x24 px is too big for header icons (I'm pretty sure pinstripe icons for the message header are 16x16), we might want some new ones.

(Also, marking dependency on bug 282068, since that has some CSS changes needed for this bug.)

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #7)

> > * Change "Delete Task" to "Delete"
Actually, looking at the updated patch I think this is a good idea. It saves some room in the toolbar, which is critical for smaller screen resolutions.


Note that we want to string freeze in the coming week, I think we should take this bug since it greatly improves the UI, even without the patch from bug 282068.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 511901 [details] [diff] [review]
Address issues in comment 4 (by porting changes in bug 519956)

r+ codewise, pending mac testing, using a new string for "Delete" and a decision on the tooltips.

Comment 10

[Andreas Nilsson (:andreasn)]

(In reply to comment #7)
> :andreasn, do you have any opinions about the following?
> 

> > * Change "Delete Task" to "Delete"
Delete Task is more describing about what you are deleting, but takes up more space. I'm ok with this change.

> > * Remove the button tooltips, which aren't that helpful anyway
Not sure about this. They offer deeper descriptions about what's going to happen when you press them.

> If 24x24 px is too big for header icons (I'm pretty sure pinstripe icons for
> the message header are 16x16), we might want some new ones.

Yes, we do. I can help with those tomorrow.

Comment 11

[Jim Porter (:squib)]

(In reply to comment #10)
> Not sure about this. They offer deeper descriptions about what's going to
> happen when you press them.

Actually, I just now noticed that the message header in Thunderbird does this too, so nevermind. :)

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Mac Screenshot

Yes, mac needs different icons. You can probably copy them from the winstripe theme, right?

Comment 13

[Jim Porter (:squib)]

Andreas said he'd look at doing new icons, so I'll wait for that and then incorporate them and the string change into the patch.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

I've taken the liberty to copy the icons from winstripe and make the string change, I hope you don't mind. I'll check this in now so we can proceed on a string freeze.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3fb257ff3fb2>
-> FIXED

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/3a42e0c1638d>

Comment 17

[Decathlon]

Attachment: Task bar on Windows XP and Windows 7

The new task bar has some problem on Win Xp and Win 7 as well, see screenshot.

Moreover, I'm not completely sure about the update timing of the repository, but I've tested Lightning 1.0b3 pre Italian locale ( http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/nightly/latest-comm-1.9.2-l10n/win32-xpi/)

and the task view doesn't work. The Error Console reports:

Error: undefined entity
Source File: chrome://calendar/content/calendar-task-view.xul
Line: 194, Column: 19
Source Code:
                  <toolbarbutton id="calendar-delete-task-button"

Comment 18

[Stefan Sitter [:ssitter]]

Decathlon, check <https://l10n-stage-sj.mozilla.org/dashboard/?tree=calendar10x> to see the current localization status for each language. You could contact the corresponding l10n team if you want additional information.

Comment 19

[Decathlon]

(In reply to comment #18)
> Decathlon, check
> <https://l10n-stage-sj.mozilla.org/dashboard/?tree=calendar10x> to see the
> current localization status for each language. You could contact the
> corresponding l10n team if you want additional information.

Sorry Stefan, my fault.
I had not understood that the patch had added new strings.

Comment 20

[Andreas Nilsson (:andreasn)]

Attachment: calendar header icons for the mac

Nicer looking icons for the mac header.

Comment 21

[Andreas Nilsson (:andreasn)]

As I noticed this bug was marked as fixed first after I attached my patch, I've opened bug 639799 about that instead