Display message when no passwords are present

RESOLVED FIXED in Firefox 43

Status

()

Firefox for Android
Logins, Passwords and Form Fill
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: liuche, Assigned: ally)

Tracking

Trunk
Firefox 42
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox43 verified)

Details

Attachments

(6 attachments, 9 obsolete attachments)

103.42 KB, image/png
Details
109.18 KB, image/png
Details
13.31 KB, application/zip
Details
85.38 KB, image/png
antlam
: review+
Details
7.58 KB, patch
liuche
: review+
Details | Diff | Splinter Review
60.68 KB, image/png
Details
Created attachment 8525479 [details]
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.
Depends on: 1093201
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → ally
(Assignee)

Comment 2

3 years ago
So, what would you like me to implement here?
Flags: needinfo?(alam)
Created attachment 8554940 [details]
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!
Flags: needinfo?(alam)
Created attachment 8554941 [details]
img_lock_emptystate.zip
Created attachment 8554945 [details]
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
(Assignee)

Comment 6

3 years ago
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
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
^ thanks Ally!
Flags: needinfo?(ally)
(Assignee)

Comment 9

3 years ago
no problem. Liuche says you're very exact. :)
Flags: needinfo?(ally)
(Assignee)

Updated

3 years ago
Whiteboard: [shovel-ready]
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Whiteboard: [shovel-ready] → [wip]
No longer depends on: 1093201
(Assignee)

Updated

3 years ago
Whiteboard: [wip] → [shovel-ready]
(Assignee)

Comment 10

3 years ago
for realz this time.
Whiteboard: [shovel-ready] → [wip]
(Assignee)

Comment 11

3 years ago
Created attachment 8566362 [details] [diff] [review]
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.
(Assignee)

Comment 12

3 years ago
liuche has asked me to hold off on this until the ux duo can sync up on the about:passwords ui on desktop & android.
Whiteboard: [wip] → [blocked]
(Assignee)

Comment 13

3 years ago
Created attachment 8566884 [details] [diff] [review]
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.
Attachment #8566362 - Attachment is obsolete: true
Unblocking this - our android mocks are good for v1, and we'll want this empty state regardless of what subsequent versions are.
Whiteboard: [blocked]
^ +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!
(Assignee)

Comment 16

3 years ago
(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
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

3 years ago
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.
(Assignee)

Comment 21

3 years ago
^ How do you feel about svg images?
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
Created attachment 8575518 [details]
icon_key_emptystate.zip

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

Thanks Ally!
Attachment #8554941 - Attachment is obsolete: true
Flags: needinfo?(ally)
(Assignee)

Comment 24

3 years ago
sgtm
Flags: needinfo?(ally)
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!
Flags: needinfo?(alam)
(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 :)
Flags: needinfo?(alam)
(Assignee)

Comment 27

3 years ago
Created attachment 8631902 [details] [diff] [review]
NoPasswordsYetPage wip

precedes Bug 1155345 in patch queue
(Assignee)

Updated

3 years ago
Blocks: 1170786
(Assignee)

Comment 28

3 years ago
(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
(Assignee)

Comment 30

3 years ago
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.

- 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 :)
Attachment #8633158 - Flags: review?(alam)
(Assignee)

Comment 31

3 years ago
Created attachment 8633160 [details] [diff] [review]
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.
Attachment #8566884 - Attachment is obsolete: true
Attachment #8631902 - Attachment is obsolete: true
Attachment #8633160 - Flags: review?(liuche)
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).
Attachment #8633158 - Flags: review?(alam) → review-
(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

3 years ago
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!
Attachment #8633160 - Flags: review?(liuche) → feedback+
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.
Attachment #8633160 - Flags: feedback+ → feedback?(mconley)
No longer blocks: 1128664
(Assignee)

Comment 38

3 years ago
(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.
(Assignee)

Comment 39

3 years ago
(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.
(Assignee)

Comment 40

3 years ago
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.
Flags: needinfo?(alam)
(Assignee)

Comment 41

3 years ago
Created attachment 8636300 [details]
Screenshot_2015-07-20-15-57-23.png
Attachment #8633158 - Attachment is obsolete: true
Attachment #8636300 - Flags: review?(alam)
(Assignee)

Comment 42

3 years ago
Created attachment 8636302 [details] [diff] [review]
NoPasswordsYetPage

- complies with the spec outlined in Comment 40
Attachment #8636302 - Flags: review?(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.
Attachment #8636302 - Flags: review?(liuche) → feedback+
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
Attachment #8633160 - Flags: feedback?(mconley) → feedback+
(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.
Flags: needinfo?(alam)
Comment on attachment 8636300 [details]
Screenshot_2015-07-20-15-57-23.png

After the comments this should be good to go!
Attachment #8636300 - Flags: review?(alam) → review-
(Assignee)

Comment 47

3 years ago
(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.
(Assignee)

Comment 49

3 years ago
Created attachment 8636721 [details]
screenshot
Attachment #8525479 - Attachment is obsolete: true
Attachment #8636300 - Attachment is obsolete: true
Attachment #8636721 - Flags: review?(alam)
(Assignee)

Comment 50

3 years ago
--> 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.
Flags: needinfo?(ally)
(Assignee)

Comment 52

3 years ago
Created attachment 8636729 [details] [diff] [review]
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.
Attachment #8633160 - Attachment is obsolete: true
Attachment #8636302 - Attachment is obsolete: true
Attachment #8636729 - Flags: review?(liuche)
(Assignee)

Comment 53

3 years ago
(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.
Flags: needinfo?(ally)
Comment on attachment 8636721 [details]
screenshot

Nice work Ally!
Attachment #8636721 - Flags: review?(alam) → review+
Attachment #8636729 - Attachment is patch: true
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.
Attachment #8636729 - Flags: review?(liuche) → review+

Comment 58

3 years ago
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
(Assignee)

Comment 59

3 years ago
(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.
Flags: needinfo?(ally)
(Assignee)

Comment 60

3 years ago
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.

Comment 64

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/7d688a2f795b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Created attachment 8648608 [details]
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)
status-firefox42: fixed → ---
status-firefox43: --- → verified
You need to log in before you can comment on or make changes to this bug.