Closed Bug 885946 Opened 9 years ago Closed 8 years ago

[l10n] [fugu][Calendar] "All Day" string in week view doesn't fit correctly in UI

Categories

(Firefox OS Graveyard :: Gaia::Calendar, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g18 wontfix, b2g-v1.2 wontfix, b2g-v1.3 affected)

RESOLVED WONTFIX
1.3 C3/1.4 S3(31jan)
tracking-b2g backlog
Tracking Status
b2g18 --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- affected

People

(Reporter: delphine, Assigned: james.zhang)

References

Details

(Keywords: l12y, Whiteboard: LocRun1.2, LocRun1.3)

Attachments

(11 files, 2 obsolete files)

Attached image Screenshot
Leo device, v1.1 Moz RIL
Gaia   cca61224e6df8e9f7e450d77cb6a8cf91029be64
BuildID 20130621070212
Version 18.0

* Repro:
- Set language to Polish
- Enter Calendar app
- Press on Week view

* Expected:
All strings appear correctly in UI

* Actual:
"All Day" is barely readable. The last letter is truncated, and the word is crossed off by a line (see screenshot)
I’m afraid this is not specific to Polish, we’ve had the same issue with Latin languages and ended up with a sub-optimal translation.  I opened a bug about that  some time ago (I think it was during the work week in Berlin, but not sure…), calling for UX help.

Maybe an icon would be enough? Or could we have a layout allowing to fit more text, e.g. thicker row for the “all day”, or a wider left column to fit “all day” on two lines?
Flags: needinfo?(jcarpenter)
Assigning to Rob since calendar is his domain and Josh is focusing on future themes work.
Flags: needinfo?(jcarpenter) → needinfo?(rmacdonald)
This issue reproduces with multiple languages:
German, Greek, Russian, Turkish, Slovak, Romanian, Croatian, Hungarian, Czech, Spanish and Portuguese (do Brasil).
When selecting the week tab, the all day event text is misaligned and truncated in some cases. 

Environmental Variables
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635
Gaia: 61453c4d32beb15a33ec91b2e740e96e5ce45759
Platform Version: 18.1
Keywords: l12y
Summary: [l10n] [Calendar] In Polish, "All Day" string in week view doesn't fit correctly in UI → [l10n] [Calendar] "All Day" string in week view doesn't fit correctly in UI
blocking-b2g: --- → leo?
just to make sure.. bug 893462 is about Day view, not Week view as bug 885946, is it still a duplicate then?
Blocks: 892075
Also Reproduces in Serbian language.
Environmental Variables
Build ID: 20130715070218
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8
Gaia: 55ed5e08a2250ea2d3571fff860c39e66fabed14
Platform Version: 18.1
Adding localizers to see if they can find a work around for this.
Attached image Calendar in Czech (cs)
I see it ok for czech (cs). Unagi phone, latest FF OS 1.1.
Attached image Croatian calendar
Croatian (hr) string fits within cell boundaries. However, it's partially striked through, which is annoying because it makes it harder to read.
If I knew how to capture screen shots I would show pt-BR (Brazilian Portuguese) is also OK
Power button + Home button takes screenshot and saves it into the 'screenshots' folder on the memory card.
Attached image pt-BR Calendar
thanks aryx, here's pt-BR Calendar, looks OK
Flags: needinfo?(rmacdonald)
Also reproduces in Catalan language

Environmental Variables
Build ID: 20130725070209
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/142cb317a178
Gaia: b1945e7eca58c9176d91998390871864f5e8eb3e
Platform Version: 18.1
Attached image Romanian calendar
Confirmed for Romanian calendar.
This also reproduces in Italian, on Buri device, on today's latest v1.1 build (Moz RIL)
Gaia   f3b30b19d80240c9cc2aa2002398c902fd1d6375
BuildID 20130926214845
Version 18.1

This was fixed for example by Russian localizer in Bug 891483 so adding Italian localizer here to see if he can fix this. Otherwise we will ask for a UX solution
Blocks: 920639
Flags: needinfo?(francesco.lodolo)
This can't be fixed on the string side. 

