1159906 - [FTU] Use DOM overlays for links

Status: VERIFIED FIXED

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

Description

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

1. have a fresh install of fxos
2. go through the FTU until the last page (the one where it asks for email address).  turn wifi on
3. select privacy policy link

Expected: some way to exit out of the privacy notice and get back to the FTU's last page.
Actual: no way to exit, and you're stuck at web browsing through the Firefox OS Privacy notice page.

Note:
1. the only real option is to reboot and it takes you through FTU again.

Gaia-Rev        6e35b0948c42a4398b8a5916015de167121683a1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/1ad65cbeb2f4
Build-ID        20150429010205
Version         40.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150429.044126
FW-Date         Wed Apr 29 04:41:37 EDT 2015
Bootloader      L1TC000118D0

Comment 1

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

[Blocking Requested - why for this release]:  Checked 2.2 and 2.2 has the x to close out.

Comment 2

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

b2g-inbound regression window:

Last Working
Device: Flame
BuildID: 20150427103815
Gaia: e8f4231141210db01f43405e0c0336f480083ae7
Gecko: fce6aac57f06
Version: 40.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

First Broken
Device: Flame
BuildID: 20150427110514
Gaia: 5fd082cb265885248974fbb4ff70239901a6427e
Gecko: 0a66e85d0a42
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Last Working Gaia & First Broken Gecko - issue does NOT repro
Gaia: e8f4231141210db01f43405e0c0336f480083ae7
Gecko: 0a66e85d0a42

Last Working Gecko & First Broken Gaia - issue DOES repro
Gaia: 5fd082cb265885248974fbb4ff70239901a6427e
Gecko: fce6aac57f06

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/e8f4231141210db01f43405e0c0336f480083ae7...5fd082cb265885248974fbb4ff70239901a6427e

Caused by Bug 1157610.

Comment 3

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

Sam, can you take a look at this please? This might have been caused by the landing for bug 1157610.

Comment 4

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

(In reply to KTucker [:KTucker] from comment #3)
> Sam, can you take a look at this please? This might have been caused by the
> landing for bug 1157610.

I tried reverting that patch (3beb72345c1db0f21b8df9c464074d46b80d0017) and it didn't fix the problem for me. I don't see any other obvious candidates in that pushlog though so I'm not sure what's going on?

Comment 5

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

The suspected offending commit is abfa5a7a3ba409245df4d6a51ef3522ef6071b44. I'm reverting that now to see if it is the cause.

Comment 6

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

My mistake. This is caused by bug 1142526. I reverted https://github.com/mozilla-b2g/gaia/commit/16029ec24d87f9e1e352d35c0f32f2171735925e and issue no longer reproduces after revert.

Comment 7

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

Sorry about that Sam, it was the other bug in the pushlog. Staś, can you take a look at this please? This has been caused by the landing for bug 1142526

Comment 8

[Staś Małolepszy :stas]

I'm able to repro.  I'll take a look at this.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1159906-ftu-remove-external_links > mozilla-b2g:master

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8600158 [details] [review]
[gaia] stasm:1159906-ftu-remove-external_links > mozilla-b2g:master

Sam found out that class="external" was being removed from the link.  It was responsible for showing the popup's chrome including the "x" button.  The reason why it was being removed was that bug 1142526 turned on l10n.js's DOM overlays logic (from bug 994357) for localization strings using innerHTML and the class attribute is not allowed in translations.

I dove deeper into the code and took a stab at refactoring it a bit by removing external_links.js and getLocalizedLink.  I could simplify further by defining data-l10n-ids in HTML instead of doing that dynamically in js/navigation.js, but that might have perf impact.  I'd like to better understand why the dynamic way was chose in the first place.

I'll wait for the try build to complete before asking for review:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=302ec377645697fd56f9d4ff472803010eff791e

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Staś Małolepszy :stas from comment #10)
> I'd like to better understand why the dynamic way was chose in the first place.

Weird, not sure why the history is partial but that file comes from bug 820344, and that's ages ago in terms of l10n.js

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8600158 [details] [review]
[gaia] stasm:1159906-ftu-remove-external_links > mozilla-b2g:master

Sam, I removed getLocalizedLink completely and instead took advantage of the DOM overlaying mechanic which recently landed in l10n.js.  It makes it possible to use some HTML in translations and overlays then with attributes from the source DOM.

I also think it would be best to define data-l10n-ids directly in HTML.  L10n.js's declarative API should be preferred over JS one whenever possible.  I think the original reason why this wasn't the case was that back when flod was implementing that, mozL10n.localize() (now obsolete) was adding the ids and args anyways in addition to actually localizing the element.  So maybe it was considered as redundant to also put the ids in the HTML.  Today we only need to define the ids in the HTML and the l10n's mutation observer will take care of the rest and localize the element asynchronously.

I separated the data-l10n-ids change into a second commit in my pull request in case you wanted to limit the extent of change in this bug.  I also removed one assertion which wasn't testing much anymore.

Can you take a look at this?

Comment 13

[Staś Małolepszy :stas]

There are 3 Marionette tests failing, I'll fix them on Monday.

Comment 14

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

Comment on attachment 8600158 [details] [review]
[gaia] stasm:1159906-ftu-remove-external_links > mozilla-b2g:master

Looks great, I took this for a spin last night and it all looked good. Getting rid of external_links.js entirely is a big win. There's some Gij test failures - they'll need fixing and I would appreciate your help there -  I took a quick look but it was not obvious to me what the issue was at first glance?

Comment 16

[Staś Małolepszy :stas]

Ted was able to identify the reason of the Gij failure as bug 1137094 and provided a fix.  I think this is good to land now.

Comment 17

[GitHub Autolander Bot]

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

Comment 18

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

This issue is verified fixed on today's 3.0 nightly Flame. Links to external webpages on FTU now contain an X on upper left and user can press on X to close out the page to return to FTU.

Device: Flame 3.0
BuildID: 20150527010201
Gaia: 8ca93673869a64e09ed6153c5402896822dfb253
Gecko: ff2e07228041
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0