264164 - Radio and check indicator w/ Classic theme remains hover state look

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Ginn Chen]

I'm using trunk GTK2 build/20041008 on JDS2.
Radio and check indicator w/ Classic theme sometimes has a trail, when mouse
move over it.
It looks as hover state after mouse pointer leaves.

See attached ScreenShot.

I don't have this problem with Mozilla 1.7.

Comment 1

[Ginn Chen]

Attachment: ScreenShot

Comment 2

[Asa Dotzler [:asa]]

Sarah, can you take a look at this and see if it's reproduceable?

Comment 3

[sairuh (rarely reading bugmail)]

Ginn, it's not clear to me what the problem is from looking at the screenshot.

testing this with today's gtk2 nightly build, after I click a radiobutton or
checkbox, when I move the mouse off of the widget, the hover "highlighting"
effect goes away for me. what remains is the the dotted focus ring --but I'd
think that's expected since that's where focus was last (ie, where I last
*clicked* the mouse).

Comment 4

[sairuh (rarely reading bugmail)]

fwiw, I also see the same thing in comment 3 using a gtk1 nightly from
2005-01-02-trunk.

Comment 5

[Ginn Chen]

(In reply to comment #3)
> Ginn, it's not clear to me what the problem is from looking at the screenshot.
> 

All the checkboxes and radios except 'Home page' is in hover state (blue).

Does checkbox/radio changes color with your gnome-theme?
If so, you can just move mouse over them without click, this bug will be reproduced.
With some gnome themes, this bug is not obvious.
I only tested GTK2 build on Linux/Solaris.

Comment 6

[sairuh (rarely reading bugmail)]

Attachment: screenshot using default gnome-theme

I realized that providing a screenshot of what I see might be helpful. :)

Ginn, I'm using the default RedHat gnome-theme, called Bluecurve. (well,
default for fedora core 2.)

Comment 7

[sairuh (rarely reading bugmail)]

Comment on attachment 170312 [details]
screenshot using default gnome-theme

oh, wait a sec --let me get an image showing the focus...

Comment 8

[sairuh (rarely reading bugmail)]

Attachment: screenshot showing focus

when taking this screenshot, I clicked on the "home page" radiobutton which
displays both the dotted focus ring and the paler-grey hover style.

when I moved my mouse away from this radiobutton, the focus ring persisted
(till I clicked elsewhere), but the paler-grey hover style went away (which
seems expected).

Comment 9

[Ginn Chen]

