1155459 - [RTL][FTE] The title of the Privacy Notice page should support bidirectional text

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P3)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: PrivacyNoticeScreenshot

Description:
The user will notice on the Privacy Notice page that the truncation appears at the beginning of the title when the language is set to Arabic. 

Prerequisite: Fresh flash or factory reset the DUT.
   
Repro Steps:
1) Update a Flame device to BuildID: 20150416010206
2) In the FTE, set the device language in Arabic under Settings > Language.
3) Keep tapping next until reaching the "About Firefox OS" page.
4) Tap on the "Privacy Notice" link and observe the title.

Actual:
The beginning of the title appears truncated. 

Expected: 
The Privacy Notice page should support bidirectional text.

Environmental Variables:
Device: Flame 3.0(Full Flash)(KK)(319mb)
BuildID: 20150416010206
Gaia: 629097847567e51095a454e7e63186a6e2ac0307
Gecko: a35163f83d22
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
  
Repro frequency: 100%
See attached: screenshot

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This also occurs on the Flame 2.2

The title of the Privacy Notice page is truncated at the beginning.

Device: Flame 2.2 (Full Flash)(KK)(319mb)
BuildID: 20150416002504
Gaia: 8e24d8b7f5e7c74c3004b22710dda0dac3e04ead
Gecko: 41388836b5c6
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

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

Not sure we would block on this at this stage but I'll nominate for consideration as its RTL-related. Either way I'll look into a fix.

Comment 3

[Dylan Oliver [:doliver]]

RTL triage: P3 for minor issue, not blocking 2.2

Comment 4

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

The privacy notice page is a dialog (PopupWindow) which loads the page at https://www.mozilla.org/ar/privacy/firefox-os/. ChildWindowFactory creates a PopupWindow instance for it, and it gets AppChrome to create the window header bar. AppChrome places the page title in the <h1> inside our old friend <gaia-header>. gaia-header's stylesheet includes the following rule: 

:host-context([dir=rtl]) ::content h1 {
    direction: rtl;
  }

This has the effect that a RTL direction is applied even when the title is in a latin characterset language like en-US. But even if we override this rule, the text is truncated and ellipsized from the left, per the RTL document direction of the system app's dialog window (not to be confused with the content document.) 

In other similar bugs we've been able to add a nested <bdi> element, but I don't think that approach can work here without some regression risk. I'm more inclined to block on bug 883884. With a dir="auto" on the <h1>, and 883884 fixed, this should "just work".

Comment 5

[Julien Wajsberg [:julienw]]

I think we should fix this for v2.2... Mostly because this panel could be used anywhere in the OS. So I fear we would have blocker bugs later on because of this, so better fix it now :/

That means without bug 883884 and adding a <bdi> with the classic ellipsis rules.

