1059017 - [Calendar] RTL settings will breaks all views

Status: VERIFIED FIXED

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

Description

[Tony Chung [:tchung]]

Attachment: screenshot 1

[Blocking Requested - why for this release]:

If you set an RTL locale, you can see the times are messing up the calendar items.

Screenshots to confirm.

Repro:
1) install 2.1 nightly on Flame
2) Swtich to a RTL locale in settings
3) Launch calendar and have a few items in there with times and events.
4) Verify day, week, and monty view has times that is misplaced

Expected:
- times on the right

Actual:
- times on both right and left, overlaps text

Comment 1

[Tony Chung [:tchung]]

Attachment: screenshot 2

Comment 2

[Johan Lorenzo [:jlorenzo]]

I'm not sure we should block on a RTL bug for 2.1. Do you confirm Stephany?

Comment 3

[Tony Chung [:tchung]]

Per ux, i'm told that RTL isnt gated on 2.1.   unnom'ing

Comment 4

[Stephany Wilkes]

We should not block on this. However, we should fix and uplift all of the RTL improvements that we can.

Comment 5

[Stephany Wilkes]

[Blocking Requested - why for this release]:

Although actually - this is different. :) There's RTL support, and then there is making an app completely unusable, and that's actually what this appears to be. The user cannot use their calendar, when in a language setting we provide. This seems to be a blocker on our basic criteria, not RTL specific criteria. 

This should be filed and assigned to a dev on the Productivity team to fix. I am wondering if l10n.js may be affecting this.

Comment 6

[Tony Chung [:tchung]]

sounds good.  lets leave it nom'd , and this is already in the productivity team triage radar.

Comment 7

[Tony Chung [:tchung]]

was told that RTL is not committed for 2.1.  un'noming.

Comment 8

[Stephany Wilkes]

If the calendar is unusable in a shipping language, then it's not necessarily just an RTL issue. Have we tested in other languages, etc?

Comment 9

[Stephany Wilkes]

Tony, is there a plan to ensure that this does not also happen in non-RTL languages we support?

Comment 10

[Tony Chung [:tchung]]

(In reply to Stephany Wilkes from comment #9)
> Tony, is there a plan to ensure that this does not also happen in non-RTL
> languages we support?

Hey Stephany, not sure i understand your question.  for Non-RTL locales, we are actively testing english, (and l10n tests european and asian locales), so things would be caught and filed right away already.  Is there something i'm missing?

Comment 11

[Stephany Wilkes]

We know that parts of the calendar are unusable in at least one, known shipping language (Arabic in this particular bug, but there may be others).  I'm asking if we know that this does not happen in other shipping languages and, if so, how we know that -- in terms of display, not language, issues. Often, for example, we'll notice similar layout issues in Russian as we do in Arabic, though Russian is not an RTL language. 

Further, if an app is not usable in a particular language, how do we determine whether or not to actually allow/activate that language in the settings or not? Is there a particular QA threshold for minimum functionality in a shipping language? If there is, we ought to know if we are meeting it for Arabic, or not, and if not, we should not allow users to choose Arabic in the language settings. 

Flagging Clint to see if he can advise further.

Comment 12

[cmtalbert]

(In reply to Stephany Wilkes from comment #11)
> We know that parts of the calendar are unusable in at least one, known
> shipping language (Arabic in this particular bug, but there may be others). 
> I'm asking if we know that this does not happen in other shipping languages
> and, if so, how we know that -- in terms of display, not language, issues.
> Often, for example, we'll notice similar layout issues in Russian as we do
> in Arabic, though Russian is not an RTL language. 
>
We run L10N testing to cover a smattering of languages - usually that testing is compressed due to time constraints around trying to do the full regression tests. The testing that is done as part of those tests is exactly this kind of testing - do the applications work in the languages and are the languages fully translated (i.e. no english strings popping up in the UI).

If there are languages that you know are fragile and are being shipped, then you should coordinate with Tony and Delphine to ensure they are on the short list for our L10N testing run.
 
> Further, if an app is not usable in a particular language, how do we
> determine whether or not to actually allow/activate that language in the
> settings or not? Is there a particular QA threshold for minimum
> functionality in a shipping language? If there is, we ought to know if we
> are meeting it for Arabic, or not, and if not, we should not allow users to
> choose Arabic in the language settings. 
>
I would say if any of our core apps have significant usability or functionality problems when running in this language, then we should remove the language from the default FTU language picker so that J. Random User doesn't wind up with a broken phone.  

Note that I said *FTU*. I would still want the ability for J Random User to go into settings and select the language (perhaps with a little warning message saying something that there are likely bugs in this language). There are a couple of reasons for this:
1. Making it easier to test these kinds of issues
2. Enabling localizers to work on languages that we have not yet deemed "ready to release" (this is probably most important).
3. To foster local communities to submit patches - maybe Arabic speakers have a customary way they like to look at calendars, and as such, they might be the best people to submit a patch for this issue, I would want them to be able to see the problem, write the patch, and test it without having to go to huge lengths to rebuild the phone and add another language etc.

> Flagging Clint to see if he can advise further.
That's my advice. Let me know if you have questions.
I

Comment 13

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Link to Github pull-request

WIP.

Comment 14

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

Decided during triage this morning that this would block 2.2

Comment 15

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8546759 [details] [review]
Link to Github pull-request

This PR is pretty ready *phew* \o/
Please review?
Thanks!
:)

Comment 16

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #15)
> Comment on attachment 8546759 [details] [review]
> Link to Github pull-request
> 
> This PR is pretty ready *phew* \o/
> Please review?
> Thanks!
> :)

