1101746 - Display message when no passwords are present

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Empty passwords

We should display some message here about "You can view and manage your saved passwords here."

Otherwise the blank screen looks broken.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I'll take a look at this. 

Note to self: probably inheriting a lot of styles from bug 1014994 and keeping bug 1091826 in mind would make the most sense.

Comment 2

[Allison Naaktgeboren :ally]

So, what would you like me to implement here?

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pw_empty.png

Hey Ally,

Thanks for taking this! Here's a mock of the empty state that shares a lot of styles from the aforementioned bugs. 

I'll also attach the placeholder "lock" image as well (I know this is a web page but I'll throw in the usual variation of sizes too for now just in case). Keep in mind, this image might change (maybe the key icon) but that still seems up in the air so let's just use this for now.

Let me know if you have questions!

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: img_lock_emptystate.zip

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_pw_empty.png

here's a simple padding spec for now, the type is the same as the empty sync panel.

Header: Roboto, Light, #363B40, 20sp
Body: Roboto, regular, #777777, 14sp

Comment 6

[Allison Naaktgeboren :ally]

So you also want the second red line in the mock to be 20dp as well? It's not labeled: https://bug1101746.bugzilla.mozilla.org/attachment.cgi?id=8554945

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> So you also want the second red line in the mock to be 20dp as well? It's
> not labeled:
> https://bug1101746.bugzilla.mozilla.org/attachment.cgi?id=8554945

Yes, sorry about that!

Also, I noticed we updated the body to be 16sp rather than 14sp as I said in comment 5.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ thanks Ally!

Comment 9

[Allison Naaktgeboren :ally]

no problem. Liuche says you're very exact. :)

Comment 10

[Allison Naaktgeboren :ally]

for realz this time.

Comment 11

[Allison Naaktgeboren :ally]

Attachment: noPasswordMessage-wip

output is:
loadList called aboutPasswords.js:90:0
there were no logins aboutPasswords.js:102:0
actually removing hidden

the removeAttribute() line does not appear to be removing hidden and unhiding the div.

Comment 12

[Allison Naaktgeboren :ally]

liuche has asked me to hold off on this until the ux duo can sync up on the about:passwords ui on desktop & android.

Comment 13

[Allison Naaktgeboren :ally]

Attachment: noPasswordMessage-wip

uploading raw wip. properly shows & hides. Handles storage-change notifications correctly.

the 4 lock images are in the patch but not yet in the jar file.

Comment 14

[Chenxia Liu [:liuche]]

Unblocking this - our android mocks are good for v1, and we'll want this empty state regardless of what subsequent versions are.

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ +1

Also, I'm talking to our Desktop and iOS counterparts to see how we want to unify going forward but there will always be parts that we nee to keep consistent even just within our own product. This would be one of them (at least for now). So thanks for picking this up again Ally!

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #15)
> ^ +1
> 
> Also, I'm talking to our Desktop and iOS counterparts to see how we want to
> unify going forward but there will always be parts that we nee to keep
> consistent even just within our own product. This would be one of them (at
> least for now). So thanks for picking this up again Ally!

As long as you understand that this an html page not an android layout panel. For panels we seem to have some of both, so please keep that in mind for unification efforts going forward.


It looks like I can convert the dp to pixels. Is there a more elegant way to do this?

/**
 * This method converts dp unit to equivalent pixels, depending on device density. 
 * 
 * @param dp A value in dp (density independent pixels) unit. Which we need to convert into pixels
 * @param context Context to get resources and device specific display metrics
 * @return A float value to represent px equivalent to dp depending on device density
 */
public static float convertDpToPixel(float dp, Context context){
    Resources resources = context.getResources();
    DisplayMetrics metrics = resources.getDisplayMetrics();
    float px = dp * (metrics.densityDpi / 160f);
    return px;
}
http://stackoverflow.com/questions/4605527/converting-pixels-to-dp

Comment 17

[Chenxia Liu [:liuche]]

Android has a built-in method to convert DP to pixels:
http://developer.android.com/reference/android/content/res/Resources.html#getDimensionPixelSize%28int%29

Comment 18

[Chenxia Liu [:liuche]]

However, css does have relative dimensions - can you use those? You might need to ask Anthony for some mocks that are in terms of % of screen dimens instead of dp.

http://www.w3schools.com/cssref/css_units.asp

Comment 19

[:Margaret Leibovic]

For HTML, you can just use pixels, and the webpage will automatically scale. You should be able to just use the dp values specified in the mock-up directly as px values.

