965117 - fix fonts and strings on about:accounts

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Zachary Carter [:zaach]]

This will add custom fonts to about:accounts, fix focusing of the email field on the sign up/in form, and a slight tweak to background color.

Comment 1

[Zachary Carter [:zaach]]

Attachment: custom fonts and visual tweak

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8367099 [details] [diff] [review]
custom fonts and visual tweak

>diff --git a/browser/base/content/aboutaccounts/aboutaccounts.js b/browser/base/content/aboutaccounts/aboutaccounts.js

>+        // Focus the first form element after a slight delay
>+        setTimeout(() => this.iframe.contentDocument.getElementsByTagName('input')[0].focus(), 500);

>+  // focus the first form element
>+  document.getElementById('remote').contentDocument.getElementsByTagName('input')[0].focus();

should these inputs just use the autofocus attribute instead?

>diff --git a/browser/base/jar.mn b/browser/base/jar.mn

>+        content/browser/aboutaccounts/fonts/clearsans-regular.woff            (content/aboutaccounts/fonts/clearsans-regular.woff)
>+        content/browser/aboutaccounts/fonts/firasans-light.woff               (content/aboutaccounts/fonts/firasans-light.woff)
>+        content/browser/aboutaccounts/fonts/firasans-regular.woff             (content/aboutaccounts/fonts/firasans-regular.woff)

Did you forget to include these in the patch? What's the licensing situation for these?

Comment 3

[Zachary Carter [:zaach]]

Clear Sans is Apache 2.0. Fira Sans is SIL Open Font License. The fonts were included in a previous patch, but not in the jar.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: focus fix

This seems to work to fix the focus. Neil should be able to tell me if there's a simpler way to do this, perhaps?

Comment 5

[Neil Deakin]

Comment on attachment 8367108 [details] [diff] [review]
focus fix

