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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: delphine, Assigned: james.zhang)

Tracking

({l12y})

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: LocRun1.2, LocRun1.3)

Attachments

(11 attachments, 2 obsolete attachments)

Created attachment 766188 [details]
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)
status-b2g18: --- → affected
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)

Comment 2

6 years ago
Assigning to Rob since calendar is his domain and Josh is focusing on future themes work.
Flags: needinfo?(jcarpenter) → needinfo?(rmacdonald)

Comment 3

5 years ago
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

Updated

5 years ago
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?
Refer : https://bugzilla.mozilla.org/show_bug.cgi?id=829471#c9
blocking-b2g: leo? → -
Duplicate of this bug: 893462

Comment 6

5 years ago
just to make sure.. bug 893462 is about Day view, not Week view as bug 885946, is it still a duplicate then?

Updated

5 years ago
Blocks: 892075

Comment 7

5 years ago
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

Comment 8

5 years ago
Adding localizers to see if they can find a work around for this.
Created attachment 778825 [details]
Calendar in Czech (cs)

I see it ok for czech (cs). Unagi phone, latest FF OS 1.1.

Comment 10

5 years ago
Created attachment 778843 [details]
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.
Created attachment 779138 [details]
pt-BR Calendar

thanks aryx, here's pt-BR Calendar, looks OK

Updated

5 years ago
Flags: needinfo?(rmacdonald)

Comment 14

5 years ago
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

Comment 15

5 years ago
Created attachment 791244 [details]
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)
Created attachment 811274 [details]
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)

Comment 20

5 years ago
Flagging Eric for a visual adjustment, noting that a few different languages are affected here.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(epang)
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!
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
status-b2g-v1.2: --- → affected
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)

Comment 26

5 years ago
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?

Updated

5 years ago
Blocks: 928174

Comment 27

5 years ago
Created attachment 833101 [details]
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?

Comment 29

5 years ago
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)

Comment 33

5 years ago
Created attachment 8334483 [details]
All Day - Catalan workaround.png

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)

Updated

5 years ago
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
(Assignee)

Comment 38

5 years ago
Created attachment 8339092 [details] [diff] [review]
885946.patch

calendar week view patch base on v1.2 branch

Comment 39

5 years ago
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. :)
(Assignee)

Updated

5 years ago
Attachment #8339092 - Flags: review?(jlal)
(Assignee)

Comment 41

5 years ago
(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!

Comment 47

5 years ago
Created attachment 8341544 [details] [diff] [review]
week_view.patch

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)

Comment 50

5 years ago
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.
Flags: needinfo?(jesse.ji)

Comment 51

5 years ago
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.

Comment 53

5 years ago
Created attachment 8342138 [details] [diff] [review]
week_view.patch

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.
status-b2g18: affected → wontfix
status-b2g-v1.2: affected → wontfix
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.
(Assignee)

Updated

5 years ago
Flags: needinfo?(james.zhang) → needinfo?(jesse.ji)

Comment 57

5 years ago
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)
Duplicate of this bug: 867930
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)
(Assignee)

Comment 61

5 years ago
v1.3 is better for us, thanks.
Flags: needinfo?(james.zhang)
(Assignee)

Updated

5 years ago
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)

Comment 64

5 years ago
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.

Comment 65

5 years ago
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

Comment 67

5 years ago
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: → bug 951071

Comment 70

5 years ago
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)

Updated

5 years ago
status-b2g-v1.3: --- → affected
Whiteboard: LocRun1.2 → LocRun1.2, LocRun1.3

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → WONTFIX

Comment 73

5 years ago
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 → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.