Closed Bug 1059017 Opened 5 years ago Closed 5 years ago

[Calendar] RTL settings will breaks all views

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S5 (6feb)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: tchung, Assigned: mmedeiros)

References

Details

(Whiteboard: [2.1-Arabic-RTL-bug-bash])

Attachments

(8 files, 3 obsolete files)

Attached image 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
Attached image screenshot 2
I'm not sure we should block on a RTL bug for 2.1. Do you confirm Stephany?
Blocks: gaia-rtl
Flags: needinfo?(swilkes)
Whiteboard: [2.1-Arabic-RTL-bug-bash]
Per ux, i'm told that RTL isnt gated on 2.1.   unnom'ing
blocking-b2g: 2.1? → ---
We should not block on this. However, we should fix and uplift all of the RTL improvements that we can.
Flags: needinfo?(swilkes)
[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.
blocking-b2g: --- → 2.1?
Flags: needinfo?(tchung)
sounds good.  lets leave it nom'd , and this is already in the productivity team triage radar.
Flags: needinfo?(tchung)
was told that RTL is not committed for 2.1.  un'noming.
blocking-b2g: 2.1? → ---
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?
Tony, is there a plan to ensure that this does not also happen in non-RTL languages we support?
Flags: needinfo?(tchung)
Blocks: calendar-rtl
No longer blocks: gaia-rtl
(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?
Flags: needinfo?(tchung)
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.
Flags: needinfo?(ctalbert)
(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
Flags: needinfo?(ctalbert)
Attached file Link to Github pull-request (obsolete) —
WIP.
Assignee: nobody → nefzaoui
Status: NEW → ASSIGNED
Decided during triage this morning that this would block 2.2
blocking-b2g: --- → 2.2?
Comment on attachment 8546759 [details] [review]
Link to Github pull-request

This PR is pretty ready *phew* \o/
Please review?
Thanks!
:)
Attachment #8546759 - Flags: ui-review?(swilkes)
Attachment #8546759 - Flags: review?(mmedeiros)
(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!
Priority: -- → P1
Blocking per RTL triage for 2.2
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8546759 [details] [review]
Link to Github pull-request

Cancelling review flags until I finish making changes per the new calendar spec.
Attachment #8546759 - Flags: ui-review?(swilkes)
Attachment #8546759 - Flags: review?(mmedeiros)
Attached file PR to Github (obsolete) —
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!
Attachment #8546759 - Attachment is obsolete: true
Attachment #8554602 - Flags: ui-review?(swilkes)
Attachment #8554602 - Flags: review?(mmedeiros)
Duplicate of this bug: 1125745
Comment on attachment 8554602 [details] [review]
PR to Github

Reassigning ui-review for calendar to Harly.
Attachment #8554602 - Flags: ui-review?(swilkes) → ui-review?(hhsu)
RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)
blocking-b2g: 2.2+ → ---
feature-b2g: --- → 2.2+
Attached file Calendar RTL Review.pdf (obsolete) —
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
Attachment #8554602 - Flags: ui-review?(hhsu) → ui-review-
Blocks: 1125747
Blocks: 1125749
Blocks: 1126202
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.
Miller: following our discussion on IRC, feel free to steal this bug. Thanks for your help!
Assignee: nefzaoui → mmedeiros
doing a new PR containing Ahmed changes + some small tweaks that I did since he is busy working on other bugs.
Attachment #8554602 - Attachment is obsolete: true
Attachment #8555008 - Attachment is obsolete: true
Attachment #8554602 - Flags: review?(mmedeiros)
Attachment #8556616 - Flags: ui-review?(hhsu)
Attachment #8556616 - Flags: review?(gaye)
See Also: → 1127770
Attached image 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?
Flags: needinfo?(mmedeiros)
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.
Flags: needinfo?(mmedeiros)
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!
Thank you, Ahmed. That is what I'm finding as well.
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.
Attachment #8556616 - Flags: review?(gaye) → review+
I do have a couple nits on GH though! ^^
Blocks: 1128862
Blocks: 1044285
Blocks: 1115109
Blocks: 1127142
Blocks: 1128445
Blocks: 1128447
Duplicate of this bug: 1128862
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.
Attachment #8556616 - Flags: ui-review?(hhsu)
landed on master: https://github.com/mozilla-b2g/gaia/commit/73435aa5dcb4b2d4eb3d692dd421b448467641a3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8556616 - Flags: approval-gaia-v2.2?
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
QA Whiteboard: [MGSEI-Triage+]
Attachment #8556616 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Duplicate of this bug: 1128445
Duplicate of this bug: 1127142
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15240/
Flags: in-moztrap+
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
Status: RESOLVED → VERIFIED
Attached image Verify_Calendar.png
You need to log in before you can comment on or make changes to this bug.