Last Comment Bug 413135 - Prevent canceling individual keystrokes on input type="file"
: Prevent canceling individual keystrokes on input type="file"
Status: VERIFIED FIXED
[sg:moderate]
: testcase, verified1.8.1.12
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 1.8 Branch
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
Depends on:
Blocks: 411072 411073 411075 411077 411080 411236
  Show dependency treegraph
 
Reported: 2008-01-19 09:31 PST by Olli Pettay [:smaug] (reviewing overload)
Modified: 2008-09-04 21:40 PDT (History)
8 users (show)
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible patch (1.41 KB, patch)
2008-01-19 09:31 PST, Olli Pettay [:smaug] (reviewing overload)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
Allow preventing return/enter/tab (2.47 KB, patch)
2008-01-19 11:57 PST, Olli Pettay [:smaug] (reviewing overload)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review
simple test (726 bytes, text/html)
2008-01-26 07:59 PST, Olli Pettay [:smaug] (reviewing overload)
no flags Details
Example using disabled property to selectively cancel keystrokes. (3.97 KB, text/html)
2008-01-26 17:59 PST, Gregory Fleischer
no flags Details
an idea for disabled file inputs (8.01 KB, patch)
2008-01-27 05:40 PST, Olli Pettay [:smaug] (reviewing overload)
no flags Details | Diff | Splinter Review
simpler one, and doesn't clear mHasBeenDisabled if event was for button (7.90 KB, patch)
2008-01-27 07:03 PST, Olli Pettay [:smaug] (reviewing overload)
no flags Details | Diff | Splinter Review
previous patch + comment fixes (8.01 KB, patch)
2008-01-27 07:52 PST, Olli Pettay [:smaug] (reviewing overload)
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review
Simple test using disabled property (906 bytes, text/html)
2008-01-30 17:57 PST, Gregory Fleischer
no flags Details
Merged to 1.8.0 branch (6.17 KB, patch)
2008-03-24 22:11 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
caillon: approval1.8.0.next+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2008-01-19 09:31:26 PST
Created attachment 297980 [details] [diff] [review]
possible patch