The only difference is that you'll need to include higher resolution images for higher DPI devices (or if you use SVG, that would magically take care of this for you). You can look at other in-content HTML pages we have as an example of how to do this, e.g. reader view:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#470

Comment 20

[Chenxia Liu [:liuche]]

It'd be great to try using svg, because we've talked about doing that but haven't done so yet. We don't use one large resource that can get scaled down because (I assume) it's more memory and CPU intensive for smaller, likely less powerful devices. I wonder if SVG has those setbacks? Or perhaps they're just larger files that we have to include.

Anthony, what do you think about trying some svg images? I don't know what your iconography design process is, so maybe that's infeasible.

In the meantime, the images provided are fine for using for code.

Comment 21

[Allison Naaktgeboren :ally]

^ How do you feel about svg images?

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #21)
> ^ How do you feel about svg images?

I'm open to it! Filed follow up bug 1141716 to track that work.

Comment 23

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_key_emptystate.zip

Let's update this to use the key that users see in the toolbar rather than the lock. 

Thanks Ally!

Comment 24

[Allison Naaktgeboren :ally]

sgtm

Comment 25

[Chenxia Liu [:liuche]]

Margaret mentioned that html/js pages could be a good candidate for using svg images. I'm not sure what the details are, but Anthony, if you have an svg resource anyways, stick it here!

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Chenxia Liu [:liuche] from comment #25)
> Margaret mentioned that html/js pages could be a good candidate for using
> svg images. I'm not sure what the details are, but Anthony, if you have an
> svg resource anyways, stick it here!

I filed and put it in bug 1141716 :)

Comment 27

[Allison Naaktgeboren :ally]

Attachment: NoPasswordsYetPage wip

precedes Bug 1155345 in patch queue

Comment 28

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #19)

> The only difference is that you'll need to include higher resolution images
> for higher DPI devices (or if you use SVG, that would magically take care of
> this for you). You can look at other in-content HTML pages we have as an
> example of how to do this, e.g. reader view:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> aboutReader.css#470

Er, aboutReader.css only goes up to 120..

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #28)
> (In reply to :Margaret Leibovic from comment #19)
> 
> > The only difference is that you'll need to include higher resolution images
> > for higher DPI devices (or if you use SVG, that would magically take care of
> > this for you). You can look at other in-content HTML pages we have as an
> > example of how to do this, e.g. reader view:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> > aboutReader.css#470
> 
> Er, aboutReader.css only goes up to 120..

HG Log shows it was split out:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css#240

Comment 30

[Allison Naaktgeboren :ally]

Attachment: Screenshot_2015-07-13-15-38-35.png

What do you think?

- Leave or remove the search/filter icon in the header? If you have no logins there's nothing to search.

- Do you want padding on the sides? There is any horizontal padding specified in the mock (unlike vertical padding), but it looks a little funny to me with out it.

Please remember this in html/css when providing feedback :)

Comment 31

[Allison Naaktgeboren :ally]

Attachment: NoPasswordsYetPage

It you're underwater, please feel free to reassign. 

- I feel like "Keep your logins safe" should end with a period especially since the line under it does. The mock does not have one though.

Comment 32

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8633158 [details]
Screenshot_2015-07-13-15-38-35.png

I left some specs in Comment 5 that would be helpful here I think. But, I put Roboto before, so we can just replace that with Clear Sans (same as our other about pages).

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #30)
> Created attachment 8633158 [details]
> Screenshot_2015-07-13-15-38-35.png
> 
> What do you think?
> 
> - Leave or remove the search/filter icon in the header? If you have no
> logins there's nothing to search.

Yeah, we can remove the Search icon if there are no logins.

Comment 34

[:Margaret Leibovic]

Comment on attachment 8633160 [details] [diff] [review]
NoPasswordsYetPage

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

::: mobile/android/themes/core/images/icon_key_emptypage.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="180px" height="180px" viewBox="0 0 180 180" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.2.2 (9983) - http://www.bohemiancoding.com/sketch -->

We should follow what was done in bug 1179303 to make sure this svg file is tidy.

ntim and shorlander made an etherpad with svg guidelines:
https://etherpad.mozilla.org/svg-guidelines

Comment 35

[Chenxia Liu [:liuche]]

Comment on attachment 8633160 [details] [diff] [review]
NoPasswordsYetPage

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

This is mostly fine, but I think it needs some more work to actually match the mocks, style- and layout-wise.

