If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

No focus indicator for radios/checkboxes/textboxes on Linux

VERIFIED FIXED

Status

()

Core
Widget: Gtk
--
major
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: bz, Unassigned)

Tracking

({access, regression})

Trunk
x86
Linux
access, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

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?

Comment 1

10 years ago
Created attachment 269616 [details] [diff] [review]
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
Attachment #269616 - Flags: superreview?(roc)
Attachment #269616 - Flags: review?(roc)

Comment 2

10 years ago
Created attachment 269617 [details] [diff] [review]
Patch 1.1

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.

Comment 5

10 years ago
(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+

Comment 7

10 years ago
Created attachment 269630 [details] [diff] [review]
Patch 2

Patch to fix Roc's comment. I'll need someone to check this in for me.
Attachment #269617 - Attachment is obsolete: true

Updated

10 years ago
Whiteboard: [checkin needed]

Comment 8

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Thanks for fixing this!

Comment 10

10 years ago
Created attachment 269826 [details] [diff] [review]
One line post-fix

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 → ---

Comment 12

10 years ago
Created attachment 269946 [details] [diff] [review]
Patch for (re)checkin

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
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

10 years ago
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?

Comment 16

10 years ago
(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.

Updated

10 years ago
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?
Depends on: 434100
You need to log in before you can comment on or make changes to this bug.