1134009 - [Contacts] The secure lock icon overlaps the header text on the import contacts and sync facebook friends sign-in pages

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Joshua Mitchell (Inactive)]

Attachment: logcat_20150217_1414.txt

Description:
In contacts, you have the option of importing contacts from Gmail or Outlook. You can also sync your facebook friends. All of these options require a sign in and will navigate to a sign in page. These pages are 'secure' sign in pages as evidenced by the lock icon in the header. This icon is out-of-place and on the import contact's sign-in pages (gmail and outlook) the icon overlaps the text. On the facebook sync page it overlaps the X (to back out) icon. 

Notes: This also reproduces in RTL

Repro Steps:
1) Update a Flame to 20150217074222
2) Launch Contacts
3) Select Settings > Import Contacts > Gmail
4) Back up and select Settings > Import Contacts > Outlook
5) Back up and flip the Facebook Sync Friends toggle. 

Actual:
lock icon overlaps other text

Expected:
 No text overlap

Environmental Variables:
Device: Flame 3.0
Build ID: 20150217074222
Gaia: ae02fbdeae77b2002cebe33c61aedeee4b9439fd
Gecko: 4bb425001d8a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: 6/6
See attached: Logcat, Screenshots

--------------------------------------------------------------

This issue also occurs on 2.2

Device: Flame 2.2 (KK - Nightly - Full Flash)
Build ID: 20150217002515
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: 78d823f7be4c
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

-------------------------------------------------------------------

This issue does NOT occur on 2.1

Actual Result: The Lock icon is properly placed so there is no overlap with text or the X icon

Device: Flame 2.1 (KK - Nightly - Full-Flashed)
Build ID: 20150217001459
Gaia: e8eba437af02820f74d122aec83b6001df6f89e3
Gecko: 9d04f9149ca4
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 1

[Joshua Mitchell (Inactive)]

Attachment: 2015-02-17-14-04-04.png

Comment 2

[Joshua Mitchell (Inactive)]

Attachment: 2015-02-17-13-53-37.png

Comment 3

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Regression on a standard user path.

Comment 4

[Johan Lorenzo [:jlorenzo]]

I thought it was a general Gaia issue, but I can't repro with Firefox Account in Settings or with Gmail in Email. Do you have an idea of what could cause the issue, Francisco?

Comment 5

[Peter Bylenga [:PBylenga]]

Requesting a window.

Comment 6

[Brogan Zumwalt [Inactive]]

Last working B2G-Inbound build:
Device: Flame 3.0
Build ID: 20150210011855
Gaia: e846bc37ade8eaad82a00ffda6eb34906dbcc07f
Gecko: 20ae4196ad67
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First broken B2G-Inbound build:
Device:  Flame 3.0
BuildID: 20150210020821
Gaia: 8cade57c020f952bfe561c76f4afbcc51029e25a
Gecko: 876dde5c2924
Version: 38.0a1 (3.0)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Working Gaia with Broken Gecko issue does NOT reproduce:
Gaia: e846bc37ade8eaad82a00ffda6eb34906dbcc07f
Gecko: 876dde5c2924

Working Gecko with Broken Gaia issue DOES reproduce:
Gaia: 8cade57c020f952bfe561c76f4afbcc51029e25a
Gecko: 20ae4196ad67


B2G-Inbound Pushlog:
https://github.com/mozilla-b2g/gaia/compare/e846bc37ade8eaad82a00ffda6eb34906dbcc07f...8cade57c020f952bfe561c76f4afbcc51029e25a


Issue appears to have been caused by changes in bug 1112131

Comment 7

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

Julien, can you take a look at this please? Looks like the work done on bug 1112131 might be the cause here.

Comment 8

[Julien Wajsberg [:julienw]]

Joshua, can you attach a screenshot of when the bug does not happen? I don't really know this piece of code so this would help me :)
Thanks !

Comment 9

[Joshua Mitchell (Inactive)]

QA-Wanted for a screenshot from a working build (Comment 8)

