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.
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
Created attachment 269617 [details] [diff] [review] Patch 1.1 This gets rid of some unwanted newlines that were added in the last patch.
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
Created attachment 269630 [details] [diff] [review] Patch 2 Patch to fix Roc's comment. I'll need someone to check this in for me.
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
Thanks for fixing this!
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.
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.
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.
Someone should reland this later when its effects won't confound my Tp measurements...
Relanded, with the extra fix.
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.
> Looks fine here, which theme are you using? No idea. Whatever FC4 defaults to, probably, since I don't actually use GNOME...
10 years ago
10 years ago