Closed Bug 1155345 Opened 5 years ago Closed 4 years ago

UI affordance during loading of list items in about:passwords

Categories

(Firefox for Android :: General, defect, P2)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: liuche, Assigned: ally)

References

(Blocks 1 open bug)

Details

(Whiteboard: manage:passwords)

Attachments

(4 files, 4 obsolete files)

Instead of creating every item and appending it to the dom on load, just load the first N items and lazy-load the more on scrolling.

Optionally, we should look at using document fragments so the dom doesn't need to reflow every time.
I'm fairly certain that we currently add all the list items to an element, the list, that is not in the DOM. When we finish add all the list items, we add the list itself back into the DOM. This is essentially like using a document fragment.

Yes, we add all the items, so we could look at "paging" in items as needed. If we do that, then we should definitely make sre we always add list items to a fragment or element that is not yet in the DOM.
I also filed bug 1154367 to see if getting the logins out of sqlite is actually the slow part. I think we need some data to figure out where to put our optimization effort.
Assignee: nobody → ally
Just FYI. I don't think this is our highest priority bug for about:passwords. If we decide it is really important, maybe we should move the UI the native, where a "virtual" display system already exists.
Bill and I setting as P2
Priority: -- → P2
Whiteboard: manage:passwords
Blocks: 1167657
Rank: 25
Blocks: 1175279
Depends on: 1148524
the api doesn't allow for batch loading of logins. It's also synchronous. If we really want to fix the sqlite issue, we can either do a lot of work on the nsLoginManager service to allow batching or move the db itself into java (rnewman's preferred solution).

Neither of those has been judged worth the investment at this time, so we're looking at UI to let the user the page is doing something.
Summary: Batch loading of list items in about:passwords → UI affordance during loading of list items in about:passwords
Attached patch aboutLoginsLoadingThrobber (obsolete) — Splinter Review
The timeout does not seem to be firing for me to test this properly, but there is no error anywhere... hmph
Attached patch aboutLoginsLoadingThrobber (obsolete) — Splinter Review
Attachment #8631901 - Attachment is obsolete: true
Attachment #8633247 - Flags: review?(margaret.leibovic)
Attached image Screenshot_2015-07-13-18-26-20.png (obsolete) —
antlam, have you taken a look at this? The throbber in that screenshot seems pretty low resolution, is there a better asset we could use?
Flags: needinfo?(alam)
Attachment #8633247 - Attachment is patch: true
Comment on attachment 8633247 [details] [diff] [review]
aboutLoginsLoadingThrobber

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

This approach seems fine, but I want to get antlam's feedback on the resource we're using here.

::: mobile/android/themes/core/aboutLogins.css
@@ +11,5 @@
>  body {
>    height: 100%;
>  }
>  .hidden {
> +  display: none !important;

Why do you need to add this !important?

@@ +221,5 @@
> +  align-items: center;
> +  justify-content: center;
> +}
> +.loading-throbber {
> +  background: url(chrome://global/skin/media/throbber.png) no-repeat center;

Instead of making an element with a background, why not just include this as an <img>? That might also avoid the pixel-y issues I'm seeing in the screenshot, since that's likely caused by up-scaling this to 100px.
Attachment #8633247 - Flags: review?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8633247 [details] [diff] [review]
> aboutLoginsLoadingThrobber
> 
> Review of attachment 8633247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach seems fine, but I want to get antlam's feedback on the
> resource we're using here.
> 
> ::: mobile/android/themes/core/aboutLogins.css
> @@ +11,5 @@
> >  body {
> >    height: 100%;
> >  }
> >  .hidden {
> > +  display: none !important;
> 
> Why do you need to add this !important?

display: flex can override display:none of the hidden class. Results in empty-body visible with class="hidden". It was quite the bug to track down.

> 
> @@ +221,5 @@
> > +  align-items: center;
> > +  justify-content: center;
> > +}
> > +.loading-throbber {
> > +  background: url(chrome://global/skin/media/throbber.png) no-repeat center;
> 
> Instead of making an element with a background, why not just include this as
> an <img>? That might also avoid the pixel-y issues I'm seeing in the
> screenshot, since that's likely caused by up-scaling this to 100px.

I was trying to keep it consistent with the code layout used for the icons on the list body & edit screens. There is no particular technical necessity.
Got ahold of Anthony on irc

antlam
ally: lets go with that one then, and we can replace the rainbow with our fennec orange (#FF9500), http://codepen.io/mrrocks/pen/EiplA

antlam
ally: sweet, and we can make it 42px x 42px , same as the icon width
Flags: needinfo?(alam)
40px is looking really small. Antlam cna we make it a little bigger?
Flags: needinfo?(alam)
Attached patch aboutLoginsLoadingThrobber (obsolete) — Splinter Review
Attachment #8633247 - Attachment is obsolete: true
Attachment #8635060 - Flags: review?(margaret.leibovic)
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #14)
> 40px is looking really small. Antlam cna we make it a little bigger?

Can I see a screenshot? maybe get one of 60px x 60px and one of 40px x 40px?
Flags: needinfo?(alam) → needinfo?(ally)
Comment on attachment 8635060 [details] [diff] [review]
aboutLoginsLoadingThrobber

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

This patch failed to apply, so I wasn't able to test it out, but I'm going to redirect this review to liuche anyway, since she said she wanted to learn more JS, and there's no better way to learn than by doing (reviews)! :)
Attachment #8635060 - Flags: review?(margaret.leibovic) → review?(liuche)
:liuche, let me know if you also have problems applying, and ill rebase for you. Just let me know which tree you're on.
Flags: needinfo?(liuche)
Attached image 40px
Attachment #8633248 - Attachment is obsolete: true
Attached image 60px
Attached image 80px
I have attached variants for 40px, 60px, and 80px (I think 60px could still be too small). What do you think?
Flags: needinfo?(alam)
Doesn't apply for me. I'm on fx-team, a clean pull.

You could definitely try reviewboard! There are never "did-not-apply" problems there.
Flags: needinfo?(liuche)
Has the 60px.

While I've no doubt that rb is mostly superior, in this case it did not save me; a previous patch is to blame in this case.
Attachment #8635060 - Attachment is obsolete: true
Attachment #8635060 - Flags: review?(liuche)
Flags: needinfo?(ally)
Attachment #8636190 - Flags: review?(liuche)
Let's go with the 60, I think it's big enough. It's similar to the icon that would be there in other empty states too which brings more visual familiarity to the user.

thanks Ally!
Flags: needinfo?(alam) → needinfo?(ally)
conveniently enough, its 60px in the patch I sent to liuche for review, so that's already done. yay.
Flags: needinfo?(ally)
Comment on attachment 8636190 [details] [diff] [review]
aboutLoginsLoadingThrobber

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

R+ with nits + trying to get rid of that !important. Perhaps you could ask in #fx-team about it, and see what strategies they have for working around a !important. Additionally, if there are no workarounds (which I find hard to believe), add a comment about how display: flex will override display: none.

::: mobile/android/chrome/content/aboutLogins.js
@@ +46,5 @@
> +    let nonemptyBody = document.getElementById("logins-list-nonempty-body");
> +    let loadingBody = document.getElementById("logins-list-loading-body");
> +
> +    nonemptyBody.classList.add("hidden");
> +    loadingBody.classList.remove("hidden");

Can you pop this out as a function? They will always be called together, so for clarity, just make it toggleBody(isEmpty) or something.

::: mobile/android/themes/core/aboutLogins.css
@@ +11,5 @@
>  body {
>    height: 100%;
>  }
>  .hidden {
> +  display: none !important;

Hmmm I really don't like this !important. From everything I've read, it's really bad practice.

Have you tried some workarounds?

http://stackoverflow.com/questions/4087670/whats-the-equivalent-of-css-displaynone-in-flex

https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Flexible_boxes#Flex_item_considerations :
Maybe you could try visibility: collapsed or visibility: hidden?

::: mobile/android/themes/core/images/spinning_throbber.svg
@@ +1,1 @@
> +<svg class="spinner" width="65px" height="65px" viewBox="0 0 66 66" xmlns="http://www.w3.org/2000/svg">

This file doesn't have a license.

@@ +1,5 @@
> +<svg class="spinner" width="65px" height="65px" viewBox="0 0 66 66" xmlns="http://www.w3.org/2000/svg">
> +<style type="text/css">
> +.spinner {
> +  animation: rotator 1.4s linear infinite;
> +  }

Nit: the indentation for this file seems to be off - <style> is at the same level as <svg>.

::: mobile/android/themes/core/jar.mn
@@ +42,5 @@
>  #endif
>  #
>  #ifdef NIGHTLY_BUILD
>  * skin/aboutLogins.css                      (aboutLogins.css)
> +  skin/images/spinning_throbber.svg           (images/spinning_throbber.svg)

Nit: Is the indentation correct for the (paren) filename?
Attachment #8636190 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #26)
> Comment on attachment 8636190 [details] [diff] [review]
> aboutLoginsLoadingThrobber
> 
> Review of attachment 8636190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+ with nits + trying to get rid of that !important. Perhaps you could ask

:: mobile/android/themes/core/jar.mn
> @@ +42,5 @@
> >  #endif
> >  #
> >  #ifdef NIGHTLY_BUILD
> >  * skin/aboutLogins.css                      (aboutLogins.css)
> > +  skin/images/spinning_throbber.svg           (images/spinning_throbber.svg)
> 
> Nit: Is the indentation correct for the (paren) filename?

(In reply to Chenxia Liu [:liuche] from comment #26)
> Comment on attachment 8636190 [details] [diff] [review]
> aboutLoginsLoadingThrobber
> 
> Review of attachment 8636190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+ with nits + trying to get rid of that !important. Perhaps you could ask
> in #fx-team about it, and see what strategies they have for working around a
> !important. Additionally, if there are no workarounds (which I find hard to
> believe), add a comment about how display: flex will override display: none.

I tried more variations than I cared to count to get this patch into review the first time. visibility none & collapse and their friends result in wacky layout and incorrect behavior moving between screens. (Ill reapply it and send you the screenshot sometime) The answer shipped involves adding a useless div so that that display:none & display:flex do not interact.
https://hg.mozilla.org/mozilla-central/rev/1a54d668a108
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1203720
You need to log in before you can comment on or make changes to this bug.