1003196 - Add icons to more imip bar buttons

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[[:MakeMyDay]]

Attachment: ImipBarIconsWin7.png

To follow up to bug 990009, we propbably need some more icon for the imipbar buttons.

These buttons currently don't have an icon:

.imipDetailsButton
-> open the event or summary dialog

.imipMoreButton
-> dropdown button to offer more options to the user

.imipAddButton
-> add an event to the calendar

.imipUpdateButton
-> ipdate an event in the calendar

.imipDeleteButton
-> delete an event from the calendar

.imipReconfirmButton
-> reconfirm the own participation status on organizer's request

Addtionally, these buttons share the same icons as those without recurrences, maybe they should also get different ones:

.imipAcceptRecurrencesButton
.imipDeclineRecurrencesButton
.imipTentativeRecurrencesButton
-> the same functionality as standard accept, deline, tentative buttons, but
   for recurring events

The respective css definitions are located at:

/calendar/base/themes/windows/win-aero/lightning.css
/calendar/base/themes/windows/win-classic/lightning.css
/calendar/lightning/themes/linux/lightning.css
/calendar/lightning/themes/osx/lightning.css

Last but not least, the icon used for accepting tentatively look a little bit blurry on Win 7 compared to the both others - see attached screenshot.

Comment 1

[[:MakeMyDay]]

@philipp: should we mark this also wanted for upcoming release for TB 31? It's not a must have, but people will will notice the mixture of buttons with and without icons.

Comment 2

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

We would need icons to do that. Although unfortunate, I don't think we should burden the release with more changes than needed. I once did a seemingly simple CSS change shortly before the release and had to do a chemspill because the CSS change broke something unrelated and major.

Comment 3

[[:MakeMyDay]]

As discussed on the summit with Paenglab, we should hide the space reserved for the icons from the buttons without icons until we have the icons. See bug 1089362.

Comment 4

[[:MakeMyDay]]

I have had a look, whether we can recyle some existing icons for the remaining buttons without icons. For Mac and Windows-Aero this seems not to hard:

