Closed Bug 965117 Opened 6 years ago Closed 6 years ago

fix fonts and strings on about:accounts

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: zaach, Assigned: zaach)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 10 obsolete files)

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.
Blocks: 905997
No longer blocks: 964920
Component: Firefox Sync: UI → FxA
Product: Mozilla Services → Firefox
Attached patch custom fonts and visual tweak (obsolete) — Splinter Review
Attachment #8367099 - Flags: review?(mhammond)
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?
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.
Attached patch focus fix (obsolete) — Splinter Review
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?
Attachment #8367108 - Flags: feedback?(enndeakin)
Whiteboard: [qa+]
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.
Attachment #8367108 - Flags: feedback?(enndeakin) → feedback-
(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.
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 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.
Attachment #8367099 - Flags: review?(mhammond)
Incorporated Neil and Gavin's feedback, although setFocus was still needed before calling moveFocus.
Attachment #8367099 - Attachment is obsolete: true
Attachment #8367108 - Attachment is obsolete: true
Attachment #8367528 - Flags: review?(mhammond)
Attachment #8367528 - Flags: review?(mhammond) → review?(gavin.sharp)
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?
Fix typo and check for visibility before trying to focus the frame.
Attachment #8367528 - Attachment is obsolete: true
Attachment #8367528 - Flags: review?(gavin.sharp)
Attachment #8367743 - Flags: review?(gavin.sharp)
Attached patch alternate patch (obsolete) — Splinter Review
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).
Attachment #8367746 - Flags: feedback?(zack.carter)
Attached patch patch (obsolete) — Splinter Review
Attachment #8367743 - Attachment is obsolete: true
Attachment #8367746 - Attachment is obsolete: true
Attachment #8367743 - Flags: review?(gavin.sharp)
Attachment #8367746 - Flags: feedback?(zack.carter)
Attachment #8367749 - Flags: review+
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.
Removed all focus related code and updated the styles with jgruen's latest.
Attachment #8367749 - Attachment is obsolete: true
Attachment #8368945 - Flags: review?(gavin.sharp)
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.
Attachment #8368945 - Flags: review?(gavin.sharp) → review-
Minus the new style this time.
Attachment #8368945 - Attachment is obsolete: true
Attachment #8368976 - Flags: review?(gavin.sharp)
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8368976 - Attachment is obsolete: true
Attachment #8368976 - Flags: review?(gavin.sharp)
Attachment #8369000 - Flags: review?(ttaubert)
Attachment #8369000 - Flags: feedback?(zack.carter)
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #8369000 - Flags: review?(ttaubert) → review+
Attached patch patch (obsolete) — Splinter Review
Attachment #8369000 - Attachment is obsolete: true
Attachment #8369000 - Flags: feedback?(zack.carter)
Attached patch patchSplinter Review
Attachment #8369001 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/923f7834d8d5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
OS: All → Mac OS X
Hardware: All → x86
Summary: Custom fonts on about:accounts and other visual tweaks → fix fonts and strings on about:accounts
Target Milestone: Firefox 29 → ---
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 29
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Depends on: 1339401
You need to log in before you can comment on or make changes to this bug.