From irc, layout of the text should be 240dp, or 60dp from the edge. I'm not sure what that translates to in em, but I'm sure you can figure it out by playing around with the values and changing screen rotation, etc, until it matches the mocks.

::: mobile/android/chrome/content/aboutLogins.js
@@ +52,5 @@
>        debug("Master password permissions error: " + e);
>        logins = [];
>      }
> +    let emptyBody = document.getElementById("logins-list-empty-body");
> +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");

Why is this called nonempty-body? Let's rename it to something descriptive and positive (as opposed to relative and negative), like content-body.

@@ +54,5 @@
>      }
> +    let emptyBody = document.getElementById("logins-list-empty-body");
> +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");
> +    if (logins.length == 0) {
> +      emptyBody.classList.remove("hidden");

What's the difference between setAttribute("hidden"...) and add/remove("hidden")? Is one stylistically preferred over the other? It also looks like we do both in this file. For some reason I feel like setAttribute is better, but I'm not sure why exactly - maybe Margaret can chime in here.

::: mobile/android/locales/en-US/chrome/aboutLogins.dtd
@@ +7,3 @@
>  <!ENTITY aboutLogins.title                       "Logins">
>  <!ENTITY aboutLogins.save                        "Save">
> +<!ENTITY aboutLogins.emptyLoginText              "Keep your logins safe">

No, let's keep this consistent with the mock - no period.

::: mobile/android/themes/core/aboutLogins.css
@@ +183,5 @@
>      background-image: url("resource://android/res/drawable-xhdpi-v4/favicon_globe.png");
>    }
>  }
> +
> +#logins-list-empty-body {

empty-body instead.

@@ +191,5 @@
> +  text-align: center;
> +  justify-content: center;
> +}
> +
> +#emptyLoginText {

Why the camelCase? Also, leave out Login to keep it generic.

@@ +197,5 @@
> +  font-size: 25px;
> +  margin: 10px;
> +}
> +
> +#emptyLoginHint {

Same as above.

