Teune van Steeg Assignee
	Description • 17 years ago

As far as I know there are two ways to give focus to a text field: clicking in it or tabbing into it.

For number box text fields the focus ring only appears when you click in it. Tabbing into it gives the widget the focused state but the focus ring is not painted. Only when you move your mouse over it will it get the focus ring. Also when the text field is partly covered by a popup menu, the part of the text field that is covered will get a focus ring once the popup menu disappears. 

When the text field has a focus ring, and you tab out of it or you select another text field, the focus ring remains. It will disappear only when you move your mouse over it. Also the covered part of the focus ring will disappear when a popup menu covers part of the text field.

Debugging this showed me that moz_gtk_entry_paint gets called every time focus is gained or lost, and state->focused has the correct value at this point.

	Ian Spence
	Comment 1 • 17 years ago

I'm giving a fix a shot but the compiling won't be done until next Tuesday

	Ian Spence
	Comment 2 • 17 years ago

Wasn't as much a fix as a "breaks everything"

	Teune van Steeg Assignee
	Comment 3 • 17 years ago

I assume that this is because repainting is asked for at nsNativeThemeGTK::WidgetStateChanged which never gets called for focus events in the number box text field.

For painting we get the state from the parent frame, so somehow the focus events should be passed to the text field from the parent frame to get WidgetStateChanged called when it is needed.

	Michael Ventnor
	Comment 4 • 17 years ago

The way this patch fixes the problem is that the focus event is not being received as a box. But if we convert it to using our XUL textbox element then everything seems to run just fine because the XUL textbox element gives off focus events properly. I'm still not sure why the discrepancy.

I haven't figured out a way to wrap autocomplete with this same principle. I really don't know why a textbox element works but an embedded input element doesn't.

	Michael Ventnor
	Comment 5 • 17 years ago

The problem is that the focus event is only being caught at two places: the anonymous element which gave off the focus event, and the binding parent. The problem is that any parent of the element giving off the focus which is still in the XBL binding doesn't get the focus event.

This is my theory which explains why using a XUL textbox works but using an embedded HTML input doesn't. The proper fix is to separate the textboxes from the elements into separate bindings. This is done for the numberbox in the patch above, but fixing this will be more difficult for autocomplete because of all the other elements in the element that takes on the textbox appearance. Does anyone want to try?

	Neil Deakin
	Comment 6 • 17 years ago

Comment on attachment 295174 [details] [diff] [review]
Hackfix for number boxes only, autocomplete still problematic

Several problems:
 - the appearance is broken on at least Windows XP
 - the inputField property returns a textbox instead of an html input
 - the numberbox test fails with an infinite recursion error

Is bug Linux only?

Why does the xul textbox work properly and not the numerbox? The binding <content> for both are almost identical.

	Michael Ventnor
	Comment 7 • 17 years ago

Neil, I gave a theory in comment #5 (but I'm not sure if its correct).

	Teune van Steeg Assignee
	Comment 8 • 17 years ago

> Is bug Linux only?
Yes, only on Linux that particular element is painted as a text field to get the native look (the text field looks attached to the spin buttons)

> Why does the xul textbox work properly and not the numerbox? The binding
> <content> for both are almost identical.
That is because for number boxes and editable drop down menus /autocomplete text boxes we do not use the element itself to paint the text field, but the <hbox> that is a sibling of the extra content (the drop down button or the spin buttons in this case) to get the native look as described above.

	Neil Deakin
	Comment 9 • 17 years ago

Comment on attachment 295300 [details] [diff] [review]
'manually' set or remove focus for the painted element, by using the focus and blur events of the binding


>       <handler event="focus" phase="capturing">
>         <![CDATA[
>           if (!this.hasAttribute('focused')) {
>             if (event.originalTarget != this.inputField)
>               this.inputField.focus();
>+            this.inputField.parentNode.setAttribute('focused', 'true');
>             this.setAttribute('focused','true');
>           }
>         ]]>
>       </handler>
> 
>       <handler event="blur" phase="capturing">
>         <![CDATA[
>+          this.inputField.parentNode.removeAttribute('focused');
>           this.removeAttribute('focused');
>         ]]>
>       </handler>

You can just use xbl inheritance for the menulist attribute, no?

Otherwise, this seems ok. But ask for an sr as well.

	Teune van Steeg Assignee
	Comment 10 • 17 years ago

Taking over review

> You can just use xbl inheritance for the menulist attribute, no?
Yes indeed. That is done with the current patch.

> Otherwise, this seems ok. But ask for an sr as well.
I assume that is for the GTK native theme changes, so I request sr from roc.

	Reed Loden [:reed]
	Comment 11 • 17 years ago

Comment on attachment 296625 [details] [diff] [review]
'manually' set or remove focus for the painted element (v2)

Fix an issue with focus events that is blocking other work.

	Reed Loden [:reed]
	Comment 12 • 17 years ago

Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.136; previous revision: 1.135
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.106; previous revision: 1.105
done
Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.34; previous revision: 1.33
done
Checking in toolkit/content/widgets/numberbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/numberbox.xml,v  <--  numberbox.xml
new revision: 1.6; previous revision: 1.5
done

	Dão Gottwald [:dao]
	Comment 13 • 17 years ago

Why didn't you use xbl inheritance in autocomplete.xml and numberbox.xml?

	Teune van Steeg Assignee
	Comment 14 • 17 years ago

IIRC, because the focus attribute is not set for those widgets.

	Dão Gottwald [:dao]
	Comment 15 • 17 years ago

They extend the textbox binding, which does set the attribute.

	Teune van Steeg Assignee
	Comment 16 • 17 years ago

(In reply to comment #15)
> They extend the textbox binding, which does set the attribute.
> 
Apparently not, I just tested it again

	Teune van Steeg Assignee
	Comment 17 • 17 years ago

The test I did was wrong, sorry about that. I will revert the autocomplete change and make the focused attribute inherited in bug 414748. I will file a new bug to do the numberbox correctly.