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

Status: RESOLVED WONTFIX

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

Description

[Delphine Lebédel [:delphine - Use needinfo]]

Attachment: 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)

Comment 1

[Fabien Cazenave [:kaze]]

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?

Comment 2

[Stephany Wilkes]

Assigning to Rob since calendar is his domain and Josh is focusing on future themes work.

Comment 3

[mdaniloff]

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

Comment 4

[bhavana bajaj [:bajaj]]

Refer : https://bugzilla.mozilla.org/show_bug.cgi?id=829471#c9

Comment 6

[Natalya Kot [:nkot]]

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

Comment 7

[Shilpa Suresh]

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

[Mila Davydova]

Adding localizers to see if they can find a work around for this.

Comment 9

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Calendar in Czech (cs)

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

Comment 10

[Sasa Tekovic]

Attachment: 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.

Comment 11

[Fernando Pereira Silveira]

If I knew how to capture screen shots I would show pt-BR (Brazilian Portuguese) is also OK

Comment 12

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

Power button + Home button takes screenshot and saves it into the 'screenshots' folder on the memory card.

Comment 13

[Fernando Pereira Silveira]

Attachment: pt-BR Calendar

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

Comment 14

[mdaniloff]

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

[Raul Nicolae Malea]

Attachment: Romanian calendar

Confirmed for Romanian calendar.

Comment 16

[Delphine Lebédel [:delphine - Use needinfo]]

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

Comment 17

[Francesco Lodolo [:flod]]

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).

Comment 18

[Francesco Lodolo [:flod]]

Attachment: calendar.png

I pushed "Giorno int.", at least if we can't come up with a UX solution this is definitely less broken.

Comment 19

[Delphine Lebédel [:delphine - Use needinfo]]

Asking for some UX help here
thanks!

Comment 20

[Stephany Wilkes]

Flagging Eric for a visual adjustment, noting that a few different languages are affected here.

Comment 21

[Eric Pang [:epang] UX]

Attachment: 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!

Comment 22

[Francesco Lodolo [:flod]]

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..."?

Comment 23

[Peter Bylenga [:PBylenga]]

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

Comment 24

[Delphine Lebédel [:delphine - Use needinfo]]

ni Eric Pang again to see if we can fiw this for 1.2

Comment 25

[Eric Pang [:epang] UX]

(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 :(.

Comment 26

[Stephany Wilkes]

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?

Comment 27

[Tony Nguyen]

Attachment: Screenshot

This issue is present in Bulgarian as well. Attaching a screenshot to represent the issue.

Comment 28

[Fabien Cazenave [:kaze]]

(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.

Comment 29

[Stephany Wilkes]

Eric's suggestion is moving and koi? is here so removing the ni? flag to me. Thanks, all!

Comment 30

[Preeti Raghunath(:Preeti)]

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

Comment 31

[Dylan Oliver [:doliver]]

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.

Comment 32

[Francesco Lodolo [:flod]]

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 ;-)

Comment 33

[Jordi Serratosa]

Attachment: 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.

Comment 34

[Preeti Raghunath(:Preeti)]

Dylan

Can your team please take a look for 1.3?

Comment 35

[Dylan Oliver [:doliver]]

triage: we will take this on for 1.3

Comment 36

[Delphine Lebédel [:delphine - Use needinfo]]

Clearing my needinfo, nothing to add to all this

Comment 38

[James Zhang (Spreadtrum)]

Attachment: 885946.patch

calendar week view patch base on v1.2 branch

Comment 39

[Jesse]

https://bug885946.bugzilla.mozilla.org/attachment.cgi?id=8339092

Works fine for me. Could anyone review it?

Comment 40

[Evan Tseng [:evanxd]]

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. :)

Comment 41

[James Zhang (Spreadtrum)]

(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!

Comment 42

[Evan Tseng [:evanxd]]

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.

Comment 43

[Dylan Oliver [:doliver]]

Assigning to James Zhang -- thanks for the patch!

Comment 44

[James Lal [:lightsofapollo]]

Comment on attachment 8339092 [details] [diff] [review]
885946.patch

Kevin - you mind taking a look this is your magic from back in Berlin :)

Comment 45

[Kevin Grandon :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).

Comment 46

[Kevin Grandon :kgrandon]

Also - once issues are addressed, please mark me as Review? again. Thanks!

Comment 47

[Jesse]

Attachment: 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.

Comment 48

[Kevin Grandon :kgrandon]

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.

Comment 49

[Kevin Grandon :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!

Comment 50

[Jesse]

Attachment: week_view_locales.zip

Works fine for me on latest v1.2 branch. I did not test it on v1.1 or v1.3.

Comment 51

[Jesse]

Maybe we need another patch for v1.3. I have not yet touched v1.3.

Comment 52

[Kevin Grandon :kgrandon]

(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

[Jesse]

Attachment: week_view.patch

Add 'white-space: nowrap;' style here?

Comment 54

[Dylan Oliver [:doliver]]

(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.

Comment 55

[Dylan Oliver [:doliver]]

James (or Jesse), can you provide an update on the status of this bug?

Comment 56

[Kevin Grandon :kgrandon]

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.

Comment 57

[Jesse]

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?

Comment 58

[Kevin Grandon :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.

Comment 60

[Dylan Oliver [:doliver]]

Jesse, are you still working on this bug? We are targeting the 1.3+ fixes to be complete and landed by 17 Jan.

Comment 61

[James Zhang (Spreadtrum)]

v1.3 is better for us, thanks.

Comment 62

[Jason Smith [:jsmith]]

We've shipped multiple releases with this bug previously, so we probably can punt this.

Comment 63

[Dylan Oliver [:doliver]]

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.

Comment 64

[Jordi Serratosa]

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

[Stephany Wilkes]

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.

Comment 66

[Preeti Raghunath(:Preeti)]

Adding to backlog per comment 64 and decision during triage.

Comment 67

[Stephany Wilkes]

Thanks, Preeti!

Comment 69

[Dylan Oliver [:doliver]]

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.)

Comment 70

[Evelyn Hung]

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.

Comment 71

[Candice Serran (:cserran)]

(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! :)

Comment 72

[Dylan Oliver [:doliver]]

Yes, now that we've committed to bug 951071 for 1.4, I agree this is a wontfix.

Comment 73

[Stephany Wilkes]

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.