(In reply to comment #8)
> Created an attachment (id=170313) [edit]
> screenshot showing focus
> 
The problem is with bluecurve theme, the radio/checkbox doesn't change when you
hover it.
Try "Ocean Dream" theme. The radio/checkbox will be lighter when you hover it.
Although it's still not very obvious.
I can reproduce this bug with RH9, 20050104 nightly GTK2 build.
I think it's a regression, I can't reproduce it with 20040720 mozilla 1.8a3.

Comment 10

[Ginn Chen]

Attachment: screenshot of RH9 Ocean Dream theme

Please notice that in this screenshot "Search" "Print" checkboxes and "Home
page" "Last page visited" radios are all in hover state.

Only the exact checkbox/radio under mouse cursor should be in hover state.

Comment 11

[Ginn Chen]

a regression caused by
Bug 241070 patch by [e-mail] (Nicolas PENINGUY), r=bryner sr=blizzard
Refactor nsNativeThemeGTK

backing out this patch does solve this problem.

Comment 12

[Ginn Chen]

mozilla/ gfx/ src/ gtk/ nsNativeThemeGTK.cpp 

252       // For XUL checkboxes and radio buttons, the state of the parent
253       // determines our state.
254       if (aWidgetType == NS_THEME_CHECKBOX || aWidgetType == NS_THEME_RADIO ||
255           aWidgetType == NS_THEME_CHECKBOX_LABEL ||
256           aWidgetType == NS_THEME_RADIO_LABEL) {
257         if (aWidgetFlags) {
258           nsIAtom* atom = nsnull;
259 
260           if (aFrame) {
261             nsIContent* content = aFrame->GetContent();
262             if (content->IsContentOfType(nsIContent::eXUL)) {
263               aFrame = aFrame->GetParent();

mozilla/ gfx/ src/ shared/ nsNativeTheme.cpp 

182   if (content->IsContentOfType(nsIContent::eXUL)) {
183     // For a XUL checkbox or radio button, the state of the parent determines
184     // the checked state
185     aFrame = aFrame->GetParent();

duplicates,

I don't know which one should be removed.
Seems we need add #ifdef in mozilla/ gfx/ src/ shared/ nsNativeTheme.cpp,
or we need another refactoring.

Comment 13

[[e-mail]]

The GTK1 hover effects work fine in my standard RH7.3 installation.

Comment 14

[Nicolas PENINGUY]

Hmm, sorry for that... I can reproduce it using the "Ocean Dream" theme.

nsNativeThemeGTK::GetGtkWidgetAndState() is quite complicated, and I'm afraid of
breaking something else. :(

Simple patch restoring the old behaviour is coming...

Comment 15

[Nicolas PENINGUY]

Attachment: Simple fix

Comment 16

[Ginn Chen]

The patch looks good to me.
I really want it get fixed before 1.8b.
It is horrible to Java Desktop System users.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 172213 [details] [diff] [review]
Simple fix

I don't want to refork this method from the shared base class.	Can you figure
out how/why nsNativeTheme::GetContentState gives a different result from the
one we're expecting?

Comment 18

[Ginn Chen]

(In reply to comment #17)
> (From update of attachment 172213 [details] [diff] [review] [edit])
> I don't want to refork this method from the shared base class.	Can you figure
> out how/why nsNativeTheme::GetContentState gives a different result from the
> one we're expecting?
> 
In nsNativeTheme.cpp, there is aFrame = aFrame->GetParent(); for radio/checkbox
in XUL.
But the nsNativeThemeGTK::GetContentState doesn't have it.
For this situation, the GetParent() method is used in nsNativeThemeGTK before
calling GetContentState(), and may have other uses there.

Comment 19

[Ginn Chen]

Attachment: another approach for this bug

Move aFrame = aFrame->GetParent(); out of nsNativeThemeGTK.

Please review it, this bug is very obvious on JDS or debian.

Comment 20

[Ginn Chen]

my patch is not working on trunk now...

Comment 21

[Ginn Chen]

Attachment: another approach for this bug

My last patch was wrong.

This patch works fine, but I'm wondering whether we should check
XULCheckboxRadio and do GetParent() in both GetGtkWidgetAndState() and
GetContentState().

But this bug is really bad to me.

Comment 22

[Eric Marsch]

*** Bug 289767 has been marked as a duplicate of this bug. ***

Comment 23

[Anders Conbere]

*** Bug 301976 has been marked as a duplicate of this bug. ***

Comment 24

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

This bug is also obvious in GNOME Industrial.

Comment 25

[Reed Loden [:reed]]

*** Bug 281228 has been marked as a duplicate of this bug. ***

Comment 26

[Reed Loden [:reed]]

Pulling blocking requests from bug 281228 over to this bug.

Comment 27

[Reed Loden [:reed]]

Pull over the CC'd list from bug 281228 to maybe help get this bug fixed (a patch is waiting to be reviewed).

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 194285 [details] [diff] [review]
another approach for this bug

Ok, I understand now.  I don't really like this patch either, because it assumes (in cross-platform code) that the checkbox-with-spacing binding is being used.  It seems like there's a pretty simple solution though:

1. Adjust aFrame for CHECKBOX_LABEL/RADIO_LABEL
2. Call GetContentState
3. All of the other processing before we start setting aState, but take out the frame adjustment for CHECKBOX_LABEL/RADIO_LABEL

Comment 29

[Reed Loden [:reed]]

*** Bug 317217 has been marked as a duplicate of this bug. ***

Comment 30

[Brian Ryner (not reading)]

Attachment: proposed fix

This fixes it for me.  It adjusts aFrame only for the label case, since GetContentState knows how to do it for the non-label case.  I also reduced the over-wide scope of the |if (aWidgetFlags)| check, since we want to do this regardless of whether aWidgetFlags is true (so we can set aState).

Comment 31

[Ginn Chen]

(In reply to comment #30)
> Created an attachment (id=203863) [edit]
> proposed fix
> 
> This fixes it for me.  It adjusts aFrame only for the label case, since
> GetContentState knows how to do it for the non-label case.  I also reduced the
> over-wide scope of the |if (aWidgetFlags)| check, since we want to do this
> regardless of whether aWidgetFlags is true (so we can set aState).
> 

This patch is not correct. I applied it to a last month's build.
Every checkbox rendered as unchecked state no matter it is checked or not.

Comment 32

[Adam Guthrie]

*** Bug 317851 has been marked as a duplicate of this bug. ***

Comment 33

[Adam Guthrie]

*** Bug 320216 has been marked as a duplicate of this bug. ***

Comment 34

[Ginn Chen]

Attachment: make bryner's patch work

I think bryner had made some mistakes in his patch.
stateFrame is assigned but never used after in his patch.

This patch worked for me now.

Comment 35

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

Comment on attachment 206382 [details] [diff] [review]
make bryner's patch work

ok, great. sorry about the delay.

Comment 36

[Ginn Chen]

Fixed on trunk

Comment 37

[Ginn Chen]

Comment on attachment 206382 [details] [diff] [review]
make bryner's patch work

important for gtk/gtk2 users, affect Firefox on a lot of Linux/Unix distributions

Comment 38

[Ginn Chen]

Checking in gfx/src/gtk/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/Attic/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.64.8.4; previous revision: 1.64.8.3
done

Comment 39

[Philip Withnall (unavailable)]

*** Bug 326147 has been marked as a duplicate of this bug. ***