385686 - No focus indicator for radios/checkboxes/textboxes on Linux

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load testcase in URL field
2)  Tab around
3)  Try to tell when the checkbox and radio are focused

EXPECTED RESULTS: Focus outlines appear when you tab into the widget, as they do without native theming.

ACTUAL RESULTS: Focus outlines do not appear; no way to tell where the focus is.

Using forms with keyboard only really sucks as a result.

Comment 1

[Michael Ventnor]

Attachment: Patch

Some of the focus targetting code was designed for XUL, this fixes that, so radio buttons get focus state properly again. It also gets rid of the GTK_PRELIGHT bandaid and replaces it with focus rings for radio buttons and checkboxes

Comment 2

[Michael Ventnor]

Attachment: Patch 1.1

This gets rid of some unwanted newlines that were added in the last patch.

Comment 3

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

I don't understand how this patch fixes the bug. At best, it just sets aState->focused to false more often than it used to, so how can that make focus show up?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er... I forgot the testcase.

Comment 5

[Michael Ventnor]

(In reply to comment #3)
> I don't understand how this patch fixes the bug. At best, it just sets
> aState->focused to false more often than it used to, so how can that make focus
> show up?

Actually its vice versa. The code in nsNativeThemeGTK that gets changed only really works properly in XUL, so by making it only apply in XUL the value that is set earlier is kept (which is correct for HTML).

But, by adding focus rings for checkboxes and radio buttons, we'd get a ring around both the checkbox or radio and its label, instead of the earlier behaviour of just the label in XUL. So the little hack at the bottom will restore that behaviour in XUL, and only in XUL.

I use Linux too so I know this works. Trust me ;)

Comment 6

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

Comment on attachment 269617 [details] [diff] [review]
Patch 1.1

Okay, now this makes sense. The key observation is that IsFocused is really XUL-specific.

For similar reasons you should put the code below for scrollbar thumbs, buttons and menuitems into your if (XUL) conditional. with that, r+sr=roc

Comment 7

[Michael Ventnor]

Attachment: Patch 2

Patch to fix Roc's comment. I'll need someone to check this in for me.

Comment 8

[Michael Wu [:mwu]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.24; previous revision: 1.23
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.95; previous revision: 1.94
done

Comment 9

[Boris Zbarsky [:bzbarsky]]

Thanks for fixing this!

Comment 10

[Michael Ventnor]

Attachment: One line post-fix

Ouch, I missed something quite important. Copy & paste is usually not a good idea when programming widget code.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I backed this out; it or bug 384836 caused the Tp2 regression in bug 385957; we should reland them separately when the tree is quiet.

Comment 12

[Michael Ventnor]

Attachment: Patch for (re)checkin

In that case, can you please check this one in when you reland? It contains the trivial oversight fix.

Comment 13

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

Someone should reland this later when its effects won't confound my Tp measurements...

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Relanded, with the extra fix.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Verified.  Thanks again for fixing this!

One question.  The focus outline seems to be 1px off center.  Anything we can do about that?

Comment 16

[Michael Ventnor]

(In reply to comment #15)
> Verified.  Thanks again for fixing this!
> 
> One question.  The focus outline seems to be 1px off center.  Anything we can
> do about that?
> 

Looks fine here, which theme are you using? There isn't much I can do since I just made the focus line draw around the entire rect, in order to ensure the outline fits around every theme's checkbox.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Looks fine here, which theme are you using?

No idea.  Whatever FC4 defaults to, probably, since I don't actually use GNOME...