1208170 - (Gaia RTL 2.5) CSS refactoring: System

Status: RESOLVED FIXED

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

Description

[Fabien Cazenave [:kaze]]

Improve the RTL support of the System app by refactoring its stylesheets and supporting right-to-left navigation: the task switcher and edge gestures are consistent with the language direction.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug-1181944-rtl-system > mozilla-b2g:master

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8665550 [details] [review]
[gaia] Phoxygen:bug-1181944-rtl-system > mozilla-b2g:master

This for the huge effort! I don't want to get in the way of this so I am just going to give you an r+ here.

I don't think I can effectively review this in any way, so I think we should ask QA for a quick approval instead.

Pallavi, is this something your team could help? Thanks!

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

BTW The csslint xfail list is getting longer and longer. Could you briefly explain what's going on on the mailing list so people can discuss what to do with it? Maybe we need to implement new feature into the linter or switch to another one that supports more features?

Comment 4

[Pallavi Yaramada :pallavi]

The plan is to have Marigold start testing this immediately (System app will be the first). William will inform them.

Comment 5

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #3)
> BTW The csslint xfail list is getting longer and longer. Could you briefly
> explain what's going on on the mailing list so people can discuss what to do
> with it? Maybe we need to implement new feature into the linter or switch to
> another one that supports more features?

See bug 1207131, as soon as we get the linter updated we should be able to get these back off the xfail list. :kaze, if you can  email a quick note to the dev-fxos list we can bring everyone up to date on the state of the CSS linter and the inline-* properties which its going to choke on with each PR until we get that fixed.

Comment 6

[Josh Cheng [:josh]]

Hi William,
As we discussed. Could you have Marigold do a quick check for a local build with the patch?
Just to make sure no major regression before official TC ready.
Thanks!

Comment 7

[William Hsu [:whsu]]

(In reply to Josh Cheng [:josh] from comment #6)
> Hi William,
> As we discussed. Could you have Marigold do a quick check for a local build
> with the patch?
> Just to make sure no major regression before official TC ready.
> Thanks!

Sure. I will find an engineer to verify local patches next Monday.
Thank you.

(Keep NI?)

Comment 8

[William Hsu [:whsu]]

Hi, Norry,

As we talked today, we can start to verify RTL patches in terms of test schedule. So, please refer to instructions which I sent you to verify RTL patches on FxOS system app.

Thank you.

Comment 9

[Fabien Cazenave [:kaze]]

Sam: mail sent to m.d.fxos.

Comment 10

[Fabien Cazenave [:kaze]]

Regarding the QA, I think the main aspect to check with the System app is that the app navigation should be mirrored in RTL: apps should be stacked according to the reading order.

Task switcher:
 • start at least two apps
 • long press on the home button
Expected result in LTR (e.g. English):
 • the first app should be stacked at the left edge
 • the last app should be stacked at the right edge
Expected result in RTL (e.g. Arabic):
 • the first app should be stacked at the right edge
 • the last app should be stacked at the left edge

Edge gestures:
 • start at least two apps
 • do a horizontal swipe from the edge of the screen
Expected result in LTR:
 • the first app should be stacked at the left edge
 • the last app should be stacked at the right edge
Expected result in RTL:
 • the first app should be stacked at the right edge
 • the last app should be stacked at the left edge

Web activities:
 • Settings app > Device Information
 • tap on the “Credits”  item
Expected result in LTR:
 • a browser window should slide in from the right edge
Expected result in RTL:
 • a browser window should slide in from the left edge

Comment 11

[Fabien Cazenave [:kaze]]

Still regarding the QA, it would be nice to check that we’ve properly mirrored all modal screens that are triggered by any app, e.g.:

 • the value selector (triggered when the user taps on a <select> element)
 • the date and time selector (taps on <input type="date|time"> element)
 • the system menu (taps on a <menu> element)
 • confirmation dialogs (OK/Cancel buttons)

The rest of the CSS refactoring of the System app will prevent a few “paper cuts” (small bugs). It would be difficult to list them all.

Comment 12

[Fabien Cazenave [:kaze]]

I’ve just rebased the patch, please make sure to use the latest branch so that all dependencies (building blocks & web components) have been refactored.

Comment 13

[Norry.L.F(Leave from Mozilla)]

Hi Fabien,

When we were verifying the patch, we found a bug in task manager view, please see bug 1209533.

Could you have a look? thanks :)