Comment 10

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot_nonReproBuild.png

Here is a screenshot from Flame b2g-inbound build 20150210004714

Comment 11

[Julien Wajsberg [:julienw]]

Currently debugging the issue.

Comment 12

[Julien Wajsberg [:julienw]]

I'll try to describe the issue as good as possible.

There are 2 cases with the header:
* case A: we can center the title relatively to the screen while still showing it entirely, possibly reducing its font size.
* case B: we can't center the title relatively to the screen, so we'll center it relatively to the available space instead.

In case A, we use "margin-left: -50px" to make up for the action button.
In case B, we don't, but we possibly add a 10px padding so that the title is not too close to the edge.

In both cases we override any padding that's set in a CSS stylesheet.

We clearly see both cases in both attachments: facebook is case A, google is case B.


The best solution IMO is to put the icon out of the <h1> element (because this element is controlled by the gaia-header component).
But we need to take care to take into account when the lock icon is displayed because otherwise the font-fit measurement will be wrong. This can be done by adjusting the "title-start" attribute for the gaia-header component. Simply adjusting the attribute will make the font-fit algorithm run.


Other solutions I considered:

* add the lock-icon in a h1:before pseudo-element; we could use a gaia-icon to display it. Bonus point because it's more accessible. But because generated content is not reflected in textContent this will yield wrong calculations in gaia-header. Not to mention the fact that gaia-header can not use the gaia-icon font currently.

* Add lock-icon directly in the markup as text. This has the same issue with the gaia-icon font.


Alive, what do you think?

Comment 13

[Julien Wajsberg [:julienw]]

Kevin might also give some insight here.

Comment 14

[Julien Wajsberg [:julienw]]

I also notice the same rule in chrome.css applies to both the gaia-header in such panels and the browser chrome (visiting https://www.mozilla.org for example), so we'll need to take care to not regress the latter.

Comment 15

[Kevin Grandon :kgrandon]

It seems like we should decouple the CSS for the SSL icon from the rocketbar vs the .bar case with this work.

We should also not use an icon font unless we do it for both cases. I'd probably recommend sticking to image assets until we port all of the rocketbar controls (back, forward, refresh), over to use gaia-icons.

I started looking briefly at using a h1::before declaration, and we can probably make that work, though we'd probably need to make the lock icon position:absolute. I'm not sure how to signal to gaia-header to change the available width. Is there a javascript API to do this?

Comment 16

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

I think Kevin is on it? Lemme know if you need my help. (I also think we need some UX input to put the two icons gracefully ?)

Comment 17

[Kevin Grandon :kgrandon]

No one on this yet, but I've added it to the SystemsFE triage, so someone should pick it up in the next few weeks. (It's up for grabs if anyone wants it)

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> It seems like we should decouple the CSS for the SSL icon from the rocketbar
> vs the .bar case with this work.
> 
> We should also not use an icon font unless we do it for both cases. I'd
> probably recommend sticking to image assets until we port all of the
> rocketbar controls (back, forward, refresh), over to use gaia-icons.
> 
> I started looking briefly at using a h1::before declaration, and we can
> probably make that work, though we'd probably need to make the lock icon
> position:absolute. I'm not sure how to signal to gaia-header to change the
> available width. Is there a javascript API to do this?

You can use "title-start" which will gives the same effect.
But I think h1::before will be difficult to make it work... You'll need to make sure that h1 is not a positioning context. It's probably easier to h

Comment 19

[Julien Wajsberg [:julienw]]

... to have the icon in a separate element before the <h1> element.
And if we relax [1] maybe you won't even have to change title-start.

[1] https://github.com/gaia-components/gaia-header/blob/5fb0d83ec16db6090a852d17024da0f7b6589c93/gaia-header.js

Comment 20

[Julien Wajsberg [:julienw]]

I removed myself from assignee on purpose: this will need change in the app_chrome code and I don't know this code well enough. If there's really nobody to do it, I can try to carry it, but otherwise I'd prefer that someone from the System FE team does it.