+.imipAddButton {
+  list-style-image: url(chrome://calendar/skin/toolbar.png);
+  -moz-image-region: rect(0px 32px 16px 16px);
+}
+
+.imipAddButton:active {
+  -moz-image-region: rect(16px 32px 32px 16px);
+}
+
+.imipUpdateButton {
+  list-style-image: url(chrome://calendar/skin/toolbar.png);
+  -moz-image-region: rect(0px 16px 16px 0px);
+}
+
+.imipUpdateButton:active {
+  -moz-image-region: rect(16px 16px 32px 0px);
+}
+
+.imipDeleteButton {
+  list-style-image: url(chrome://calendar/skin/toolbar.png);
+  -moz-image-region: rect(0px 96px 16px 80px);
+}
+
+.imipDeleteButton:active {
+  -moz-image-region: rect(16px 96px 32px 80px);
+}
+
+.imipReconfirmButton {
+  list-style-image: url(chrome://calendar/skin/tasks-actions.png);
+  -moz-image-region: rect(0px 48px 16px 32px);
+}
+
+.imipReconfirmButton:active {
+  /*currently no separate image available*/
+}
+
+.imipMoreButton {
+  /* we should crop the image space of this button */
+}

For Linux and Win Classic, I'm not sure whether the corresponding icons from toolbar.png are really appropriate.

Richard, what do you think?

Comment 5

[Richard Marti (:Paenglab)]

I think this should work. Actually I'm on moving from png files to svg files for the toolbars. This would then use one shared file for Linux, OS X and Windows Vista+. I leave XP as it is because I think users which are still on XP don't want any change.
For now, the Linux and XP icons should also work also when they are a little different. For Linux this should change with my patch. I don't know when my patch can be checked-in as I'm awaiting the review for TB first (bug 1150627).

I have started a try build with the changes for TB and Lightning if you want to check it: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d378ce077386

Every feedback is welcome.

Comment 6

[[:MakeMyDay]]

(In reply to Richard Marti (:Paenglab) from comment #5)
> I think this should work.

So, you're satisfied with the association of the different icons to the respective buttons? (Sorry, I didn't manage to prepare a screenshot yet)

> Actually I'm on moving from png files to svg files
> for the toolbars. This would then use one shared file for Linux, OS X and
> Windows Vista+. I leave XP as it is because I think users which are still on
> XP don't want any change.

Is the new style similar to the current Mac/Win Aero style or different?

> For now, the Linux and XP icons should also work also when they are a little
> different. For Linux this should change with my patch. I don't know when my
> patch can be checked-in as I'm awaiting the review for TB first (bug
> 1150627).

Ok, I keep an eye on it, but I would love to get the icons to the imip bar button still for the next beta, so let's see.

> I have started a try build with the changes for TB and Lightning if you want
> to check it:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=d378ce077386
> 
> Every feedback is welcome.

Unfortunately, I don't have a Mac around, but if you retrigger an all-plattform try build, I'm happy to give feedback.

Comment 7

[Richard Marti (:Paenglab)]

(In reply to MakeMyDay from comment #6)
> (In reply to Richard Marti (:Paenglab) from comment #5)
> > I think this should work.
> 
> So, you're satisfied with the association of the different icons to the
> respective buttons? (Sorry, I didn't manage to prepare a screenshot yet)
> 
> > Actually I'm on moving from png files to svg files
> > for the toolbars. This would then use one shared file for Linux, OS X and
> > Windows Vista+. I leave XP as it is because I think users which are still on
> > XP don't want any change.
> 
> Is the new style similar to the current Mac/Win Aero style or different?

Yes, like the current Mac/Win Aero style.

> > For now, the Linux and XP icons should also work also when they are a little
> > different. For Linux this should change with my patch. I don't know when my
> > patch can be checked-in as I'm awaiting the review for TB first (bug
> > 1150627).
> 
> Ok, I keep an eye on it, but I would love to get the icons to the imip bar
> button still for the next beta, so let's see.

No problem, I can then update my patch.

> Unfortunately, I don't have a Mac around, but if you retrigger an
> all-plattform try build, I'm happy to give feedback.

For Win and Linux: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a113818e2f82

Comment 8

[[:MakeMyDay]]

Attachment: NewImipBarIcons.png

Attaching the selected icons - I have a patch ready for this.

The only missing thing is to crop the space in the More.. button. Richard, do you have an idea what inherited style needs to be overwritten for that?

Apart from that: I checked your Lightning SVG try build for Windows and have some findings. Do you already have a bug for that where I can comment?

Comment 9

[Richard Marti (:Paenglab)]

(In reply to MakeMyDay from comment #8)
> Created attachment 8591328 [details]
> NewImipBarIcons.png
> 
> Attaching the selected icons - I have a patch ready for this.

This looks good. With Classic you mean XP, right?

> The only missing thing is to crop the space in the More.. button. Richard,
> do you have an idea what inherited style needs to be overwritten for that?

Add in calendar/lightning/themes/common/lightning.css:

.imipMoreButton > .toolbarbutton-icon {
  display: none;
}

> Apart from that: I checked your Lightning SVG try build for Windows and have
> some findings. Do you already have a bug for that where I can comment?

Not yet, but I'm filing one today and CC you.

Comment 10

[[:MakeMyDay]]

Attachment: AddIconsToToolbarButtons-V1.diff

Yes, classic = XP.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 8591339 [details] [diff] [review]
AddIconsToToolbarButtons-V1.diff

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

Looks good. I have only tested Linux, XP and Win7 but from code OS X looks good.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/e3416e8b3075

Uplifts to aurora and beta being done after the patch passes on central.

Comment 13

[Martin Schröder [:mschroeder]]

This still not landed on aurora and beta, so this fix is not in 4.0 afaik.

Comment 14

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

Backported to releases/comm-aurora changeset 0111428ad283

Comment 15

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

Backported to releases/comm-beta changeset 37812d78b380