Another set of patch to improve RTL for system app, this time to fix the stylesheets in apps/system/lockscreen/style/

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master Adding Gabriele for the Dialer part (callscreen) and Greg for the lockscreen part (which is the biggest part of this patch). Concerning the purely system part, the notification gets aligned with the date (on the left) instead of the icon (on the right). I'm testing with the Mirrored English locale but it does look broken.

> Concerning the purely system part, the notification gets aligned with the > date (on the left) instead of the icon (on the right). I'm testing with the > Mirrored English locale but it does look broken. Can you take a screenshot of the issue please? I re-did a quick test and didnt experience this problem using Arabic locale or Mirrored English (see attached screenshot)

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master The UI elements look good in both modes but for some reason with your patch applied the lockscreen's incoming call slider doesn't work anymore. Pressing the button and dragging it has no effect.

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master Gabriele: I fixed the incoming call slider issue, ready for review again.

(In reply to Etienne Segonzac (:etienne) from comment #7) > Created attachment 8674293 [details] > Screenshot of the notification issue Oh right. PR updated to fix this issue.

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master r=me on the system (not lockscreen) part, and adding back Gabriele and Greg for the rest of the review. The flags got lost in the PR update.

I've tested the last version of your patch and there's a glitch related to the SIM elements and clock position. The carrier name is shifted downwards and it seems to partially cover the clock which is also shifted downwards. Ask me for review again once you've solved this issue too. BTW it would be best if you could push a patch with your incremental changes on top of the current one, it makes reviewing the changes much easier. You can squash all the patches back into one once you're ready for landing.

PR updated - as recommended I didn't squash the new commit to ease review. Seems to work - at least the 2 above bugs (can't pickup calls and carrier detail glitch) are fixed.

I've applied the patch but it looks the notification highlights in a shorter way. This is the screenshot shows the symptom. I will attach another version without the patch for comparison.

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master As a notification of the issue above I cancel the review first.

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master The callscreen stuff looks fine now, thanks!

> > I've applied the patch but it looks the notification highlights in a shorter > way. This is the screenshot shows the symptom. I will attach another version > without the patch for comparison. Ouch, it was caused by a typo (padding replaced by margin)... Fixed now.

Comment on attachment 8672656 [details] [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master Applied the patch on my device and it looks good now. Thanks.

Thanks Greg. I squashed the patches and updated the PR. I suppose this can get merged as soon as the tests are finished.

This has a broken unit test: https://treeherder.mozilla.org/logviewer.html#?job_id=3158005&repo=b2g-inbound Reverted in https://github.com/mozilla-b2g/gaia/commit/11e4c572e3a2690e4ef1e8436beba55587ffed6d

Comment on attachment 8679325 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_notification > mozilla-b2g:master I'm taking this, as Pierre-Éric was on other subject recently. I've pushed another commit on top of Pierre-Éric one, that corrects the unit tests and handle the dir parameter correctly (at least it's how I understood w3c's spec about WebNotifications).

Comment on attachment 8679325 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_notification > mozilla-b2g:master Anything changed in the system (but not lockscreen) part of the patch since my last r+? If not you can "carry" the r+ for this part :) Also, looks like this PR needs rebasing.

Comment on attachment 8679325 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_notification > mozilla-b2g:master Ditto Etienne, I can't see any changes to the dialer part so you can carry my r+.

Etienne, yes, to notifications in the drawer and toaster. Is it your part?

Apparently I'm breaking some marionette tests. I'm updating.

(In reply to Augustin Trancart [:autra] from comment #25) > Etienne, yes, to notifications in the drawer and toaster. Is it your part? Yes! But frankly I'm getting lost, 2 commits on the PR, some changes partially reverted between the 2 etc... Can we keep this bug for the lockscreen-related changes and make a proper PR for the utility tray part?

(In reply to Etienne Segonzac (:etienne) AFK on October 21 and 26 from comment #28) > (In reply to Augustin Trancart [:autra] from comment #25) > > Etienne, yes, to notifications in the drawer and toaster. Is it your part? > > Yes! > But frankly I'm getting lost, 2 commits on the PR, some changes partially > reverted between the 2 etc... > Can we keep this bug for the lockscreen-related changes and make a proper PR > for the utility tray part? Well I think to split it is a good way to keep tracking and reviewing (and if it unfortunately need to be backed out). Although the review will be still good with a correct visual.

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master Hi Gabriel, Greg THe notification work has already landed in another PR. This one is only about lockscreen and callscreen. I preferred keep them together as it is stated they should be in sync. From the original commit, I just replaced a dir="ltr" with a direction: ltr in the css. Please R?

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master I'm taking over snowmantw's review in the interest of time. I applied this and started looking at the lockscreen in RTL. * An SMS notification on the lockscreen shows the '+' on the right of the sender number in RTL. I think this should be on the left? * The controls for the music player on the lockscreen are flipped in RTL and shouldn't be. * Also, looking at the callscreen itself, the toolbar buttons have not been reversed per the spec?

The music controls are flipped in RTL and shouldnt be.

(In reply to Sam Foster [:sfoster] from comment #32) > Comment on attachment 8680600 [details] > [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master > > I'm taking over snowmantw's review in the interest of time. I applied this > and started looking at the lockscreen in RTL. > > * An SMS notification on the lockscreen shows the '+' on the right of the > sender number in RTL. I think this should be on the left? Indeed, and it only occurs when you have one sim card :-) Filled bug 1220282 (seems to be a sms issue for me) > > * The controls for the music player on the lockscreen are flipped in RTL and > shouldn't be. Indeed. I'm uploading a fix in a moment. > * Also, looking at the callscreen itself, the toolbar buttons have not been > reversed per the spec? That's a bug! As this bug is only for the lockscreen part of the callscreen, I filled bug 1220285.

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master Another try! Sam, please r?

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master Better! thanks for filing those follow-ups.

(In reply to Augustin Trancart [:autra] from comment #35) > Comment on attachment 8680600 [details] > [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master > > Another try! Looks like tests are green so land at will once you've flatten the PR into one commit.

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master [Approval Request Comment] [Bug caused by] (feature/regressing bug #): none [User impact] if declined: Lockscreen will be broken in RTL [Testing completed]: on flame master [Risk to taking this patch] (and alternatives if risky): Medium, as it is quite a big patch. A complete testing have been made to mitigate this. There is no alternative if we want to support RTL. [String changes made]: none

Comment on attachment 8680600 [details] [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master Clearing uplift, as this is already in 2.5