176100 - tabbing out of file upload form control highlights text area contents

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Christopher Blizzard (:blizzard)]

Steps to reproduce:

1. Open a bugzilla attachment page
2. Give focus to the file upload form control with the mouse
3. Type something into the file upload form text area
4. Hit tab

Current behaviour:

Contents of the control are selected.

Expected behaviour:

Focus is given to the file selector button.

Comment 1

[Aaron Leventhal]

This must be a regression due to my fix for bug 28583.

Who's [e-mail]? I'll look into this if you want.

Comment 2

[Aaron Leventhal]

There turned out to be a bug in the focus system related to file controls that
caused this.

When you tab to a button, esm::SendFocusBlur() generates a focus event on the
button, which bubbles up to the file control, which gets focused, which turns
around and focuses the textfield!

This causes 2 other problems than the 1 in this bug:
* when tabbing forward through a file control, the button becomes "half"
focused, and the text field becomes half focused.
* when tabbing backward into a file control, it would skip the button and go
straight to the textfield.

Marking sec508, access because of the focus problems.

Comment 3

[Aaron Leventhal]

Attachment: When file control gets bubbling focus event, don't focus text field, because it might be bubbling up from the button

Seeking r=jkeiser, sr=bryner

Comment 4

[Aaron Leventhal]

This also fixes bug 132557 and bug 141310

Comment 5

[Aaron Leventhal]

Attachment: Test case

This shows that tabbing, reverse tabbing, script focus and all work correctly.
You can even click-drag on the "browse" button to focus it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104025 [details] [diff] [review]
When file control gets bubbling focus event, don't focus text field, because it might be bubbling up from the button

+	     formControlFrame->GetType(&formControlType);

Um... how about just checking mType directly and skipping out on the three
virtual calls (to the frame, to the helper, and back to this content node)?

Alternately, you could use "type", which is in scope here and was gotten
above.. but I think that code should die and just check mType too.  ;)

Comment 7

[Aaron Leventhal]

Attachment: Uses variable "type" instead of getting it off the form control frame -- duh, thanks

Seeking r=/sr=

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104219 [details] [diff] [review]
Uses variable "type" instead of getting it off the form control frame -- duh, thanks

r/sr=bzbarsky if you reinstate that comment saying why the check is needed from
the earlier patch (no need for new patch for that).

Comment 9

[John Keiser (jkeiser)]

Comment on attachment 104219 [details] [diff] [review]
Uses variable "type" instead of getting it off the form control frame -- duh, thanks

r=jkeiser

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 104219 [details] [diff] [review]
Uses variable "type" instead of getting it off the form control frame -- duh, thanks

a=roc+moz for trunk

Comment 11

[Aaron Leventhal]

checked in

Comment 12

[[e-mail]]

Attachment: Remove the BF_SKIP_FOCUS_EVENT hack

The code needs set the focus to the inner text field when the form element
receives a focus event. Now the old code used a hack to prevent it recursively
setting the focus when the text field's focus event bubbled up to the form
element. However the new code explicitly ignores these bubbled events and
therefore the old hack is no longer required.

Comment 13

[Aaron Leventhal]

Great, I just checked this in last night for 1.2 -- you're saying that was
wrong? It seemed to work fine in my tests. Can you explain what my code does wrong?

Comment 14

[Aaron Leventhal]

I see -- you're saying now with the new bubble check I just checked in, we can
get rid of all the BF_SKIP_FOCUS_EVENT stuff.

Bz?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Don't regress bug 17196.... And it's not clear what other problems that's
"fixing".  I would want rods to comment on that....

Comment 16

[rods (gone)]

what am I suppose to comment on?

Comment 17

[Boris Zbarsky [:bzbarsky]]

On whether the "skip focus event" boolean still serves a useful function in
light of the change in this bug....

Comment 18

[[e-mail]]

My patch was just intended to point out the duplication of code.
I'll do more testing to make sure that I don't get any more events.

Comment 19

[[e-mail]]

bz: the difference with my patch is that the onfocus event is generated when
whichever of the inner textfield or button gets focus first; currenty the
onfocus event is only generated when the button gets focus (tabbing forward;
obviously tabbing backward always focuses the button first).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Ah..  That would be a consistency issue worth fixing...

Comment 21

[[e-mail]]

Attachment: Fix focus event when tabbing into file textbox

Updated the "Removes the BF_SKIP_FOCUS_EVENT hack" patch for bitrot.

Comment 22

[Asa Dotzler [:asa]]

is this fixed? should the bug be reopened for this last patch? If not then can
you unset the review requests? thanks.

Comment 23

[[e-mail]]

Reopening to fix focus events.

Comment 24

[John Keiser (jkeiser)]

Comment on attachment 107891 [details] [diff] [review]
Fix focus event when tabbing into file textbox

Yay, goodie.

But one little thing off the bat: you should really decrement the other
#defines if you are removing the 0th one.  Or else put in a comment there
saying // 0 unused - was BF_SKIP_FOCUS_EVENT

So that people will realize that they can use that slot in the future.

r=me otherwise though.

Comment 25

[[e-mail]]

Attachment: Updated for review comment and bitrot

Comment 26

[[e-mail]]

Fix was checked in by timeless.

Comment 27

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381