305840 - Focus doesn't move to FindToolBar. (Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLInputElement.select]"

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Hiro]

When hit Ctrl+f or "/"(FAYT mode), focus doesn't move to FindToolBar. 
And the following errors are output to JavaScript Console. 

Reproducible: Always

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLInputElement.select]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/textbox.xml :: select :: line 69"  data: no]


Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.9a1) Gecko/20050824
Firefox/1.6a1

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Maybe, regression of Bug 303620.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIDOMHTMLInputElement.select]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame :: chrome://global/content/bindings/textbox.xml :: select ::
line 69"  data: no]
> ************************************************************

Debug build's message.

Comment 3

[Dave Townsend [:mossop]]

*** Bug 305894 has been marked as a duplicate of this bug. ***

Comment 4

[Mats Palmgren (inactive)]

It's a regression from bug 303620.  Patch coming up...

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

We should return early but not fail in case the focus was moved elsewhere.

The extra focus checks we added in ESM fails for the Find toolbar -
we have either a null frame (in case it was hidden) or a
non-focusable <textbox> if it was visible.

So, we need to add an explicit 'tabindex' attribute to make it
"IsFocusable" and also to flush frames to fix the null frame issue.

Comment 6

[Mats Palmgren (inactive)]

Hmm, now that I think about, shouldn't a <textbox> be "IsFocusable" by default?
Is this also what causes bug 305913 and potentially many others?

Comment 7

[Mats Palmgren (inactive)]

Yep, this fixes bug 305913:

 textbox {
   -moz-binding: url("chrome://global/content/bindings/textbox.xml#textbox");
   -moz-user-select: text;
+  -moz-user-focus: normal;
 }

Maybe we should go for that solution instead of having to add "tabindex"
all over?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

I prefer this version. It also fixes the about:config and GTK1 file picker
issues mentioned in bug 305913.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 10

[Mats Palmgren (inactive)]

"Patch rev. 2" also fixes the URL bar problem, bug 305872

Comment 11

[TimoT]

 textbox {
   -moz-binding: url("chrome://global/content/bindings/textbox.xml#textbox");
   -moz-user-select: text;
+  -moz-user-focus: normal;
 }

This change prevents the address bar dropdown history from opening

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #11)
>  textbox {
>    -moz-binding: url("chrome://global/content/bindings/textbox.xml#textbox");
>    -moz-user-select: text;
> +  -moz-user-focus: normal;
>  }
> 
> This change prevents the address bar dropdown history from opening

Thanks for testing the patch and finding this!

It seems the event handling in this file needs tweaking:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&root=/cvsroot
I can't say I really understand how that code works though...

Comment 13

[Mats Palmgren (inactive)]

Attachment: Additional tweak

Timo, could you apply this additional patch and see if it fixes it?
(it seems to work for me)

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 193887 [details] [diff] [review]
Additional tweak

Nevermind, it does not popup the autocompletion menu unless
you have opened it manually once...
Suggestions are welcome if someone knows this code.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Additional tweak, rev. 2

Ok, this seems to work. I'm not sure exactly why, I'm guessing the popup now
occurs later and that there is some focus handler somehwhere that
does stuff the takes down the menu...  please test it if you can, thanks.

Comment 16

[Aaron Leventhal]

Comment on attachment 193898 [details] [diff] [review]
Additional tweak, rev. 2

Mats, is this part of the patch necessary -- did you want to seek review for
it?

Comment 17

[Aaron Leventhal]

Comment on attachment 193870 [details] [diff] [review]
Patch rev. 2

> +  -moz-user-focus: normal 
I think doing this might mess up tab order somehow, because a XUL textbox
already has a focusable anonymous HTML input child.

Instead, let's fix the IsFocusable() check from SendFocusBlur(). Or, for now
just change it to a visibility check. That's still better than what it was,
which was just a check to see if the frame still existed.

Comment 18

[Mats Palmgren (inactive)]