@@ +203,5 @@
> +  font-size: 20px;
> +  margin: 10px;
> +}
> +
> +.lock-icon {

Might as well make this .empty-icon - the other about: pages will end up using the same styling.

@@ +206,5 @@
> +
> +.lock-icon {
> +  background-image: url("chrome://browser/skin/images/icon_key_emptypage.svg");
> +  background-position: center;
> +  background-size: 70px 70px;

Do we generally do things in px, or is there a screen-scalable unit of measure? %? We should take advantage of our SVG usage.

@@ +208,5 @@
> +  background-image: url("chrome://browser/skin/images/icon_key_emptypage.svg");
> +  background-position: center;
> +  background-size: 70px 70px;
> +  background-repeat: no-repeat;
> +  height: 70px;

I see other places using background-position only - do you need to set height and width as well, esp if you're already setting background-position? (or maybe vice versa would work better)

::: mobile/android/themes/core/images/icon_key_emptypage.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>

We have this key icon in the Java resources - can you use that here instead of including another resource? On the other hand, if we want to try out svg, we can do that too!

Comment 36

[Chenxia Liu [:liuche]]

Oh, also, fonts should use em I think, not px.

Comment 37

[Chenxia Liu [:liuche]]

Comment on attachment 8633160 [details] [diff] [review]
NoPasswordsYetPage

Hey mconley, I'm trying to get more experience reviewing/writing modern JS/CSS because I've been more focused on my Java skills and a lot of the JS in Mobile is kind of old and not great as far as code examples go, so I wanted to know if you could give some feedback about this patch - not a full review, but just quick pass to get an idea of where we might be having blind spots.

Comment 38

[Allison Naaktgeboren :ally]

(In reply to Chenxia Liu [:liuche] from comment #36)
> Oh, also, fonts should use em I think, not px.

Margaret notes in https://bugzilla.mozilla.org/show_bug.cgi?id=1101746#c19 that px can be used for this directly.

Comment 39

[Allison Naaktgeboren :ally]

(In reply to Chenxia Liu [:liuche] from comment #35)
> Comment on attachment 8633160 [details] [diff] [review]
> NoPasswordsYetPage
> 
> Review of attachment 8633160 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +54,5 @@
> >      }
> > +    let emptyBody = document.getElementById("logins-list-empty-body");
> > +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");
> > +    if (logins.length == 0) {
> > +      emptyBody.classList.remove("hidden");
> 
> What's the difference between setAttribute("hidden"...) and
> add/remove("hidden")? Is one stylistically preferred over the other? It also
> looks like we do both in this file. For some reason I feel like setAttribute
> is better, but I'm not sure why exactly - maybe Margaret can chime in here.
>

In general, the classList approach is generally held to be more robust and better form. The setAttribute() calls were replaced with classList.remove/add at Margaret's behest in a previous bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1148524#c19.

> @@ +191,5 @@
> > +  text-align: center;
> > +  justify-content: center;
> > +}
> > +
> > +#emptyLoginText {
> 
> Why the camelCase? Also, leave out Login to keep it generic.

I am happy enough to leave out camel case, but not to make it generic.

This is an id, as indicated by the #, which to be legal css must be unique among all the rules that do(or could) cascade into this page. Uniqueness & specificiness in name is encouraged for ids. Perhaps you are thinking of classes, which should be more generic? 
> 
> @@ +197,5 @@
> > +  font-size: 25px;
> > +  margin: 10px;
> > +}
> > +
> > +#emptyLoginHint {
> 
> Same as above.
> 

> 
> @@ +206,5 @@
> > +
> > +.lock-icon {
> > +  background-image: url("chrome://browser/skin/images/icon_key_emptypage.svg");
> > +  background-position: center;
> > +  background-size: 70px 70px;
> 
> Do we generally do things in px, or is there a screen-scalable unit of
> measure? %? We should take advantage of our SVG usage.

As margaret notes in comment 19, using px will cause it to scale automatically. Which is nice.

> 
> ::: mobile/android/themes/core/images/icon_key_emptypage.svg
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 
> We have this key icon in the Java resources - can you use that here instead
> of including another resource? On the other hand, if we want to try out svg,
> we can do that too!

The idea is to try out svg, and that it will be less fragile in the future, and be one less thing to port over to svgs since we've decided to go that way.

Comment 40

[Allison Naaktgeboren :ally]

Keeping track of the visual spec for this has become quite complicated, and the outdated mock is probably not helping. Let's recap! 

comment 5:
	"here's a simple padding spec for now, the type is the same as the empty sync panel.
	Header: Roboto, Light, #363B40, 20sp
	Body: Roboto, regular, #777777, 14sp"
	mock [mock has lock icon]
comment 7:
	"updated the body to be 16sp rather than 14sp as I said in comment 5."
comment 19:
	margaret notes that one can use these same #s as px and hte page will scale automatically. win
comment 22:
	svg images a-ok! 
comment 23:
	"Let's update this to use the key that users see in the toolbar rather than the lock.'
	lock icon -> key icon
comment 32:
	Clear Sans replaces Roboto 
comment 33:
	remove filter icon on empty page
comment 34:
	svg internals clean up https://etherpad.mozilla.org/svg-guidelines
	possibly relevant to :antlam
comment 35:
	 text 60dp from the edge (of screen)

--> Current Visual Spec

	filter icon should be hidden
	padding between header & key icon 60 px
	svg key icon
	text in clear sans
	left/right margains text fields 60px
	hint/body has 16px padding above & below
	no period
	text in #363B40
	hint in #777777

Anthony, is there is anything else you'd like tweaked, now would be a marvelous time to let me know.

Comment 41

[Allison Naaktgeboren :ally]

Attachment: Screenshot_2015-07-20-15-57-23.png

Comment 42

[Allison Naaktgeboren :ally]

Attachment: NoPasswordsYetPage

- complies with the spec outlined in Comment 40

Comment 43

[Chenxia Liu [:liuche]]

Comment on attachment 8636302 [details] [diff] [review]
NoPasswordsYetPage

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

I still feel like the naming should be generic, and so we should make those items in css classes instead of ids. Does that make sense?

::: mobile/android/themes/core/aboutLogins.css
@@ +192,5 @@
> +  padding-left: 60px;
> +  padding-right: 60px;
> +}
> +
> +#empty-login-text {

These should be classes then, and generic, and you can set the id in the xhtml. There's nothing specific to this id that should make it a particular color or size.

@@ +198,5 @@
> +  font-size: 25px;
> +  margin-bottom: 16px;
> +}
> +
> +#empty-login-hint {

Actually, why are all these ids then, and not classes? I am under the assumption that classes should be for things that are reused, and ids should be for things that are specific to one particular place.

::: mobile/android/themes/core/images/icon_key_emptypage.svg
@@ +1,4 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<!-- 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/. -->

Nit: newline here.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8633160 [details] [diff] [review]
NoPasswordsYetPage

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

Not sure I've added much value here - I'm not too familiar with Fennec front-end.

Anyhow, the patch looks reasonable - just some nits and suggestions.

::: mobile/android/chrome/content/aboutLogins.js
@@ +52,5 @@
>        debug("Master password permissions error: " + e);
>        logins = [];
>      }
> +    let emptyBody = document.getElementById("logins-list-empty-body");
> +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");

