Closed Bug 393763 Opened 13 years ago Closed 9 years ago

Make Today Pane toolbar button image more i18n-friendly

Categories

(Calendar :: Lightning Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sipaq, Assigned: bv1578)

References

Details

(Keywords: intl)

Attachments

(5 files, 3 obsolete files)

Spinoff from bug 392669.

The current toolbarbutton image is not i18n-friendly as it only corresponds to users, using western arabic numerals. This could be fixed by applying a stack element to the the toolbar button with the image on the bottom layer and a text box with the (localizeable!) date at the top layer.
Keywords: intl
Summary: Make today pane toolbarbutton more i18n-friendly → Make Today Pane toolbar button image more i18n-friendly
The bottom bar to open the pane? It should display today's date, not a 31 icon and the rest. Shouldn't need to open the today pane just to see the date.
Duplicate of this bug: 514811
Duplicate of this bug: 534087
The 'Today Pane' button in the toolbar :

-  could perhaps display today's date,

-  but more importantly, should display the title of today's next event (or perhaps even the title of ALL today's events), like ReminderFox  http://reminderfox.mozdev.org/

This issue could perhaps be merged with :
'Today Pane' button in the toolbar should be more dynamic  https://bugzilla.mozilla.org/show_bug.cgi?id=536559

Thanks in advance.
Attached image proposal β€”
I have a patch for this issue that simply replace the static number "31" on the icon with the number of today's date as shown in the screenshot.
I ask for UI review before attaching the patch because I'm not completely sure if you want to solve the issue in this way.
Reading some comments, also in duplicate bugs, and comment #0 as well, I don't know if a label with value |now().day| (more or less like day date in miniday inside todaypane) over the icon is the right way to solve the issue.
Attachment #419242 - Flags: ui-review?(clarkbw)
Simply replacing the static "31" with the actual current date number would be great. Other's may want more functionality (today's tasks showing in a "hover" box and/or the day itself (Sunday) in a localized language, but your proposal is all I seek.

Thanks for taking the time to work on this.

GaryK
Reading about bug 392669 I've understood that the button should basically have a *localizable* date (and not only a "non static" value), therefore I've changed my patch and I've done it as requested in comment #0, so I ask for review.

My doubts are:

1. since the binding for the button ("doubleimage-toolbarbutton") is used only for the todaypane button, I've modified directly the binding. Should  an extended binding specific for today button be created instead?

2. I've modified the image "toolbar-small.png" deleting the "31" from the 21st icon only on the first row (that one used in the button). 
I've open the image with Gimp, covered the "31" with a gradient background and save the image, but since I'm not used to use Gimp to do this kind of work, the image must be checked by someone that knows how to do it.

3. the patch needs testing on Linux and Mac.
In particular for Mac case I've merely copied the modified lines in the lightning.css file, but I noticed that coordinates for the icon inside toolbar-small.png image (pinstripe theme) seem wrong because they are the same of the image in the winstripe theme which has icons with different size (16x16 instead of 24x24). A correct value for the image-region, should be (21st icon):
0, 504, 24, 480.
Since no bug has been reported yet about the look of todaypane button on Mac, I've probably missed something.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Attachment #419463 - Flags: review?(philipp)
Attachment #419463 - Attachment description: patch with a localizable day's date -v1 → [test,short,l10n] patch with a localizable day's date -v1
Comment on attachment 419242 [details]
proposal

this looks good to me
Attachment #419242 - Flags: ui-review?(clarkbw) → ui-review+
Attached image Linux Screenshot β€”
Not quite working on linux, I do hope we find a way to fix this independent of the system font.
Comment on attachment 419463 [details] [diff] [review]
[test,short,l10n] patch with a localizable day's date -v1


> 
>+    // Set up today's date on todaypane button
>+    let todaypaneButton = document.getElementById("calendar-status-todaypane-button");
>+    let dayNumber = calGetString("dateFormat", "day." + now().day + ".number");
>+    document.getAnonymousElementByAttribute(todaypaneButton, "anonid", "day-label").
>+             value = dayNumber;

Codewise, I think we might want to make a separate binding for the today button, that takes care of this code bit as a method, which can be called from both locations.

Since there is already a binding, we can either rename the existing binding, or create a new one that extends the existing one. I'll leave that to you.

