Prevent canceling individual keystrokes on input type="file"

VERIFIED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({testcase, verified1.8.1.12})

1.8 Branch
x86
All
testcase, verified1.8.1.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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.
Flags: blocking1.8.1.12?
Comment on attachment 297980 [details] [diff] [review]
possible patch

this is a great idea
Attachment #297980 - Flags: superreview+
Attachment #297980 - Flags: review+
(Assignee)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
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.
Attachment #297980 - Attachment is obsolete: true
Attachment #298006 - Flags: superreview?(jonas)
Attachment #298006 - Flags: review?(jonas)
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12+
Whiteboard: [sg:investigate] worth a try
Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

r+sr=bzbarsky
Attachment #298006 - Flags: superreview?(jonas)
Attachment #298006 - Flags: superreview+
Attachment #298006 - Flags: review?(jonas)
Attachment #298006 - Flags: review+
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
Attachment #298006 - Flags: approval1.8.1.12?
Comment on attachment 298006 [details] [diff] [review]
Allow preventing return/enter/tab

approved for 1.8.1.12, a=ss for release-drivers
Attachment #298006 - Flags: approval1.8.1.12? → approval1.8.1.12+
(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.

(Assignee)

Comment 8

10 years ago
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.
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.12, testcase
Resolution: --- → FIXED

Comment 9

10 years ago
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

10 years ago
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.
Can we make it impossible to disable a file control while we're firing the key event?
(Assignee)

Comment 12

10 years ago
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.
(Assignee)

Comment 13

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

Will simplify this a bit.
Attachment #299553 - Attachment is obsolete: true
(Assignee)

Comment 14

10 years ago
Created attachment 299559 [details] [diff] [review]
simpler one, and doesn't clear mHasBeenDisabled if event was for button
(Assignee)

Comment 15

10 years ago
Created attachment 299567 [details] [diff] [review]
previous patch + comment fixes

Jonas, what do you think about this?
Attachment #299567 - Flags: superreview?(jonas)
Attachment #299567 - Flags: review?(jonas)
(Assignee)

Comment 16

10 years ago
Remarking this 1.8.12? because I think we should take the last patch too.
Flags: blocking1.8.1.12+ → blocking1.8.1.12?
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 :(
Attachment #299567 - Flags: superreview?(jonas)
Attachment #299567 - Flags: superreview+
Attachment #299567 - Flags: review?(jonas)
Attachment #299567 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #299567 - Flags: approval1.8.1.12?
Removing fixed1.8.1.12 to signal a "reopen" based on new testcase and so we can tell when the new patch has landed.
Keywords: fixed1.8.1.12
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Comment on attachment 299567 [details] [diff] [review]
previous patch + comment fixes

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #299567 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.12
Whiteboard: [sg:investigate] worth a try → [sg:moderate]

Comment 20

10 years ago
Shouldn't bug 411236 be dependent on this bug?  It is also addressed by the fix.
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

10 years ago
Created attachment 300559 [details]
Simple test using disabled property

Updated simple test to show disabled property functionality.

Comment 23

10 years ago
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.
Sam: That sounds right. You should be able to type vowels, but typing a consonant should clear the control
Verified in comment 21.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.12 → verified1.8.1.12
Blocks: 411236

Updated

9 years ago
Flags: blocking1.8.0.15+
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.
Attachment #311509 - Flags: approval1.8.0.15?
Comment on attachment 311509 [details] [diff] [review]
Merged to 1.8.0 branch

a=caillon for 1.8.0.15
Attachment #311509 - Flags: approval1.8.0.15? → approval1.8.0.15+

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
Group: core-security
You need to log in before you can comment on or make changes to this bug.