(and I feel somewhat guilty because I knew about this bug earlier and hadn't filed any)

Comment 6

[Julien Wajsberg [:julienw]]

This panel comes as soon as we use "window.open" in an application. For example the Contacts app uses this panel for importing contacts -- but I think this issue does not exist there because the title is small enough for now. I don't know all the places where we use this panel though.

Comment 7

[Julien Wajsberg [:julienw]]

Note that the panel could simply comes from a 3rd party app. Even maybe from web content using window.open (I'm not sure for this one though).

Comment 8

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

(In reply to Julien Wajsberg [:julienw] (PTO -> Apr 27) from comment #5)
> I think we should fix this for v2.2... Mostly because this panel could be
> used anywhere in the OS. So I fear we would have blocker bugs later on
> because of this, so better fix it now :/
> 
> That means without bug 883884 and adding a <bdi> with the classic ellipsis
> rules.

Ok, I can be convinced. I made a quick test of adding <bdi> to the template, and having the .title getter return that instead of the surrounding h1 and all hell broke loose. But assuming something like that can be made to work, we still need a way to audit all the ways this header gets populated and a manageable way to test deeply enough that we can be confident in the patch. You are probably right, it may come back to bite us if we dont fix this now though.

So we have the loading of external links from FTU, window.open from any app - both via ChildWindowFactory - what other entry points are there for this? If they all end up assigning (directly or indirectly) to appWindow.appChrome.title.textContent, that's quite managable. But I'm not sure at this point? If we have code trying to insert elements into the header for example, that will likely complicate things.

Comment 9

[Gregor Wagner [:gwagner]]

[Blocking Requested - why for this release]:

We knew that RTL is not going to be perfect. Lets keep working on it but decide about uplift based on the risk vs. value.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Sam Foster [:sfoster] from comment #8)

> So we have the loading of external links from FTU, window.open from any app

And actually FTU is using window.open.

> - both via ChildWindowFactory - what other entry points are there for this?

I think you need to find out the use from inside the System app only; find out when AppChrome uses the plain view (as opposed to the combined view). PopupWindow is one case, is there any other case ?

> If they all end up assigning (directly or indirectly) to
> appWindow.appChrome.title.textContent, that's quite managable. But I'm not
> sure at this point? If we have code trying to insert elements into the
> header for example, that will likely complicate things.

When I worked on the lock icon in bug 1141832 I made sure that the title got only the text; otherwise gaia-header can't do its magic. Using a <bdi> works as far as gaia-header is concerned (as it uses textContent) but using anything else than pure text would break gaia-header anyway.

So I think we're safe about your concern here.

Comment 12

[Julien Wajsberg [:julienw]]

Just found a STR that should reproduce the issue as well:
1. open http://lemonde.fr in Browser.
2. open any article
3. tap on the "P" (the icon for Pinterest)
=> This should open an overlay, and the title should have an ellipsis.

Note: The Twitter icon shows an ellipsis too, but only during some milliseconds until it's redirected; other icons' targets have shorter titles.

This shows how the issue can come up unexpectedly.

Comment 13

[Julien Wajsberg [:julienw]]

Note that it's still likely not a blocker -- not blocking any action, user's actions should still be properly carried.

I'm only renominating because of the new information.

There _could_ lead to a phishing issue if the target has no title (in that case because of the wrong ellispis we don't see the start of the URL -- so we don't see the domain).
But most websites have a title and in that case we don't have the URL anyway.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:bidi-popup-titles-bug-1155459 > mozilla-b2g:master

Comment 15

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

Moving component as the issue and fix is in system app chrome.

Comment 16

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

Comment on attachment 8603546 [details] [review]
[gaia] sfoster:bidi-popup-titles-bug-1155459 > mozilla-b2g:master

Basically, we'll get the plain chrome if: 

  !app.config.chrome || app.chrome.bar

...That is true for PopupWindows, for inline ActivityWindows (e.g. add to homescreen), and for TrustedWindows (e.g. persona login). 

So, this is a simple fix that inserts a nested <bdi> in the header, and which works for every case I've tested against (bidi content in both LTR and RTL locales). As this is inside a gaia-header - where integration testing is difficult - we are going to be leaning on review and manual verification for this I think.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8603546 [details] [review]
[gaia] sfoster:bidi-popup-titles-bug-1155459 > mozilla-b2g:master

I could not find any problems with this. LGTM!

Comment 18

[Sue]

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

Comment 19

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/062acd88f2d008718fd750d2ab37bc4cc87f0c7f

Comment 20

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

This patch should fix display of URLs and similar Bi-Directional issues in popup windows (window.open) as well as inline activities and trusted-UI  (see Comment #16) The FTU case in the bugs description is a good example of bidi content in a popup produced by window.open. We need this fix on 2.2, but this is quite a broad set of cases to test, so I wanted to make sure it was on QA's radar.

Comment 21

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

Correction: This patch should fix display of URLs and similar Bi-Directional issues in popup windows *titles*

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks, Sam.  Since it's SystemFE, I'll take a look.  also letting Delphine know since it's RTL related.

FYI, Delphine.

Comment 23

[Pi Wei Cheng [:piwei] (inactive)]

This issue looks fixed to me on master and I've not discovered any issues introduced by this patch. Following STR on comment 0 and on comment 12, LTR title is displayed as LTR in RTL environment.

Device: Flame
BuildID: 20150513010202
Gaia: 0d6c04f13fd385bda045f4e539b2a67cb5d84b1d
Gecko: 62d9b117c688
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Marking 3.0 as fixed.

Comment 24

[Pi Wei Cheng [:piwei] (inactive)]

Marking as verified on master.

Comment 25

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

Thanks Naoki! Am out at l10n workweek and unfortunately won't be very responsive until I'm back - but I've communicated this to Josh to confirm if this is in Marigold's scope. thanks for the heads up

Comment 26

[Josh Cheng [:josh]]

Hi William,
Could you help to check with Marigold that they have correspondent test cases (window.open) for RTL ready when the patch is landed? Thanks!

Comment 27

[William Hsu [:whsu]]

Test case had been created on MozTrap.
- https://moztrap.mozilla.org/manage/case/16082/

But, I would like to arrange one resource to do exploratory testing on related cases.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hi, Norry,

Please assign one resource to do tests on this patch (v3.0).
We need to make sure that all popup windows *titles* are displayed as expected in RTL languages.
In addition, please note the other tests cases you tested here.
Many thanks.

Comment 28

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

We are searching the system on master to try to trigger the pop-up window in some possible places. If any new bug is found, we will mark the new bugID into "see also" column.

NI Sue who is in charge of the exploratory testing.

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

A missed case is the popup for email setup for google.  ( I shared a link and then was trying to setup a gmail to use with email ).  I think there's some RTL issues with those pages in general.  Looks like there's a bug filed already : bug 1165205

Comment 30

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

Comment on attachment 8603546 [details] [review]
[gaia] sfoster:bidi-popup-titles-bug-1155459 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bidi in popup window titles
[User impact] if declined: Titles may be truncated/ellipsized incorrectly (RTL vs. LTR)
[Testing completed]: Tested on device in multiple languages. QA verification currently ongoing. 
[Risk to taking this patch] (and alternatives if risky): Small change but affects broad number of cases. That said, no known risk. 
[String changes made]: None

Comment 31

[Josh Cheng [:josh]]

Comment on attachment 8603546 [details] [review]
[gaia] sfoster:bidi-popup-titles-bug-1155459 > mozilla-b2g:master

Hi Sue,
Please also verify on 2.2 after patch landed there with comment 27 to 29.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Sue]

Attachment: verify_v2.2_pass.png

This issue has been verified as pass on latest nightly build of Flame 2.2 and Nexus 5 2.2 by STRs in comment 0 and comment 12.
Results: LTR title is displayed as LTR in RTL environment.
See attachment:verify_v2.2_pass.png
Rate:0/3

Device: Flame 2.2 (pass)
Build ID               20150520162503
Gaia Revision          bc42fbc12d622bffd7e8afcb8d56f8a1d9773c60
Gaia Date              2015-05-20 22:32:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8663598512f7
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150520.195701
Firmware Date          Wed May 20 19:57:10 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150520162503
Gaia Revision          bc42fbc12d622bffd7e8afcb8d56f8a1d9773c60
Gaia Date              2015-05-20 22:32:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8663598512f7
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150520.193900
Firmware Date          Wed May 20 19:39:15 EDT 2015
Bootloader             HHZ12f