1212329 - (Gaia RTL 2.5) CSS refactoring System followup: lockscreen

Status: RESOLVED FIXED

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

Description

[pelloux]

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

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 8672656 [details] [review]
[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.

Comment 3

[pelloux]

> 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 4

[pelloux]

Attachment: Notification Arabic (left), Mirrored English (right)

Comment 5

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8672656 [details] [review]
[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 6

[pelloux]

Comment on attachment 8672656 [details] [review]
[gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master

Gabriele: I fixed the incoming call slider issue, ready for review again.

Comment 7

[Etienne Segonzac (:etienne)]

Attachment: Screenshot of the notification issue

Comment 8

[pelloux]

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

[Etienne Segonzac (:etienne)]

Comment on attachment 8672656 [details] [review]
[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.

Comment 10

[Gabriele Svelto [:gsvelto]]

Attachment: Lockscreen call overlay issue

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.

Comment 11

[pelloux]

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.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Notification highlight with the patch (e2cf7d7)

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 13

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Notification highlight without the patch (e2cf7d7)

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8672656 [details] [review]
[gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master

As a notification of the issue above I cancel the review first.

Comment 15

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8672656 [details] [review]
[gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master

The callscreen stuff looks fine now, thanks!

Comment 16

[pelloux]

> 
> 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 17

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8672656 [details] [review]
[gaia] Phoxygen:bug1212329-system-rtl-refactor-lockscreen > mozilla-b2g:master

Applied the patch on my device and it looks good now. Thanks.

Comment 18

[pelloux]

Thanks Greg.

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

Comment 19

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/ceb860bc4904b258da9763bc8683395b09f60983

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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 21

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug1212329-system_lockscreen_notification > mozilla-b2g:master

Comment 22

[Augustin Trancart [:autra]]

Comment on attachment 8679325 [details] [review]
[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 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8679325 [details] [review]
[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 24

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8679325 [details] [review]
[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+.

Comment 25

[Augustin Trancart [:autra]]

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

Comment 26

[Augustin Trancart [:autra]]

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

Comment 28

[Etienne Segonzac (:etienne)]

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

Comment 29

[Greg Weng [:snowmantw][:gweng][:λ]]

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

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master

Comment 31

[Augustin Trancart [:autra]]

Comment on attachment 8680600 [details] [review]
[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 32

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8680600 [details] [review]
[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?

Comment 33

[Sam Foster [:sfoster] (he/him)]

Attachment: lockscreen-rtl-music.png

The music controls are flipped in RTL and shouldnt be.

Comment 34

[Augustin Trancart [:autra]]

(In reply to Sam Foster [:sfoster] from comment #32)
> Comment on attachment 8680600 [details] [review]
> [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 35

[Augustin Trancart [:autra]]

Comment on attachment 8680600 [details] [review]
[gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master

Another try!

Sam, please r?

Comment 36

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8680600 [details] [review]
[gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master

Better! thanks for filing those follow-ups.

Comment 37

[Sam Foster [:sfoster] (he/him)]

(In reply to Augustin Trancart [:autra] from comment #35)
> Comment on attachment 8680600 [details] [review]
> [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 38

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/9fc74209e7046e435938d8949b525d80666fff78

Comment 39

[Augustin Trancart [:autra]]

Comment on attachment 8680600 [details] [review]
[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 40

[Augustin Trancart [:autra]]

Comment on attachment 8680600 [details] [review]
[gaia] Phoxygen:bug1212329-system_lockscreen_rtl > mozilla-b2g:master

Clearing uplift, as this is already in 2.5