r- to take care of the linux issue, and consider the above.
Attachment #419463 - Flags: review?(philipp) → review-
I've fixed the part related to the binding and the method (I've added an extended binding), but have no idea how to properly fix the problem about day's date position on the icon for Windows/Linux.

I've only verified that the position depends on the font type like you said.
On my system I use "Segoe UI" font, but if I add to the class |toolbarbutton-day-text| a font-family property with a value e.g. "Arial", the day's date is misplaced 1px to the top and I get the same identical screenshot you posted for Linux.
In this case I have to change the margin top and bottom to 4px and -4px to get the right position. In other words the following code works on Win XP (and maybe, without the font-family property, it will work on your Linux too) :

+#calendar-status-todaypane-button > stack > .toolbarbutton-day-text {
+  text-align: center;
+  margin-left: 0px;
+  margin-top: 4px;
+  margin-bottom: -4px;
+  font-size: 7pt;
+  font-family: Arial;
+  font-weight: bold;
+  color: #0000FF;
 }
 
What about adding to the font-family property a list of fonts (typical fonts for Windows and Linux) with position verified for that margin values? I know, it doesn't give us the certainty, but maybe it will cover almost all cases.
(In reply to comment #11)

> +  color: #0000FF;

Can we please use a system color here? Dark blue should be readily available in the system color palette.
Attached patch patch with a localizable day's date - v2 (obsolete) β€” β€” Splinter Review
I've tried three font families on Win XP and I've verified individually the text's position on the icon. Since in other part of Calendar code the same fonts (for Linux too) are used, I've added the font-family property with values: Arial, Helvetica and sans-serif.

Could you try this patch on Linux?

About the icon for pinstripe theme, I've read bug 502095 comment #4 and last comments on bug 547654, hence there is an open issue for the todaypane button on Mac platform, so I've added a pinstripe part as a simply copy of the winstripe part but obviously it will not work on Mac.
Attachment #419463 - Attachment is obsolete: true
Attachment #434402 - Flags: review?(philipp)
Comment on attachment 434402 [details] [diff] [review]
patch with a localizable day's date - v2

We'll have to see how the widths work out for days in other languages, but at least on Linux and arabic numerals everything looks fine.


> toolbarbutton[doubleimage="true"] {
>-  -moz-binding: url(chrome://calendar/content/widgets/calendar-widgets.xml#doubleimage-toolbarbutton);
>+  -moz-binding: url(chrome://calendar/content/widgets/calendar-widgets.xml#todaypane-toolbarbutton);
> }
Since you are keeping the doubleimage toolbarbutton binding, I'd suggest you keep this rule as it was and create a new rule for the todaypane-toolbarbutton. You could use a selector like toolbarbutton[type="todaypane"] or maybe just toolbarbutton.todaypane-toolbarbutton and change the type attribute/class accordingly (remove doubleimage=true then).



>+  <binding id="todaypane-toolbarbutton" extends="chrome://calendar/content/widgets/calendar-widgets.xml#doubleimage-toolbarbutton">
>+    <implementation>
>+      <method name="setUpTodayDate">
>+        <body><![CDATA[
>+            let dayNumber = calGetString("dateFormat", "day." + cal.now().day + ".number");
>+            document.getAnonymousElementByAttribute(this, "anonid", "day-label").value = dayNumber;
>+        ]]></body>
>+      </method>
>+    </implementation>
Instead of calling this function in ltnOnLoad, could you call it in the constructor instead? Keep the call in refreshUIBits though.


r- to get a new patch with the comments fixed. We may want to postpone checkin until bug 547654 is fixed and the css for mac needs to be right obviously.
Attachment #434402 - Flags: review?(philipp) → review-
Attached patch patch - v3 (obsolete) β€” β€” Splinter Review
(In reply to comment #14)
> Since you are keeping the doubleimage toolbarbutton binding, I'd suggest you
> keep this rule as it was and create a new rule for the todaypane-toolbarbutton.
> You could use a selector like toolbarbutton[type="todaypane"] or maybe just
> toolbarbutton.todaypane-toolbarbutton and change the type attribute/class
> accordingly (remove doubleimage=true then).

Since the today button is quite different from a normal button, what if I add the element "todaypane-toolbarbutton" directly in the xul instead of a toolbarbutton with a property "type" = "todaypane" or similar?  I'll attach a new patch if it's wrong though.


About the issue of the wrong icon on the Mac (bug 502095 comment #4), I've added a 16x16 size icon to the image "toolbar-smal.png" used for pinstripe theme. The new icon is similar to that one of winstripe theme, but it has a Mac style (I've modified the original 24x24 size icon that should be used for the today pane button). I attach the new image in the next screenshot. Let me know if it has to be done, or positioned, in a different way (maybe someone could do it a bit better).
The problem is that I can't verify if it works on a Mac, neither for the new icon nor for the date position over the icon, so it must be checked by someone with a Mac.

Alternatively, I can attach a patch, ready for checkin, with the base code (for pinstripe too) but only with winstripe theme, and another patch only with the pinstripe theme that should be tested (and adjusted) by someone with a Mac. Just let me know.
Attachment #434402 - Attachment is obsolete: true
Attachment #443304 - Flags: review?(philipp)
I tested your new patch and icon for the Today Pane button on my Mac. Overall it looks great but I have a few nits to pick (see picture for comparison):
1. The date number is not centered in the icon
2. The button should stay highlighted when the Today Pane is visible
3. The text "Today Pane" in the button is raised by one pixel so it does not align in the center of the button or with the rest of the text in the status bar.
Comment on attachment 443304 [details] [diff] [review]
patch - v3

>+todaypane-toolbarbutton {
>+  -moz-binding: url(chrome://calendar/content/widgets/calendar-widgets.xml#todaypane-toolbarbutton);
>+}
The advantage of keeping the element name as "toolbarbutton" is that it also gets all the toolkit rules meant for a normal toolbarbutton. This way we don't have to copy any styles from toolkit.

>diff --git a/calendar/base/themes/pinstripe/images/toolbar-small.png b/calendar/base/themes/pinstripe/images/toolbar-small.png
Your mac image looks fine to me. We'll need to take care of the positioning issues though.

>+#calendar-status-todaypane-button > stack > .toolbarbutton-day-text {
>+  text-align: center;
>+  margin-left: 0px;
>+  margin-top: 4px;
>+  margin-bottom: -4px;
>+  font-size: 7pt;
>+  font-family: Arial, Helvetica, sans-serif;
>+  font-weight: bold;
>+  color: DarkBlue;
> }
Please add an explicit
  background-color: transparent;
You should always color/background-color together to avoid issues when either is inherited from its parent.

r- to fix the nits and since mac is not ready yet. I think the best way to take care of the mac issue is that you provide a patch that fixes the above comments, and then let someone else make the needed changes for the mac theme.


Adil, do you think you could provide Decathlon with the needed CSS rules to make this look OK on mac?
Attachment #443304 - Flags: review?(philipp) → review-
Attached patch patch - v4 β€” β€” Splinter Review
I've written a new patch to take care of the previous comment. For the button, I've used the attribute "todaypane".
I also talked to Alid for the correct look on the Mac. He tried the patch and sent to me the right lightning.css file for the pinstripe theme which I've included in this patch. Thanks a lot Adil :-)
Hence, this patch should work fine for every platform.
Attachment #443304 - Attachment is obsolete: true
Attachment #472124 - Flags: review?(philipp)
Comment on attachment 472124 [details] [diff] [review]
patch - v4

Patch works fine and still looks great on Linux. r=philipp
Thanks for your hard work!

(btw, I see you're not adding yourself as a contributor to files you've changed. If you want, we can file a bug to add your name to the files you've changed in the past?)
Attachment #472124 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/1acfd3ebf2c3>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
I've tested the last Lightning 1.1a1pre with Shredder and the day label is misplaced one pixel to the left, instead it looks fine when the patch is applied on Lightning 1.0b3pre and Lanikai. Since the today button on Shredder has already some problem (bug 554274), should something be done about this issue?

(In reply to comment #20)
> (btw, I see you're not adding yourself as a contributor to files you've
> changed. If you want, we can file a bug to add your name to the files you've
> changed in the past?)

If it's fine for you, I will add my name in the next patches, for old patches too, without a specific bug.
Can we have this patch pushed to comm-1.9.2 as well?
You need to log in before you can comment on or make changes to this bug.