> function getStarted() {
>   hide("intro");
>   hide("stage");
>   show("remote");
>+  let iframe = document.getElementById("remote");
>+  Services.focus.setFocus(iframe, 0);
>+  Services.focus.moveFocus(iframe.contentWindow, null,
>+                           Services.focus.MOVEFOCUS_FORWARD, 0);

The above doesn't wait for remote to load, so when I tried it, sometimes the tab would be focused instead as that happens to be the next focusable element when the content isn't available yet.

A safer option is to use MOVEFOCUS_FIRST instead of MOVEFOCUS_FORWARD to move to the first focusable element in the window, and not call setFocus. This shouldn't navigate out of iframe.contentWindow.

The remote page could also just call focus() on the textbox itself, which makes the code here less dependent on the remote content, but maybe that isn't as easily changeable.

Comment 6

[Zachary Carter [:zaach]]

(In reply to Neil Deakin from comment #5)
> Comment on attachment 8367108 [details] [diff] [review]
> focus fix
> 
> > function getStarted() {
> >   hide("intro");
> >   hide("stage");
> >   show("remote");
> >+  let iframe = document.getElementById("remote");
> >+  Services.focus.setFocus(iframe, 0);
> >+  Services.focus.moveFocus(iframe.contentWindow, null,
> >+                           Services.focus.MOVEFOCUS_FORWARD, 0);
> 
> The above doesn't wait for remote to load, so when I tried it, sometimes the
> tab would be focused instead as that happens to be the next focusable
> element when the content isn't available yet.
> 
> A safer option is to use MOVEFOCUS_FIRST instead of MOVEFOCUS_FORWARD to
> move to the first focusable element in the window, and not call setFocus.
> This shouldn't navigate out of iframe.contentWindow.
> 
> The remote page could also just call focus() on the textbox itself, which
> makes the code here less dependent on the remote content, but maybe that
> isn't as easily changeable.

Thanks Neil. The remote page is using autofocus on the inputs, but it doesn't seem to work in this context, perhaps because the remote page is loaded in a hidden iframe first. I will try your suggested method to shift focus as we reveal the iframe.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Thanks Neil. It sounds like we should wait for the iframe's load event, and then do the moveFocus(MOVEFOCUS_FIRST) call then.

I didn't want to depend on the page content doing this (since the page content can't control or easily detect when it is shown), and I also didn't want to have this code depend on the specific contents of the remote page (since that could change in the future).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8367099 [details] [diff] [review]
custom fonts and visual tweak

font packaging/styling changes are fine, but we need to rework the focusing changes.

Comment 9

[Zachary Carter [:zaach]]

Attachment: alternative approach to focus first input

Incorporated Neil and Gavin's feedback, although setFocus was still needed before calling moveFocus.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8367528 [details] [diff] [review]
alternative approach to focus first input

>diff --git a/browser/base/content/aboutaccounts/aboutaccounts.js b/browser/base/content/aboutaccounts/aboutaccounts.js

>   handleEvent: function (evt) {
>     switch (evt.type) {
>       case "load":
>         this.iframe.contentWindow.addEventListener("FirefoxAccountsCommand", this);
>         this.iframe.removeEventListener("load", this);
>+        // focus the frame after a slight delay to ensure it's ready
>+        setTimeout(() => focusFrameInut(this.iframe), 200);

Typo here (focusFrameInut).

> function getStarted() {
>   hide("intro");
>   hide("stage");
>   show("remote");
>+  focusFrameInput(document.getElementById("remote"));
> }

Seems like what we actually want is for the focus to occur either when clicking the "Get started button", or when the iframe content is loaded, whichever is latest. Setting focus before either of those has happened seems like it could mess things up, so we should probably try to be smarter here?

Comment 11

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-965117-Custom-fonts-on-about-accounts-and-other-.patch

Fix typo and check for visibility before trying to focus the frame.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch

This is what I was thinking. I haven't tested this, and I'm not sure whether the setTimeout is actually needed in the onload handler - did you verify that experimentally? It could very well be possible that focusing things from under the onload handler would not work for some reason (but in that case a setTimeout(0) should be sufficient).

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 14

[Zachary Carter [:zaach]]

Good news: we were able to take care of the focus issue in the hosted code by polling the focus status, so we can remove code related to that from this PR. I can wait for jgruen to finish his additional style changes and we can land them all at once if that's easier.

Comment 15

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-965117-Custom-fonts-on-about-accounts-and-other-.patch

Removed all focus related code and updated the styles with jgruen's latest.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8368945 [details] [diff] [review]
0001-Bug-965117-Custom-fonts-on-about-accounts-and-other-.patch

>diff --git a/browser/base/content/aboutaccounts/aboutaccounts.xhtml b/browser/base/content/aboutaccounts/aboutaccounts.xhtml

>+  <!ENTITY % syncPrefsDTD SYSTEM "chrome://browser/locale/preferences/sync.dtd">
>+  %syncPrefsDTD;

We should avoid re-using this dtd, and just add the "manage" string to aboutAccounts (string re-use in different contexts is bad in general)

>+          <h1>&nbsp;</h1>
>+          <h2><span>&aboutAccounts.pageTitle;</span></h2>

This seems odd? Why the empty <h1>?

>-              <a class="button" href="#" onclick="openPrefs()">Manage</a>
>+              <a class="button" href="#" onclick="openPrefs()">&manage.label;</a>

Good catch :/

>diff --git a/browser/base/content/aboutaccounts/main.css b/browser/base/content/aboutaccounts/main.css

>+#fxa-signup-header.fox-logo {

>+  -webkit-animation: movein .5s;
>+  -moz-animation: movein .5s;

don't want these (this ain't the web). I assume you just copied the style from the remote page, though? I suppose it's going to be a pain to keep them in sync :(

Can you put up a patch with just the string addition and font packaging changes, and then split the other stuff off to a new bug? We should get the easy/critical stuff in and focus on the styling later.

Comment 17

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-965117-Custom-fonts-on-about-accounts-and-other-.patch

Minus the new style this time.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

OK, this changes things as discussed:
- Use "Firefox Sync" as the main heading/title everywhere, so about:Accounts doesn't reference "Firefox Account" anywhere
- get rid of the strings causing trouble in bug 965113

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8369000 [details] [diff] [review]
patch

Review of attachment 8369000 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: browser/locales/en-US/chrome/browser/aboutAccounts.dtd
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!ENTITY aboutAccounts.welcome "Welcome to Sync">

Welcome to &syncBrand.shortName.label;! :)

::: browser/locales/en-US/chrome/browser/syncBrand.dtd
@@ -3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY syncBrand.shortName.label  "Sync">
>  <!ENTITY syncBrand.fullName.label   "Firefox Sync">
> -<!ENTITY syncBrand.fxa-singular.label "Firefox Account">

That one is still used by browser/components/preferences/sync.xul. Should fix that up before landing.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 22

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/923f7834d8d5

Comment 23

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/923f7834d8d5

Comment 24

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:
FF 29.0b8
Build Id: 20140414143035
OS: Win 7 x64, Mac Os x 10.9.2, Ubuntu 13.04 x 64

about:accounts page was updated with the new fonts, colors and the heading was changed to Firefox Sync.