Comment 14

[Fabien Cazenave [:kaze]]

I’ve just assigned Pierre-Éric on bug 1209533.

Shoud we land this patch first, then work on bug 1209533?

Comment 15

[SandKing]

Hi Fabien,

When we were verifying the patch, we found a bug in  volume bar, please see bug 1209836.

Could you have a look? thanks :)

Comment 16

[William Hsu [:whsu]]

(In reply to Fabien Cazenave [:kaze] from comment #11)
> Still regarding the QA, it would be nice to check that we’ve properly
> mirrored all modal screens that are triggered by any app, e.g.:
> 
>  • the value selector (triggered when the user taps on a <select> element)
>  • the date and time selector (taps on <input type="date|time"> element)
>  • the system menu (taps on a <menu> element)
>  • confirmation dialogs (OK/Cancel buttons)
> 
> The rest of the CSS refactoring of the System app will prevent a few “paper
> cuts” (small bugs). It would be difficult to list them all.

Thank you, Fabien.

After you provided this RTL package (System app), I had reviewed the changes and delivered the test scope to our test engineers.
Here are the details. Please feel free to let me know if I missed something.

@ Test scopes (Switch to Arabic language)
  - Task manager/Card view
  - Edge gesture 
  - Network FET (Force error testing on WIFI & 3G & BT connections)
  - App installation
  - App - Ask permission page/App permission
  - App title
  - Utility tray/System status bar
  - Bluetooth transfer
  - NFC connection
  - Rocketbar
  - Warming message - Crash
  - Emergency callback
  - IME Menu
  - Modal dialog
  - Notification
  - Pin the web (Basic UI presentation)
  - Quick settings - Power off/Restart/Volume up&down…
  - Software buttons (Previous, next, and home button)
  - Text selection
  - Update manager

Comment 17

[Fabien Cazenave [:kaze]]

This list seems exhaustive to me, thanks William.

Just to be clear: this patch is just a CSS refactoring of the System app. We expect it to fix most BiDi issues, but not *all* BiDi issues. So before we can land this patch, we would like QA to:
 • validate the RTL navigation (task switcher and edge gestures), as described in comment 10;
 • check there’s no regression regarding the rest of the System app, as described in comment 16;
 • open issues for items that are not properly mirrored (comment 16).

As this is a rather long patch, we’d like to land it soon if it doesn’t cause any regression, and open follow-ups for the BiDi issues that it doesn’t fix. These follow-ups should depend on bug 1181944 rather than this one.

Comment 18

[SandKing]

According to comment 16, I have verified the items in the following list.
 - Task manager/Card view
  - Edge gesture 
  - App installation
  - App - Ask permission page/App permission
  - App title
  - Utility tray/System status bar
  - NFC connection
  - Rocketbar
  - Warming message - Crash
  - Emergency callback
  - IME Menu
  - Modal dialog
  - Notification
  - Pin the web (Basic UI presentation)

Comment 19

[pelloux]

As a refactoring of TaskManager landed on master I've rebased our patchset and updated the PR.

bug 1209533 has been solved in the process.

Comment 20

[Josh Cheng [:josh]]

Patch looks good, let's land this!

Comment 21

[Fabien Cazenave [:kaze]]

Tests are green, merged on master: https://github.com/mozilla-b2g/gaia/commit/b4faf78

Comment 22

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

(In reply to Fabien Cazenave [:kaze] from comment #21)
> Tests are green, merged on master:
> https://github.com/mozilla-b2g/gaia/commit/b4faf78

\o/ nice work :kaze and team.

Comment 23

[Fabien Cazenave [:kaze]]

Thanks Sam!

Comment 24

[Shine(Leave from Mozilla)]

According to comment 16 and comment 18, I have verified the rest items, please refer to the following list.
- Network FET (Force error testing on WIFI & 3G & BT connections)
- Bluetooth transfer
- Quick settings - Power off/Restart/Volume up&down…
- Software buttons (Previous, next, and home button)
- Text selection
- Update manager