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)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 26
People
(Reporter: jwatt, Assigned: jwatt)
References
Details
(Keywords: hang, perf)
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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>.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #786614 -
Flags: review?(enndeakin)
Assignee | ||
Comment 2•11 years ago
|
||
Hmm. Even limiting it to 2000 causes a noticeable pause. Maybe 200 is a better limit.
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
Happy to ui-review, but :jwatt, could you possibly attach a build with attachment 786614 [details] [diff] [review]? Thanks so much!
Flags: needinfo?(jwatt)
Assignee | ||
Comment 6•11 years ago
|
||
Neil, how would I go about implementing the "and 27 more…" thing, given that this string is generated by script?
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 7•11 years ago
|
||
Boriss, can you provide some sort of estimate of when you'll be able to tell me how this should behave?
Flags: needinfo?(jboriss)
Comment 8•11 years ago
|
||
(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)
Comment 9•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(jboriss)
Updated•11 years ago
|
Attachment #786614 -
Flags: ui-review?(ux-review)
Comment 10•11 years ago
|
||
Attachment #787206 -
Attachment is obsolete: true
Comment 11•11 years ago
|
||
Comment on attachment 786614 [details] [diff] [review]
cap
I'm going to assume that another patch will be coming.
Attachment #786614 -
Flags: review?(enndeakin)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #786614 -
Attachment is obsolete: true
Attachment #789568 -
Flags: review?(enndeakin)
Assignee | ||
Updated•11 years ago
|
Attachment #789568 -
Attachment is patch: true
Comment 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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
Comment 15•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
(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.
Description
•