994725 - Screen spacing is off on Firefox Account sign in screen in FTE on device

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[No-Jun Park [:njpark]]

Attachment: 2014-04-10-11-05-35.png

The next button is not showing properly (as per screenshot)

STR:
- Flash the phone, and launch first time experience
- Select to create a new firefox account
- From the setup dialog, a new 'Firefox Account' window shows up

Expected:
The layouts should not exceed the screen real estate.

Actual:
Next button is cut off.

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: 20140415-overlay.png

Well, on my hamachi with today's pvt build, the spacing errors are *almost* totally fixed. See the second screenshot.

There is still some slight spacing problem, it looks like some kind of funny border on the top and left sides of the overlay. Also, the 'Next' button isn't quite centered.

I don't know what the underlying bug is that's causing this, which is a bit frustrating.

Anyhow, hopefully this resolves itself in the next day or two. Going to wait and see.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hmm, maybe I am wrong. now I can repro the error again?!?!

Working on understanding what's happening here

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sam volunteered to grab this, marking as assigned so I don't forget

Comment 5

[Sam Penrose]

It's also off on the Flame.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Pretty sure this is the source of the bug, need to test more thoroughly to be sure:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/firefox_account/firefox_account_container.css#L8-L15

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Yup, pretty sure that's it. Opening a PR, sorry Sam, found this while fixing something else :-)

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 19086

I'm not sure why this was needed in the past, but it no longer fixes the FxA panel height in FTU - it causes it to break.

Tested on Hamachi.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8419809 [details] [review]
Github PR 19086

Thanks Jared. I think Borja is a better reviewer for this change :)

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Adding blocking-b2g flag for this one

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8419809 [details] [review]
Github PR 19086

There is a deeper bug here.

Even with the CSS change, if I open the FxA dialog in FTU, then close it, and reopen from settings, it's too tall. Opening/closing the keyboard always fixes the height problem, so the dialog isn't getting reflowed when it is reloaded.

The dialog inherits from SystemDialog. The LayoutManager is supposed to manage height for SystemDialogs by triggering a reflow when it's opened, but in our case, this isn't working.

Digging into the deeper bug, unsetting r? for now.

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Aha! The underlying bug was in how SystemDialog.updateHeight() invoked LayoutManager's height getter.

This seems weird to me, but window.LayoutManager is the constructor, and window.layoutManager is the actual manager. I don't understand the value in exporting the constructor to the global scope, but maybe it's something related to unit tests?

At any rate, SystemDialog was attempting to reflow itself by setting its height to LayoutManager.height + 'px'. Since that's 'undefinedpx', Gecko was discarding the bad instruction.

I've updated SystemDialog to get the height from window.layoutManager.height. It seems weird to me to refer to a global that's lowerCamelCased, so I explicitly included the window object, to signify what's going on.

Since Alive did the review for the creation of SystemDialog (in bug 964653), I'll see if he can review.

Still going to remove the FTU CSS, since we don't need it.

Comment 13

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

BTW, I have used this test to reproduce the bug and the fix on hamachi:

1. Open FTU
2. Inside FTU, click the FxA 'Create Account or Sign In' button, opening the FxA dialog.
3. Close out the dialog.
4. Open Settings > Firefox Accounts
5. Click 'Create Account or Sign In' to open the dialog
6. Dialog is too tall
7. If you then trigger a reflow by clicking in and out of the email field, so that the keyboard is shown and hidden, the dialog will shrink to the correct height.

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8419809 [details] [review]
Github PR 19086

Hey Alive,

I've found a bug in the SystemDialog invocation of LayoutManager. It's a very small patch, do you have time for a review? (You looked at the original SystemDialog commit in bug 964653, so I thought you might be a good reviewer for this fix.)

The STR are in c13, and a discussion of the problem and solution are in c12.

Thanks!

Jared

(also setting ni? as mentioned in your bugzilla profile)

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8419809 [details] [review]
Github PR 19086

Sad typo...Thank you very much to catch and fix!

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks Alive!

master: https://github.com/mozilla-b2g/gaia/commit/464bcb9e91dca6196e4daf6a9d1b2c510050d716

RyanVM, I'm not sure if I should modify the blocking-b2g:2.0? flag. I've found a number of wiki pages on what the tracking flags mean, but still a bit mystified. Anything I need to do here?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Only B2G release management should be changing the blocking flags. Everything else looks fine here :)