Closed Bug 902234 Opened 11 years ago Closed 11 years ago

Cap the number of files that we include in the tooltip for <input type=file>

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: hang, perf)

Attachments

(2 files, 2 obsolete files)

In bug 894840 I'm extending the behavior of <input type=file to allow picking of a directory, which picks all the files in that directory and all of its subdirectories. This can potentially result in the .files property containing a FileList containing hundreds of thousands of files or more. In my testing I'm seeing very significant jank. One of the causes of this jank is that the code in popup.xml that builds up the tooltip for <input type=file> can take long enough to trigger the slow script prompt. We should cap the number of files that we include in the tooltip for <intput type=file>.
Attached patch cap (obsolete) (deleted) — Splinter Review
Attachment #786614 - Flags: review?(enndeakin)
Hmm. Even limiting it to 2000 causes a noticeable pause. Maybe 200 is a better limit.
Comment on attachment 786614 [details] [diff] [review] cap Review of attachment 786614 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/popup.xml @@ +581,5 @@ > } else { > titleText = files[0].name; > + // In the case of a directory being picked there could be hundreds of thousands of > + // files in .files or more. We cap the number that we show in the tooltip. > + let length = Math.min(files.length, 2000); 2000 lines seems like way too many to show and looks unpolished IMO. I think a better UX would be to list a smaller number such as 50 or 100 that could reasonably fit on users' screens and then include text to indicate that the list is truncated. Example of the bottom of the tooltip with 77 files selected (assume it starts from file01.pdf): file49.pdf file50.pdf and 27 more… @@ +582,5 @@ > titleText = files[0].name; > + // In the case of a directory being picked there could be hundreds of thousands of > + // files in .files or more. We cap the number that we show in the tooltip. > + let length = Math.min(files.length, 2000); > + for (let i=1; i<length; ++i) { Nit: while you're touching this line, I think we should put spaces around the operators like we normally do.
Attachment #786614 - Flags: ui-review?(ux-review)
Happy to ui-review, but :jwatt, could you possibly attach a build with attachment 786614 [details] [diff] [review]? Thanks so much!
Flags: needinfo?(jwatt)
Got it
Flags: needinfo?(jwatt)
Neil, how would I go about implementing the "and 27 more…" thing, given that this string is generated by script?
Flags: needinfo?(enndeakin)
Boriss, can you provide some sort of estimate of when you'll be able to tell me how this should behave?
Flags: needinfo?(jboriss)
(In reply to Jonathan Watt [:jwatt] from comment #6) > Neil, how would I go about implementing the "and 27 more…" thing, given that > this string is generated by script? String in .properties files support substitution but since this involves plurals it's a little more complicated. You'll need to use PluralForm.jsm. See the downloads example at https://developer.mozilla.org/en-US/docs/Localization_and_Plurals#Developing_with_PluralForm The new string can go in the same chrome://global/locale/layout/HtmlForm.properties file that is used above your change.
Flags: needinfo?(enndeakin)
Let’s cap it at 20 and then have a message at the end that indicates the list has been truncated. The first 20 in the list are fine, no need to show the last. “and 1000 more....” is fine for the string at the end.
Flags: needinfo?(jboriss)
Attachment #786614 - Flags: ui-review?(ux-review)
Comment on attachment 786614 [details] [diff] [review] cap I'm going to assume that another patch will be coming.
Attachment #786614 - Flags: review?(enndeakin)
Attached patch patch (deleted) — Splinter Review
Attachment #786614 - Attachment is obsolete: true
Attachment #789568 - Flags: review?(enndeakin)
Attachment #789568 - Attachment is patch: true
Comment on attachment 789568 [details] [diff] [review] patch >+# LOCALIZATION NOTE (AndXMore): this string is shown at the end of the tooltip >+# text for <input type='file' multiple> when there are more than 21 files >+# selected (when we will only list the first 20, plus an "and X more" line). >+# %S will be the number of files minus 20. >+AndXMore=and %S more I'd make this a clearer name such as 'AndXMoreFiles'
Attachment #789568 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #13) > I'd make this a clearer name such as 'AndXMoreFiles' Done. Thanks for the suggestions, pointers, UX and reviews everyone! https://hg.mozilla.org/integration/mozilla-inbound/rev/4bae09cea372
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Depends on: 905933
Comment on attachment 789568 [details] [diff] [review] patch Review of attachment 789568 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/locales/en-US/chrome/layout/HtmlForm.properties @@ +31,5 @@ > +# LOCALIZATION NOTE (AndXMore): this string is shown at the end of the tooltip > +# text for <input type='file' multiple> when there are more than 21 files > +# selected (when we will only list the first 20, plus an "and X more" line). > +# %S will be the number of files minus 20. > +AndXMore=and %S more This should use plural form not %S substitution.
(In reply to Stefan Plewako [:stef] from comment #16) > This should use plural form not %S substitution. Commenting in a closed bug is not a good way to get issues addressed (I for one don't even know enough about localization infrastructure to know whether what you say is correct or not). Please open a new bug, CC and needinfo? some peers, and maybe even add a patch and request review if this is a simple change. Please also comment here noting the bug number.
(In reply to Jonathan Watt [:jwatt] from comment #17) It was fixed in 905933
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: