Closed Bug 409388 Opened 12 years ago Closed 12 years ago

Number box text fields seem not to get repainted on focus events

Categories

(Core :: Widget: Gtk, defect, major)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: twanno, Assigned: twanno)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
I'm giving a fix a shot but the compiling won't be done until next Tuesday
Wasn't as much a fix as a "breaks everything"
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.
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.
Attachment #295174 - Flags: review?(rflint)
Attachment #295174 - Flags: review?(rflint) → review?(enndeakin)
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?
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.
Attachment #295174 - Flags: review?(enndeakin) → review-
Neil, I gave a theory in comment #5 (but I'm not sure if its correct).
> 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.
Attachment #295300 - Flags: review?(enndeakin)
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.
Attachment #295300 - Flags: review?(enndeakin) → review+
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.
Assignee: nobody → twanno
Attachment #295300 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #296625 - Flags: superreview?(roc)
Attachment #296625 - Flags: review+
Attachment #296625 - Flags: superreview?(roc) → superreview+
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.
Attachment #296625 - Flags: approval1.9?
Attachment #296625 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Why didn't you use xbl inheritance in autocomplete.xml and numberbox.xml?
IIRC, because the focus attribute is not set for those widgets.
They extend the textbox binding, which does set the attribute.
(In reply to comment #15)
> They extend the textbox binding, which does set the attribute.
> 
Apparently not, I just tested it again
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.
Depends on: 416379
You need to log in before you can comment on or make changes to this bug.