We should display some message here about "You can view and manage your saved passwords here." Otherwise the blank screen looks broken.

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.

So, what would you like me to implement here?

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!

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

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

(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.

^ thanks Ally!

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

for realz this time.

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.

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

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.

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

^ +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!

(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

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

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

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

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.

^ How do you feel about svg images?

(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.

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

sgtm

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!

(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 :)

precedes Bug 1155345 in patch queue

(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..

(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

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 :)

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 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).

(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 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 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!

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

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.

(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.

(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.

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.

- complies with the spec outlined in Comment 40

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 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

(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 on attachment 8636300 [details] Screenshot_2015-07-20-15-57-23.png After the comments this should be good to go!

(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.

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

--> 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.

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.

> 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.

(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 on attachment 8636721 [details] screenshot Nice work Ally!

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.

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

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

(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.

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

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.

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.

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.

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