(In reply to comment #17)
> (From update of attachment 193870 [details] [diff] [review] [edit])
> > +  -moz-user-focus: normal 
> I think doing this might mess up tab order somehow, because a XUL textbox
> already has a focusable anonymous HTML input child.

Yes, you're right, this breaks SHIFT-TAB over <textboxes> because we
get stuck on the outer textbox frame. I guess this means that the
-moz-user-focus property and the tabindex attribute is basically
unusable for a XUL textbox?  (let's deal with that elsewhere though)


> Instead, let's fix the IsFocusable() check from SendFocusBlur(). Or, for now
> just change it to a visibility check. That's still better than what it was,
> which was just a check to see if the frame still existed.
> 

Agreed, I added a "visible && !disabled" check instead in the upcoming patch...

Comment 19

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 20

[Aaron Leventhal]

(In reply to comment #19)
> Created an attachment (id=193917) [edit]
> Patch rev. 3
> 

I wonder if any other tags end up creating a XUL textbox. It might be better to
attempt a QI to nsIDOMXULTextboxElement, and then GetInputField() and then
GetPrimaryFrameFor to get the focusFrame for that, then continue with the normal
checking.

However, for branch we could just simplify this code completely and just check
for visibility only on everything, including area and textbox. The same check
for everything.

What's your thought?

Comment 21

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Yes, that's even better.

I kind of like the fact that we don't execute onfocus for stuff that isn't
focusable. Checking only for visibility wouldn't catch disabled textboxes
for example.

This patch isn't necessary for the branch unless we also check in bug 303620
on the branch?	I don't know how much time there is to shake out regressions
before 1.8 but IMO we should go for it (both bug 303620 and this).

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 306064 has been marked as a duplicate of this bug. ***

Comment 23

[Aaron Leventhal]

Fixed on trunk.

Comment 24

[[e-mail]]

This doesn't fix editable menulists. Test case forthcoming.

Comment 25

[David :Bienvenu]

the thunderbird compose window addressing widget is still broken, even with this
patch, as near as I can tell. Have you tried that?

Comment 26

[[e-mail]]

Attachment: Try Alt+T and Alt+M

On old builds both Alt+T and Alt+M work.
On recent builds neither Alt+T or Alt+M work.
On new builds Alt+T works but Alt+M doesn't.

Comment 27

[David :Bienvenu]

never mind, I was somehow out of date in toolkit - all seems ok in mailnews compose

Comment 28

[Mats Palmgren (inactive)]

Attachment: Additional patch rev. 1

I tried to fix the menulist in a similar manner, but it seems its
GetInputField() always returns a null nsIDOMXULTextBoxElement?
Also, I see there is a GetInputField() in nsIDOMXULTreeElement.h too.

So, let's just test visibility for other XUL for now...

This fixes the attached <menulist> testcase, if someone can provide a
small testcase for the tree-element case that would be appreciated.

Comment 29

[[e-mail]]

(In reply to comment #28)
>I tried to fix the menulist in a similar manner, but it seems its
>GetInputField() always returns a null nsIDOMXULTextBoxElement?
>Also, I see there is a GetInputField() in nsIDOMXULTreeElement.h too.
Odd, it looks as if it should work...

>So, let's just test visibility for other XUL for now...
Wouldn't that make the textbox code unnecessary?

>This fixes the attached <menulist> testcase, if someone can provide a
>small testcase for the tree-element case that would be appreciated.
1. Trees can already take focus
2. That element is reserved for future expansion.

Comment 30

[TimoT]

The additional patch seems to fix bug 305833 too.

Comment 31

[[e-mail]]

Seems that people rely on being able to .focus() arbitrary XUL elements...

I've just throught of another case - Suite's compose window has a tree with an
explicit focus style to stop you tabbing to it although you can still use the
access key or F6 to cycle to it if you have attachments.

Comment 32

[Aaron Leventhal]

Comment on attachment 193979 [details] [diff] [review]
Additional patch rev. 1

It's getting a little messy and hard to read. Maybe it needs to be factored out
into a separate method. I'd say the separate requireFocusability variable
somehow doesn't make it more readable.

Comment 33

[Mats Palmgren (inactive)]

Attachment: Cleaned up version, rev. 1

Comment 34

[Aaron Leventhal]

Comment on attachment 193994 [details] [diff] [review]
Cleaned up version, rev. 1

Please remove the else's after returns, since they aren't necessary and are
against Mozilla style.

For the textbox, I recommend you consider saving a couple of lines by using
recursion:
+      nsCOMPtr<nsIContent> input = do_QueryInterface(inputNode);
+      return IsFocusable(aPresShell, input);

Comment 35

[Aaron Leventhal]

Come to think of it, it's strange that of all the XUL elements, we're only
checking focusability on the textbox element, and only visibility on everything
else.

For XUL just check visibility. Anyway, you still have r=

Comment 36

[Mats Palmgren (inactive)]

Attachment: Cleaned up version, rev. 2

With Aaron's suggestions in comment 34.

Comment 37

[Mats Palmgren (inactive)]

(In reply to comment #35)
But the goal here is that we should not focus the element if it is no longer
focusable after the blur handlers have run.
I don't see why should we break that for textboxes just because we can't
implement it correctly for other XUL elements at the moment.

Of course, I would rather see that we fix nsIFrame::IsFocusble(),
'-moz-user-focus' and tabindex to behave in a sane way for XUL.
Then we can just use focusFrame->IsFocusable() here (as we did
in the original checkin for bug 303620).

Comment 38

[Aaron Leventhal]

(In reply to comment #37)
> (In reply to comment #35)
> But the goal here is that we should not focus the element if it is no longer
> focusable after the blur handlers have run.
> I don't see why should we break that for textboxes just because we can't
> implement it correctly for other XUL elements at the moment.
> 
> Of course, I would rather see that we fix nsIFrame::IsFocusble(),
> '-moz-user-focus' and tabindex to behave in a sane way for XUL.
> Then we can just use focusFrame->IsFocusable() here (as we did
> in the original checkin for bug 303620).
> 

Okay, but my point was that if we're going to do a whitelist, then it seems
strange to be skipping all the easy ones like <button> and <checkbox>. At least
it seems worth commenting on in the code.

Comment 39

[Mats Palmgren (inactive)]

(I filed bug 306160 and bug 306162 for XUL problems I stumbled upon)

Comment 40

[Mats Palmgren (inactive)]

Comment on attachment 194021 [details] [diff] [review]
Cleaned up version, rev. 2

(In reply to comment #38)
> Okay, but my point was that if we're going to do a whitelist, then it seems
> strange to be skipping all the easy ones like <button> and <checkbox>. At least
> it seems worth commenting on in the code.

Ok, so let's use nsIContent::IsFocusable instead, which covers all
disabled controls. New patch coming up...

Comment 41

[Mats Palmgren (inactive)]

Attachment: Testcase #2

This triggers the code where we find we are no longer focusable after the blur
handlers have been run. The first testcase used disabled="disabled" which is
confusing since the XUL <textbox> is then focusable but the inner html:input is

not - this caused some of my earlier confusion in this bug, sorry for that.

Comment 42

[Mats Palmgren (inactive)]

Attachment: Cleaned up version, rev. 3

Comment 43

[Adam Guthrie]

*** Bug 306008 has been marked as a duplicate of this bug. ***

Comment 44

[Aaron Leventhal]

Comment on attachment 194038 [details] [diff] [review]
Cleaned up version, rev. 3

Please put in a comment as to why nsIContent->IsFocusable(&tabIndex) with
tabIndex = 0 insteadl if frame->IsFocusable()
I understand it, but the casual observer won't.

Is the :: necessary before ::IsFocusable?

Comment 45

[Chris Beard]

has this landed on the trunk yet for testing?

Comment 46

[Mats Palmgren (inactive)]

(In reply to comment #44)
> Please put in a comment ...

ok, did so

> Is the :: necessary before ::IsFocusable?
> 

Not really, it's to point out that this is a function in global scope rather
than a method. I think it was dbaron who suggested this style to me
and I have used it ever since.

Comment 47

[Mats Palmgren (inactive)]

"Cleaned up version, rev. 3" was checked in on trunk at 2005-08-30 13:22 PDT.

Note that this will only be needed on the branch if bug 303620 goes in there.

-> FIXED

Comment 48

[Asa Dotzler [:asa]]

not applicable for the branch since we're not taking the bug that regressed this
on the trunk.

Comment 49

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061205 Minefield/3.0a1