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

RESOLVED FIXED in 1.0b3

Status

defect
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: philor, Assigned: squib)

Tracking

(Blocks 1 bug)

Trunk
1.0b3
Dependency tree / graph
Bug Flags:
blocking-calendar1.0 +

Details

(Whiteboard: [needed beta][has l10n impact])

Attachments

(5 attachments, 2 obsolete attachments)

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.
Flags: wanted-calendar1.0?
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.
Posted image 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.
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. :)
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #510540 - Flags: review?(philipp)
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!
Attachment #510540 - Flags: review?(philipp) → review-
(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.
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.
Attachment #510540 - Attachment is obsolete: true
Attachment #511901 - Flags: review?(philipp)
: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.)
Depends on: 282068
(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.
Flags: wanted-calendar1.0? → blocking-calendar1.0+
Whiteboard: [needed beta][no l10n impact]
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.
Attachment #511901 - Flags: review?(philipp) → review+
Whiteboard: [needed beta][no l10n impact] → [needed beta][has l10n impact]
(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.
(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. :)
Posted image Mac Screenshot β€”
Yes, mac needs different icons. You can probably copy them from the winstripe theme, right?
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.
Posted patch Fix - v3 β€” β€” Splinter Review
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.
Attachment #511901 - Attachment is obsolete: true
Attachment #513959 - Flags: review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3fb257ff3fb2>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/3a42e0c1638d>
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: Trunk → 1.0b3
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"
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.
(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.
Nicer looking icons for the mac header.
As I noticed this bug was marked as fixed first after I attached my patch, I've opened bug 639799 about that instead
Blocks: 657250
You need to log in before you can comment on or make changes to this bug.