413135 - Prevent canceling individual keystrokes on input type="file"

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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]

Comment on attachment 297980 [details] [diff] [review]
possible patch

this is a great idea

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: 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 [:bzbarsky]]

Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

r+sr=bzbarsky

Comment 5

[Daniel Veditz [:dveditz]]

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]]

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)]

(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][[e-mail]]]

Attachment: 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]

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]

Attachment: 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]

Can we make it impossible to disable a file control while we're firing the key event?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

Comment on attachment 299553 [details] [diff] [review]
an idea for disabled file inputs

Will simplify this a bit.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simpler one, and doesn't clear mHasBeenDisabled if event was for button

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: previous patch + comment fixes

Jonas, what do you think about this?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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]

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]]

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]]

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]

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)]

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]

Attachment: Simple test using disabled property

Updated simple test to show disabled property functionality.

Comment 23

[Gregory Fleischer]

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]

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)]

Verified in comment 21.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 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)]

Comment on attachment 311509 [details] [diff] [review]
Merged to 1.8.0 branch

a=caillon for 1.8.0.15