I agree - perhaps logins-list-populated-body or logins-list-body is descriptive enough.

@@ +53,5 @@
>        logins = [];
>      }
> +    let emptyBody = document.getElementById("logins-list-empty-body");
> +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");
> +    if (logins.length == 0) {

You could also do

if (!logins.length)

But I'm not sure it adds much value or clarity.

Another alternative is to use the classList.toggle with the force argument, like this:

let hasLogins = logins.length > 0;
emptyBody.classList.toggle("hidden", hasLogins);
nonemptyBody.classList.toggle("hidden", !hasLogins);

Again - not sure if that adds much value or clarity here, but thought I'd mention it as a possibility.

@@ +60,5 @@
> +    } else {
> +      emptyBody.classList.add("hidden");
> +      nonemptyBody.classList.remove("hidden");
> +    }
> +

Nit - double newline

Comment 45

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #40)
> Keeping track of the visual spec for this has become quite complicated, and
> the outdated mock is probably not helping. Let's recap! 
> 
> comment 5:
> 	"here's a simple padding spec for now, the type is the same as the empty
> sync panel.
> 	Header: Roboto, Light, #363B40, 20sp
> 	Body: Roboto, regular, #777777, 14sp"
> 	mock [mock has lock icon]
> comment 7:
> 	"updated the body to be 16sp rather than 14sp as I said in comment 5."
> comment 19:
> 	margaret notes that one can use these same #s as px and hte page will scale
> automatically. win
> comment 22:
> 	svg images a-ok! 
> comment 23:
> 	"Let's update this to use the key that users see in the toolbar rather than
> the lock.'
> 	lock icon -> key icon
> comment 32:
> 	Clear Sans replaces Roboto 
> comment 33:
> 	remove filter icon on empty page
> comment 34:
> 	svg internals clean up https://etherpad.mozilla.org/svg-guidelines
> 	possibly relevant to :antlam
> comment 35:
> 	 text 60dp from the edge (of screen)
> 
> --> Current Visual Spec
> 
> 	filter icon should be hidden
> 	padding between header & key icon 60 px
> 	svg key icon

What size is this icon's height? Can it be 60 px tall? :D

> 	text in clear sans

I need a quick memory jog, do we support Clear sans light? If so, could we use it for these headers?

> 	left/right margains text fields 60px
> 	hint/body has 16px padding above & below

The hint should have 20px padding above

> 	no period
> 	text in #363B40
> 	hint in #777777
> 
> Anthony, is there is anything else you'd like tweaked, now would be a
> marvelous time to let me know.

Comment 46

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8636300 [details]
Screenshot_2015-07-20-15-57-23.png

After the comments this should be good to go!

Comment 47

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #45)
> 
> > 	text in clear sans
> 
> I need a quick memory jog, do we support Clear sans light? If so, could we
> use it for these headers?
> 

Looks like we don't, it's not a valid font according to the css computation engine.

Comment 48

[Chenxia Liu [:liuche]]

Are these useful? It looks like we have something in our tree.

http://mxr.mozilla.org/mozilla-central/search?string=clearsans&find=mobile/android&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 49

[Allison Naaktgeboren :ally]

Attachment: screenshot

Comment 50

[Allison Naaktgeboren :ally]

--> Current Visual Spec
	filter icon should be hidden
	padding between header & key icon 60 px
	svg key icon 60pc high
	text in clear sans
	left/right margains text fields 60px
	hint/body has 20px padding above & below
	no period
	text in #363B40
        hint in #777777

Liuche, wrt to clear sans light, I spent the morning talking to devtools & co. about css engine's behavior wrt this rule and the behavior displayed by the inspector as a result. The computation drops the rule, not as in 'the rule is invalid' or 'this rule is overridden' but that the rule is likely deemed by the engine to be redundant for some reason and optimized out. The hypothesis is the engine resolves Clear Sans Light to the font Clear Sans, making it redundant. 

If you wish to file a followup bug for whether or not this is a bug in css spec itself, in our css engine, or what, feel free. Regardless, I do not have reason to block this bug on it.

Comment 51

[Chenxia Liu [:liuche]]