The shortest thing I can think of is "giorno int.", which is still too long (int. is displayed under the event's name).
Flags: needinfo?(francesco.lodolo)
Attached image calendar.png
I pushed "Giorno int.", at least if we can't come up with a UX solution this is definitely less broken.
Asking for some UX help here
thanks!
Flags: needinfo?(firefoxos-ux-bugzilla)
Flagging Eric for a visual adjustment, noting that a few different languages are affected here.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(epang)
Attached image calendar spacing.png
Here's my proposal to help fix the truncation issue occurring with some of the localizations.  When the string can't fit on one line we should allow it to flow into a second line.

This includes widening the columns.
Time column: Widened to 2.8 rem, all times are vertically centered aligned
Day Columns: Widened to 7.8 rem

Let me know if you have any questions about this proposal, thanks!
Flags: needinfo?(epang)
My first thought would be to get rid completely of the vertical centering, this would make things better if the single word is longer then the available space.

If I write "agreem..." you probably recognize the word, would you if I write "...greemen..."?
Issue reproduces on v1.2 as well with multiple languages (including those that are unique to v1.2)

Environmental Variables
Device: Buri v1.2 COM RIL
Build ID: 20131021004006
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/d585fe28cd55
Gaia: 1fd315337d8ae891c3024e4c682c4c50797ea40e
Platform Version: 26.0a2
RIL Version: 01.02.00.019.082 
Firmware Version: US_20131015
Whiteboard: LocRun1.2
ni Eric Pang again to see if we can fiw this for 1.2
Flags: needinfo?(epang)
(In reply to delphine from comment #24)
> ni Eric Pang again to see if we can fiw this for 1.2

Redirecting to Stephany, is it possible to get this change into 1.2?  I think it may be too late :(.
Flags: needinfo?(epang) → needinfo?(swilkes)
Sorry - I am buried in flags. The best thing to do, if we want something to get into a certain release, is just to nominate it (as koi?) and that way we'll definitely review it in triage. Is there a fix in a patch somewhere that I should reference, or do we just want this to block 1.2?
Blocks: 928174
Attached image Screenshot
This issue is present in Bulgarian as well. Attaching a screenshot to represent the issue.
(In reply to Eric Pang [:epang] from comment #21)
> Created attachment 813300 [details]
> calendar spacing.png
> 
> Here's my proposal to help fix the truncation issue occurring with some of
> the localizations.  When the string can't fit on one line we should allow it
> to flow into a second line.
> 
> This includes widening the columns.
> Time column: Widened to 2.8 rem, all times are vertically centered aligned
> Day Columns: Widened to 7.8 rem
> 
> Let me know if you have any questions about this proposal, thanks!

I think your proposal makes sense, and I’d really like this issue to be solved (it’s been 5 months now). Let’s see if we can work on it for koi.
blocking-b2g: - → koi?
Eric's suggestion is moving and koi? is here so removing the ni? flag to me. Thanks, all!
Flags: needinfo?(swilkes)
We've shipped with this in 1.1. Why is it essential to fix for 1.2?

Given how late we are in 1.2 I am concerned about taking it for 1.2
delphine, flod: do you think the proposal in comment #21 can be made to work for all locales? 

I'm not sure that we can get this into 1.2, but if we can settled on a solution then we can get to work fixing it on master and proceed from there once we have a fix.
Flags: needinfo?(lebedel.delphine)
Flags: needinfo?(francesco.lodolo)
I explained my only concern in comment 22 (better get rid of the vertical centering). What happens if a single word is longer than the available space? Will it be hyphenated?

But honestly, we're at a point where anything can improve the current situation ;-)
Flags: needinfo?(francesco.lodolo)
As a workaround, Catalan decided to use a lowercase "tot el dia" instead of "Tot el dia". From "T" to "t", a couple of pixels can be saved so that the string fits in the available space.
It still looks bad, but not as much as a truncated string.
I sympathize with longer locales with no chance to workaround the issue in this manner.
Dylan

Can your team please take a look for 1.3?
blocking-b2g: koi? → 1.3?
Flags: needinfo?(doliver)
triage: we will take this on for 1.3
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(doliver)
Clearing my needinfo, nothing to add to all this
Flags: needinfo?(lebedel.delphine)
Duplicate of this bug: 942777
Summary: [l10n] [Calendar] "All Day" string in week view doesn't fit correctly in UI → [l10n] [fugu][Calendar] "All Day" string in week view doesn't fit correctly in UI
Attached patch 885946.patch (obsolete) — Splinter Review
calendar week view patch base on v1.2 branch
https://bug885946.bugzilla.mozilla.org/attachment.cgi?id=8339092

Works fine for me. Could anyone review it?
Hi Jesse,

You could find the owners or peers of Calendar App to review the patch.
They are listed in https://wiki.mozilla.org/Modules/All#System(please see the Calendar part).

BTW, I think you already knew how to send a pull request to the gaia repo on GitHub and set a review for it on Bugzilla, right?

Thanks for the patch. :)
Attachment #8339092 - Flags: review?(jlal)
(In reply to Evan Tseng [:evanxd] from comment #40)
> Hi Jesse,
> 
> You could find the owners or peers of Calendar App to review the patch.
> They are listed in https://wiki.mozilla.org/Modules/All#System(please see
> the Calendar part).
> 
> BTW, I think you already knew how to send a pull request to the gaia repo on
> GitHub and set a review for it on Bugzilla, right?
> 
> Thanks for the patch. :)
I'll ask James Lal to review this patch, Thank you!
Hi James,

Please refer to the "Submitting a patch" part of the doc in https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Hacking to send a pull request for a review.

Thanks.
Assigning to James Zhang -- thanks for the patch!
Assignee: nobody → james.zhang
Comment on attachment 8339092 [details] [diff] [review]
885946.patch

Kevin - you mind taking a look this is your magic from back in Berlin :)
Attachment #8339092 - Flags: review?(jlal) → review?(kgrandon)
Comment on attachment 8339092 [details] [diff] [review]
885946.patch

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

Hi James, Jesse,

Thank you for the patch. There are a few issues I'd like to address here before giving R+. My main concern is about adding another DOM element - so I'd like to understand why it's necessary instead of migrating the styles to the existing one. (It seems like we removed all of the styles for the all-day element).

Also - it may be easier for you guys to manage branches on a fork of the github repository. Feel free to ask Evan or myself if you have any trouble doing that.

Thanks!

::: apps/calendar/js/templates/week.js
@@ +31,4 @@
>      frame: function() {
>        return '<section class="sticky">' +
>            '<section class="children">' +
> +            '<span class="all-day-wrap"><span class="all-day">' +

It appears that we remove the all-day class from week_view.css. Could we simply change the styles on all-day instead of adding another wrapping span?

::: apps/calendar/style/week_view.css
@@ +86,4 @@
>    z-index: 11;
>  }
>  
> +/*#week-view .sticky .all-day {

Please do not submit code with comments. Simply remove this block, or update the styles.

@@ +95,5 @@
>    left: -1rem;
>    font-size: 1.3rem;
> +}*/
> +#week-view .sticky .all-day-wrap{
> +	display: block;

Please follow our conventions of two spaces (instead of a tab).
Attachment #8339092 - Flags: review?(kgrandon)
Also - once issues are addressed, please mark me as Review? again. Thanks!
Attached patch week_view.patch (obsolete) — Splinter Review
Sorry for mistakes in my patch. Upload a new patch here. I would say that:

1. It's hard to fix the bug by simply change the styles. Adding a wrap block named 'all-day-wrap' looks nice,at least not so bad. Please tell me if u have 
a better solution without adding a wrap block.

2. The new patch removes comment lines and replace all tabs with two spaces.

3. According to the last weekly meeting(Mozilla-Spreadtrum), we're suffering from slow network speed. We would rather submit our patches than submit a pull request.
Attachment #8339092 - Attachment is obsolete: true
Comment on attachment 8341544 [details] [diff] [review]
week_view.patch

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

Marking myself as reviewer - will get to this soon.
Attachment #8341544 - Flags: review?(kgrandon)
Comment on attachment 8341544 [details] [diff] [review]
week_view.patch

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

Hi Jesse -

I applied your patch, but I was not seeing the desired behavior in this spec as attached to this bug: https://bug885946.bugzilla.mozilla.org/attachment.cgi?id=813300

A little bit more text fit, but my concern that this is not flexible enough for all locales. Can you confirm if your implementation matches the desired spec? Are you able to attach a screenshot of your fix? Thanks!
Attachment #8341544 - Flags: review?(kgrandon)
Flags: needinfo?(jesse.ji)
Attached file week_view_locales.zip
Works fine for me on latest v1.2 branch. I did not test it on v1.1 or v1.3.
Flags: needinfo?(jesse.ji)
Maybe we need another patch for v1.3. I have not yet touched v1.3.
(In reply to Jesse from comment #50)
> Created attachment 8342100 [details]
> week_view_locales.zip
> 
> Works fine for me on latest v1.2 branch. I did not test it on v1.1 or v1.3.

Eric's spec shows that the text may wrap to a second line if it's long - that's currently not happening.

If it's not required to happen - we should get the longest string and test with that to make sure we don't need to do that.
Attached patch week_view.patchSplinter Review
Add 'white-space: nowrap;' style here?
Attachment #8341544 - Attachment is obsolete: true
(In reply to Jesse from comment #51)
> Maybe we need another patch for v1.3. I have not yet touched v1.3.

Please just focus on 1.3. This bug is not a blocker for 1.2 and it's too late to land on that branch.
James (or Jesse), can you provide an update on the status of this bug?
Flags: needinfo?(james.zhang)
It looks like a patch was updated, but it still does not match Eric's spec. I'm ok going with this patch should it have enough room to contain the longest expected string, but I don't think that's the case.
Flags: needinfo?(james.zhang) → needinfo?(jesse.ji)
I don't know actually what does Eric's spec requires how a long string displays in a fixed-width container.

Should it render an ellipsis to represent clipped text or just break words in two(or more) lines?
Flags: needinfo?(jesse.ji) → needinfo?(kgrandon)
(In reply to Jesse from comment #57)
> I don't know actually what does Eric's spec requires how a long string
> displays in a fixed-width container.
> 
> Should it render an ellipsis to represent clipped text or just break words
> in two(or more) lines?

You can find his spec here: https://bug885946.bugzilla.mozilla.org/attachment.cgi?id=813300

His spec breaks the words into two lines. We may be able to present an alternative if you think there is a better option.
Flags: needinfo?(kgrandon)
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Jesse, are you still working on this bug? We are targeting the 1.3+ fixes to be complete and landed by 17 Jan.
Target Milestone: 1.3 C1/1.4 S1(20dec) → 1.3 C2/1.4 S2(17jan)
Flags: needinfo?(jesse.ji)
Flags: needinfo?(james.zhang)
v1.3 is better for us, thanks.
Flags: needinfo?(james.zhang)
Attachment #8342138 - Flags: review?(ehung)
We've shipped multiple releases with this bug previously, so we probably can punt this.
blocking-b2g: 1.3+ → 1.3?
Agreed this won't truly block 1.3 but I'd like to see if we can the review done and uplift if this one is ready this week.

Adding :gaye in case he has time to take a look at this today.
Flags: needinfo?(jesse.ji)
Flags: needinfo?(firefoxos-ux-bugzilla)
Suggestion: Why not completely getting rid of the "All day" string from the Week view? I do not think it would cause confusion to the user. Events showing in the upper part are "All day" events. Otherwise, non-"All day" events will appear in the corresponding hour slot.
Incidentally, this is how it works in Android's Calendar.
I am not clear on the nature of the UX need on the flag. A string change should be filed as a separate bug, and should be in the backlog of Adam Rogers and the Productivity team if they think it's a good idea. Eric provided his spec, and the goal is to implement to that spec. Please re-flag if there's an issue.
Flags: needinfo?(firefoxos-ux-bugzilla)
Adding to backlog per comment 64 and decision during triage.
blocking-b2g: 1.3? → backlog
Thanks, Preeti!
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
Duplicate of this bug: 830710
It looks like this bug will be obsoleted by the visual design changes that are coming in bug 951071. (Details to come, but the short story is that the "All Day" string will be replaced by an icon.)
See Also: → 951071
Comment on attachment 8342138 [details] [diff] [review]
week_view.patch

I'm not the one who can review this app's patch. redirect to James Lal.
Attachment #8342138 - Flags: review?(ehung) → review?(jlal)
Whiteboard: LocRun1.2 → LocRun1.2, LocRun1.3
Blocks: 967050
(In reply to Dylan Oliver [:doliver] from comment #69)
> It looks like this bug will be obsoleted by the visual design changes that
> are coming in bug 951071. (Details to come, but the short story is that the
> "All Day" string will be replaced by an icon.)

So would this be a "wontfix" if we're going to replace this in the visual design? Please advise if we should be working on this...thanks! :)
Yes, now that we've committed to bug 951071 for 1.4, I agree this is a wontfix.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
We are working on the visual refresh schedule during the joint PM-UX work week in SF right now, but here's my take. If we are definitely, 100% for sure going to replace this text with an icon and it will definitely make it into 1.4, it doesn't make sense to do an interim fix, or to block on this for 1.3. L10N may disagree, though, or have different blocking criteria; they would need to accept this truncation in 1.3.

I've also flagged Patryk to see if he can add any info here on the icon schedule for this one.
Attachment #8342138 - Flags: review?(jlal)
Duplicate of this bug: 841421
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.