I think one solution for those file upload bugs is to prevent canceling
individual keystrokes. Or basically, if some script cancels one keystroke, 
the value of <input type="file"> is cleared.
This means that automatic file upload may happen only if user writes the
whole filename.
Comments? Is this enough for branch? Would this break something?
(Shouldn't break the common cases)

See bug 411075, bug 411077, bug 411080, bug 411073, bug 411072.

As far as I see, at least Opera has the same bugs. The event handling is just
a bit different, so keyCode selection in the testcases should be changed a bit.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-19 09:49:30 PST
Comment on attachment 297980 [details] [diff] [review]
possible patch

this is a great idea
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2008-01-19 10:50:34 PST
So this doesn't sound very bad. good.
I'll post a new patch which handles all the cases when enter/return/space is
pressed etc. Didn't think those before - we do not always want to clear the file name.
Comment 3 Olli Pettay [:smaug] (reviewing overload) 2008-01-19 11:57:34 PST
Created attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

At least enter/return should be preventable. preventing tab doesn't cause any
harm either and might not be very rare.
In other cases clearing the file name should be ok.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2008-01-25 19:16:14 PST
Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

r+sr=bzbarsky
Comment 5 Daniel Veditz [:dveditz] 2008-01-25 19:58:35 PST
Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

Tested patch, appears to prevent the 5 variants gfleischer reported without breaking normal file control use. Wouldn't help if you could convince someone to type in the whole filename without interruption, but should be good enough until we have the untypeable control in FF3
Comment 6 Daniel Veditz [:dveditz] 2008-01-25 20:51:30 PST
Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

approved for 1.8.1.12, a=ss for release-drivers
Comment 7 Samuel Sidler (old account; do not CC) 2008-01-25 20:58:00 PST
(In reply to comment #6)
> (From update of attachment 298006 [details] [diff] [review])
> approved for 1.8.1.12, a=ss for release-drivers

I guess I could've come in and done it, but yes, a=me.

Comment 8 Olli Pettay [:smaug] (reviewing overload) 2008-01-26 07:59:59 PST
Created attachment 299422 [details]
simple test

When first typing something to file input, and then prevent default and continue
writing, file input should be cleared.
Comment 9 Gregory Fleischer 2008-01-26 17:52:41 PST
There is another method for canceling keystrokes that is not addressed by this fix.  By disabling and then re-enabling the file input element individual keystrokes can be canceled.

The use of the disable/enable approach is explained bug 56236.  Since that bug is not fixed on the branch, it remains a viable option to selectively capturing keystrokes.
Comment 10 Gregory Fleischer 2008-01-26 17:59:47 PST
Created attachment 299492 [details]
Example using disabled property to selectively cancel keystrokes.

This is an example from bug 411075 that has been updated to use the disable/enable approach to selectively cancel keystrokes.  When an unwanted key is detected, the file control is disabled.  A timer is started that immediately sets the disabled property to false.
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-26 20:07:07 PST
Can we make it impossible to disable a file control while we're firing the key event?
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2008-01-27 05:40:48 PST
Created attachment 299553 [details] [diff] [review]
an idea for disabled file inputs

I need to think this some more, but something like could work.
So basically if disabled file input is enabled, filename gets cleared before
when typing.
Comment 13 Olli Pettay [:smaug] (reviewing overload) 2008-01-27 06:29:11 PST
Comment on attachment 299553 [details] [diff] [review]
an idea for disabled file inputs

Will simplify this a bit.
Comment 14 Olli Pettay [:smaug] (reviewing overload) 2008-01-27 07:03:37 PST
Created attachment 299559 [details] [diff] [review]
simpler one, and doesn't clear mHasBeenDisabled if event was for button
Comment 15 Olli Pettay [:smaug] (reviewing overload) 2008-01-27 07:52:22 PST
Created attachment 299567 [details] [diff] [review]
previous patch + comment fixes

Jonas, what do you think about this?
Comment 16 Olli Pettay [:smaug] (reviewing overload) 2008-01-28 00:38:08 PST
Remarking this 1.8.12? because I think we should take the last patch too.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-28 02:25:11 PST
Comment on attachment 299567 [details] [diff] [review]
previous patch + comment fixes

Looks good. Man can't wait to get rid of all this stuff with the 1.9 branch :(
Comment 18 Daniel Veditz [:dveditz] 2008-01-28 11:38:03 PST
Removing fixed1.8.1.12 to signal a "reopen" based on new testcase and so we can tell when the new patch has landed.
Comment 19 Daniel Veditz [:dveditz] 2008-01-28 12:35:59 PST
Comment on attachment 299567 [details] [diff] [review]
previous patch + comment fixes

approved for 1.8.1.12, a=dveditz for release-drivers
Comment 20 Gregory Fleischer 2008-01-30 17:28:03 PST
Shouldn't bug 411236 be dependent on this bug?  It is also addressed by the fix.
Comment 21 Samuel Sidler (old account; do not CC) 2008-01-30 17:33:53 PST
Making sure I'm understanding the testcases, but...

For attachment 299422 [details], with Firefox 2.0.0.12, the file input control should be cleared for each keystroke in that control after checking the "prevent default" checkbox. This wasn't the case in Firefox 2.0.0.11, which left the characters that existed in that control prior to checking the "prevent default" box but didn't show new characters.

For attachment 299492 [details], with Firefox 2.0.0.12 typing vowels in the file input control shows a green bar over top of the control. Typing a consonant removes the bar and empties the control of all characters. This contrasts with Firefox 2.0.0.11, in that same testcase, where the green bar with vowels in it never clears and continues to cover the "Browse" button after typing consonants.

I'm certain that the change in behavior in attachment 299422 [details] is right and would verify this bug based on that, but I'm uncertain about the behavior change in attachment 299492 [details].

If someone can confirm that the behavior above is expected after the patches in this bug, that'd help a lot and allow me to call this bug "verified".

Tested using Firefox 2.0.0.12 RC 1: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/2008012820 Firefox/2.0.0.12
Comment 22 Gregory Fleischer 2008-01-30 17:57:23 PST
Created attachment 300559 [details]
Simple test using disabled property

Updated simple test to show disabled property functionality.
Comment 23 Gregory Fleischer 2008-01-30 18:02:17 PST
Attachment 299492 [details] and attachment 299492 [details] were showing different things.

Added test case in attachment 300559 [details] that lines up with attachment 299492 [details] and that can be used to test the disabled property method.  Results in these should match up more clearly.
Comment 24 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-30 22:10:10 PST
Sam: That sounds right. You should be able to type vowels, but typing a consonant should clear the control
Comment 25 Samuel Sidler (old account; do not CC) 2008-02-03 12:47:50 PST
Verified in comment 21.
Comment 26 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-24 22:11:15 PDT
Created attachment 311509 [details] [diff] [review]
Merged to 1.8.0 branch

Only a minor adaption on this patch, notably the |else if (NS_IS_TRUSTED_EVENT(aEvent) && ...| block does not exist on this branch.  Everything else is the same.
Comment 27 Christopher Aillon (sabbatical, not receiving bugmail) 2008-04-03 14:40:50 PDT
Comment on attachment 311509 [details] [diff] [review]
Merged to 1.8.0 branch

a=caillon for 1.8.0.15

Note You need to log in before you can comment on or make changes to this bug.