Oh interesting, that sounds like a bug in our css engine then. Is there already a bug on file? And if not, can you send me some of the discussion or file the bug yourself? I don't feel like I have the context on why this doesn't work, but it definitely should be a bug on file.

Comment 52

[Allison Naaktgeboren :ally]

Attachment: NoPasswordsYetPage

> Actually, why are all these ids then, and not classes? I am under the assumption that classes should be for things that are reused, and ids should be for things that are specific to one particular place.

Ids being the highest priority & unique in css mean fewer surprises in the cascade, classes stomping on each other, and less (ab)use of !important. The one particular place is a nice rule of thumb, but the primary value is its much more predictable & less fragile place in the priority computation. We know that on fennec that jss/html/css is not our strong suite, writing less sophisticated but more robust css seems like the most sensible thing to do.

I have ported them to classes for the nonce because this bug has been dragging on far too long.

> Not sure I've added much value here - I'm not too familiar with Fennec front-end.
That's ok, we're looking to tap your superior js skills, something we on fennec don't have a ton of collectively.

Comment 53

[Allison Naaktgeboren :ally]

(In reply to Chenxia Liu [:liuche] from comment #51)
> Oh interesting, that sounds like a bug in our css engine then. Is there
> already a bug on file? And if not, can you send me some of the discussion or
> file the bug yourself? I don't feel like I have the context on why this
> doesn't work, but it definitely should be a bug on file.

I'm to talk to fantasai/dbaron about it to  either rule out or confirm bug in spec and then file in the appropriate system accordingly. W3C does not track their bugs in bmo.

Comment 54

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8636721 [details]
screenshot

Nice work Ally!

Comment 55

[Chenxia Liu [:liuche]]

Comment on attachment 8636729 [details] [diff] [review]
NoPasswordsYetPage

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

::: mobile/android/chrome/content/aboutLogins.js
@@ +58,5 @@
>        logins = [];
>      }
>      loadingBody.classList.add("hidden");
> +    if (!logins.length) {
> +      emptyBody.classList.remove("hidden");

Nit: would you add a newline here, just to separate the unhiding from the hiding?

@@ +62,5 @@
> +      emptyBody.classList.remove("hidden");
> +      filterIcon.classList.add("hidden");
> +      contentBody.classList.add("hidden");
> +    } else {
> +      emptyBody.classList.add("hidden");

Nit: Same as above, newline here.

Comment 56

[Pulsebot]

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

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/57628d4abc6b for breaking builds.

https://treeherder.mozilla.org/logviewer.html#?job_id=3886703&repo=fx-team

Comment 58

[:Margaret Leibovic]

I'm not sure I have all the context around the light font weight here, but what exactly were you trying to do that wasn't working? This currently works in about:feedback using `font-weight: lighter;`:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutFeedback.css#45

Comment 59

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #58)
> I'm not sure I have all the context around the light font weight here, but
> what exactly were you trying to do that wasn't working? This currently works
> in about:feedback using `font-weight: lighter;`:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> aboutFeedback.css#45

I was explicitly specifying the font. I'll try the font weight.

Comment 60

[Allison Naaktgeboren :ally]

moved the font weight to bug 1187059 since it impacts all the headers not just the empty page one.

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7d688a2f795b

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d688a2f795b

Comment 63

[Francesco Lodolo [:flod]]

http://hg.mozilla.org/mozilla-central/diff/7d688a2f795b/mobile/android/locales/en-US/chrome/aboutLogins.dtd

I have no idea how to read this

<!ENTITY % brandDTD
     SYSTEM "chrome://branding/locale/brand.dtd">
     %brandDTD;

But I'm pretty sure it will break every possible tool. For sure compare-locales complains about a missing brandDTD entity.

Comment 64

[Axel Hecht [:Pike]]

https://dxr.mozilla.org/mozilla-central/search?q=path%3Alocales%2Fen-US+SYSTEM&case=true shows that we're using this in various places already, unless I missed a typo in there.

Comment 65

[Francesco Lodolo [:flod]]

Interesting, I have absolutely no memory of this (and thus didn't search) but it's there in several files :-\

Issue is in my memory, not in the code.

Comment 66

[Matthew N. [:MattN]]

https://hg.mozilla.org/mozilla-central/rev/7d688a2f795b

Comment 67

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-08-17-11-16-15.png

Verified as fixed using:
Device: LG Nexus 4 (Android 5.0)
Build: Firefox for Android 43.0a1 (2015-08-17)