Closed Bug 385686 Opened 14 years ago Closed 14 years ago

No focus indicator for radios/checkboxes/textboxes on Linux

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

()

Details

(Keywords: access, regression)

Attachments

(1 file, 4 obsolete files)

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.
Flags: blocking1.9?
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #269616 - Flags: superreview?(roc)
Attachment #269616 - Flags: review?(roc)
Attached patch Patch 1.1 (obsolete) — Splinter Review
This gets rid of some unwanted newlines that were added in the last patch.
Attachment #269616 - Attachment is obsolete: true
Attachment #269617 - Flags: superreview?(roc)
Attachment #269617 - Flags: review?(roc)
Attachment #269616 - Flags: superreview?(roc)
Attachment #269616 - Flags: review?(roc)
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?
Er... I forgot the testcase.
(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 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
Attachment #269617 - Flags: superreview?(roc)
Attachment #269617 - Flags: superreview+
Attachment #269617 - Flags: review?(roc)
Attachment #269617 - Flags: review+
Attached patch Patch 2 (obsolete) — Splinter Review
Patch to fix Roc's comment. I'll need someone to check this in for me.
Attachment #269617 - Attachment is obsolete: true
Whiteboard: [checkin needed]
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Thanks for fixing this!
Attached patch One line post-fix (obsolete) — Splinter Review
Ouch, I missed something quite important. Copy & paste is usually not a good idea when programming widget code.
Attachment #269826 - Flags: superreview?(roc)
Attachment #269826 - Flags: review?(roc)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In that case, can you please check this one in when you reland? It contains the trivial oversight fix.
Attachment #269630 - Attachment is obsolete: true
Attachment #269826 - Attachment is obsolete: true
Attachment #269826 - Flags: superreview?(roc)
Attachment #269826 - Flags: review?(roc)
Someone should reland this later when its effects won't confound my Tp measurements...
Whiteboard: [checkin needed]
Relanded, with the extra fix.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Verified.  Thanks again for fixing this!

One question.  The focus outline seems to be 1px off center.  Anything we can do about that?
(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.
Status: RESOLVED → VERIFIED
> Looks fine here, which theme are you using?

No idea.  Whatever FC4 defaults to, probably, since I don't actually use GNOME...
Flags: blocking1.9?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.