PS: You may notice event name under month view out of its position + some black regions that becomes normal when refreshed (e.g. clicking on another day in), that is related to Bug 1121748.
Thanks!

Comment 17

[bhavana bajaj [:bajaj]]

Blocking per RTL triage for 2.2

Comment 18

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8546759 [details] [review]
Link to Github pull-request

Cancelling review flags until I finish making changes per the new calendar spec.

Comment 19

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: PR to Github

Another PR to keep the other aside which contained the reversed swipes (we'll need it for future ux spec)
For now, how dos that look? :)
Thanks!

Comment 21

[Stephany Wilkes]

Comment on attachment 8554602 [details] [review]
PR to Github

Reassigning ui-review for calendar to Harly.

Comment 22

[Dylan Oliver [:doliver]]

RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)

Comment 23

[Harly Hsu[:harly]]

Attachment: Calendar RTL Review.pdf

Attached is a quick review of the RTL calendar. After installing the patch, I am unable to see events in week or day view. For month view, most are completed according to spec, but the name of the event isn't aligned correctly. Other than that, when first initiate calendar app, it shows a black screen with "other month" in the event list. I will give a r- for now. Thanks

Comment 24

[Fabien Cazenave [:kaze]]

Thanks for the extensive feedback Harly, your PDF really helps. Looking at this right now.

(In reply to Harly Hsu from comment #23)
> Created attachment 8555008 [details]
> Other than that, when first initiate calendar app, it shows a
> black screen with "other month" in the event list.

This is not related to Ahmed’s patch. It is caused by to bug 1121748, which affects many RTL-related features.

Comment 25

[Fabien Cazenave [:kaze]]

Miller: following our discussion on IRC, feel free to steal this bug. Thanks for your help!

Comment 26

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27788

doing a new PR containing Ahmed changes + some small tweaks that I did since he is busy working on other bugs.

Comment 27

[Harly Hsu[:harly]]

Attachment: 2015-01-30-10-22-37.png

Hi Miller,
I've flash the patch on my Flame, but when opening the calendar app, it will just show a black screen and doesn't seem to be working. Also the app icon also become a rocket icon like the attached screenshot. Is there something wrong with my flame or is because of the patch?

Comment 28

[Miller Medeiros [:millermedeiros]]

Harly, the latest build is indeed not working as expected (month view displays the black screen + "other month" and the event title is not aligned properly like you described on your first review).. I was testing with a build from a few weeks ago and it was working fine. There is probably something on Gecko that broke in the past month (maybe Bug 1121748)

I never seen the "rocket icon" and my patch doesn't change the app manifest, so it was not supposed to affect that.

Comment 29

[Ahmed Nefzaoui [:Nefzaoui]]

Guys, please note that what you're seeing of black screens and "other month" issues is not related to this patch or bug.
This is caused by bug 1121748, and it has been backed out for 2.2. The current 2.2 flame builds are clean and safe to try on, and I just tried the patch on v2.2 of gaia and gecko and I confirm it's working as expected.
1121748 is going to have further investigations for master.
So we if those are all the issues you're seeing, we can still safely land and then uplift :)
Thanks!

Comment 30

[Stephany Wilkes]

Thank you, Ahmed. That is what I'm finding as well.

Comment 31

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8556616 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27788

Looks good. I would like to see some marionette tests but no need to block on that.

Comment 32

[Gareth Aye [:gaye] (back from PTO)]

I do have a couple nits on GH though! ^^

Comment 34

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8556616 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27788

as discussed today during our weekly meeting we are going to land it and maybe do follow up bugs if anything else needs to be fixed. There are too many duplicates being reported for RTL and the sooner we land this patch the better (more eyeballs, more time to fix issues).

important to note that latest master builds have some issues with RTL that are not related to this patch (mostly gecko problems) so you need to test this against v2.2 to get the desired output.

Comment 35

[Miller Medeiros [:millermedeiros]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/73435aa5dcb4b2d4eb3d692dd421b448467641a3

Comment 36

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8556616 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27788

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): none
[User impact] if declined: no RTL support for calendar (most views are broken, unusable)
[Testing completed]: manual test on flame, by 3 engineers
[Risk to taking this patch] (and alternatives if risky): should be low risky since we tested it against v2.2 builds during development
[String changes made]: none

Comment 37

[Coler]

This issue has been verified successfully on Flame 3.0.
Reproduce rate:0/5
Attachment:Verify_RTL_Calendar_Month.png,Verify_RTL_Calendar_Week.png,Verify_RTL_Calendar_Day.png

Flame 3.0 build:

Build ID               20150204160237
Gaia Revision          b9607aef7debbde09a8db801ce4d021b8262e7f3
Gaia Date              2015-02-04 16:29:57
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/3cda3997f45d
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150204.192122
Firmware Date          Wed Feb  4 19:21:34 EST 2015
Bootloader             L1TC000118D0

Comment 38

[Coler]

Attachment: Verify_RTL_Calendar_Month.png

Comment 39

[Coler]

Attachment: Verify_RTL_Calendar_Week.png

Comment 40

[Coler]

Attachment: Verify_RTL_Calendar_Day.png

Comment 41

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/7fdd150994f435b4be8c9fcdd8cf23935b5c3062

Comment 44

[Coler]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15240/

Comment 45

[Coler]

This issue has been verified successfully on Flame2.2.
Attachment:Verify_Calendar.png.

Flame 2.2 build:

Build ID               20150214002504
Gaia Revision          ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gaia Date              2015-02-13 05:27:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6de30e6bbc84
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150214.044811
Firmware Date          Sat Feb 14 04:48:22 EST 2015
Bootloader             L1TC000118D0

Comment 46

[Coler]

Attachment: Verify_Calendar.png