Comment 22

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

I'll have a stab at this

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

Comment 25

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

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

Sad to have to do this is JS, this is what CSS is for. I get that gaia-header / fontFit cant really afford to get getComputedStyle on each pass though. 

I didn't think very hard about names with the new test app and marionette tests so I'm open to suggestions. Obviously the app chrome is used everywhere, I've tested different scenarios and looking at the CSS I think this is fixing the right things and not breaking anything else, but we'll need to keep an eye out.

Comment 26

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

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

Flagging Julien for feedback in light of earlier comments and bug 1138340

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

I haven't actually tried on the device, but I outlined some issues you need to take care about.

It's sad we need to use JS for this :/

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

I feel very dirty about moving it into JS, but the tests make it trivial to migrate later. One thing that we might be able to do is to hide this badness within some component which extends gaia-header, some kind of <app-popup-header> element or similar.

Please address Julien's comments on Github before landing. Feel free to re-flag me for a review if needed. Thanks!

Comment 29

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

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

I've updated the PR with the CSS nits and refactoring that code a little to hit it from both a localized event and the mozsecuritychange event, I hope that addresses your concern. 

I looked for an existing bug but didn't see one - hard to believe - I know this is a well known issue. So I filed bug 1140668, and referenced that in app_chrome.js

I'll carry kgrandon's r+, but wait for Julien's f+ (and clean Gaia-Try) before landing.

Comment 30

[Julien Wajsberg [:julienw]]

Sorry Sam, I won't be able to look at it as i'm leaving for holidays right now :)

Comment 31

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

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

Looking for some feedback/sanity check on how this plays out in practice.

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

LGTM on the windows opened to do oauth in contacts services.

Comment 33

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

I haven't noticed any problems with gaia-header in Messages for cases I can think of (for both RTL and LTR).

Comment 34

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

Thank you both.

Comment 35

[GitHub Autolander Bot]

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

Comment 36

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

Can we verify this before I request uplift? The dialog/popup app chrome is used gaia-wide so we need to be on the look-out for regressions. Naoki, flagging you to get this on your radar.

Comment 37

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

Attachment: 2015-03-10-16-52-54.png

There's related issue to this bug, when you have the orientation in landscape when you tap on the google accounts and then switch to portrait you won't have the lock and the X will overlap.  Not sure if that was accounted for.  See screenshot.

Comment 38

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

bug 1141832 filed for comment 37.

Comment 39

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

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #38)
> bug 1141832 filed for comment 37.

thx, clearing flags.

Comment 40

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly Flame 3.0 build.

Actual Results: The lock icon no longer overlaps  with the 'x' icon.

Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150312010235
Gaia: 0c4e8b0b330757e261b031b7e7f326ef419c9808
Gecko: 5334d2bead3e
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.2 on this when you get a chance.

Comment 42

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

Comment on attachment 8573342 [details] [review]
[gaia] sfoster:ssl-header-bug-1134009 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Gaia-Header component, SSL Lock icon in application popup headers
[User impact] if declined: Lock icon is misaligned and overlap/underlaps header content
[Testing completed]: Gaia-Try, tested on device, also tested by SMS and Contacts peers
[Risk to taking this patch] (and alternatives if risky): Low/Moderate. This code is shared by content popups across Gaia. Alternative is to live with the misalignment. 
[String changes made]: None

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Shally Li（leave from Mozilla）]

According to the STR of Comment 0,this bug has been successfully verified on latest Flame v2.2.
See attachment: verified_v2.2.png
Reproduce rate: 0/5

Device: Flame 2.2 (Pass)
Build ID               20150323162503
Gaia Revision          e54c4ed1cc188f70ddf1156534d364005dc45490
Gaia Date              2015-03-23 19:09:26
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150323.200543
Firmware Date          Mon Mar 23 20:05:54 EDT 2015
Bootloader             L1TC000118D0

Comment 45

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.png