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.

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

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.

The timeout does not seem to be firing for me to test this properly, but there is no error anywhere... hmph

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?

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.

(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

40px x 40px :)

40px is looking really small. Antlam cna we make it a little bigger?

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

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

:liuche, let me know if you also have problems applying, and ill rebase for you. Just let me know which tree you're on.

I have attached variants for 40px, 60px, and 80px (I think 60px could still be too small). What do you think?

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.

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.

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!

conveniently enough, its 60px in the patch I sent to liuche for review, so that's already done. yay.

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?

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