Closed Bug 404512 Opened 17 years ago Closed 7 years ago

Improve GTK button pressed state

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: micmon, Assigned: ispence)

References

Details

Attachments

(2 files, 3 obsolete files)

Shot
(deleted), image/png
Details
Now with the ability to distinguish between up/down and right/left
(deleted), patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071022 Ubuntu/7.10 (gutsy) Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112004 Minefield/3.0b2pre

GTK shifts the icon one pixel to the right and to the bottom when a button is pressed. This gives additional feedback to the user. Firefox should copy this behaviour for GTK builds.

Reproducible: Always
Attached image Shot (deleted) — 
This shows how a button changes from hover to pressed in plain GTK.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch makes pressed buttons look native (obsolete) (deleted) — — Splinter Review 
Draws a depressed GtkToggleButton, when a button is open/checked.
Assignee: nobody → twanno
Status: NEW → ASSIGNED
Attachment #291095 - Flags: superreview?(roc)
Attachment #291095 - Flags: review?(roc)
Component: OS Integration → Widget: Gtk
Product: Firefox → Core
QA Contact: os.integration → gtk
Version: unspecified → Trunk
How does this move the icon? Normally we do that by changing the padding inside buttons.

Document what "depressed" means where you declare it, so people know how it differs from "active" in particular.
Comment on attachment 291095 [details] [diff] [review]
makes pressed buttons look native

Oops, wrong bug... (confusing titles)
I meant to attach this patch to bug 404514; I'll attach an improved version of this patch there later .
Attachment #291095 - Attachment is obsolete: true
Attachment #291095 - Flags: superreview?(roc)
Attachment #291095 - Flags: review?(roc)
After looking at this more, I'm kinda stumped. This should go in ::GetWidgetPadding but we would need the state, which isn't available.  If there was some way to get the state into the padding, then this patch would be trivial
Can't you fix this by changing the padding in the theme?
The offset is a gtk setting. Some themes don't have any offset, others have a few pixels. The offset setting also allows for moving to the right but not down and vice versa
Right now I think we don't have any support for widget state changes changing native theme metrics. Adding that support would be quite a bit of work and I'd prefer not to take it on for FF3 at this stage. Can we pick one "most common"/"least objectionable" value and just use that?
I was able to make some strides, although nothing amazing.

I currently have buttons in the bookmark toolbar dropping the correct amount, but this leads to the button expanding since I can't give the negative multiple of padding on the other side.

If there is no way to offset the added padding with the removal on the other side, then I'll just have to go the "most common value" route.
I've actually gotten this pretty much done. Turns out the negative padding worked fine.

My only issue right now is that the toolbar buttons are not effected.

This is weird because every other button is effected correctly. I'm going to list what works, so maybe someone will see a pattern and tell me what I'm messing up.

Work:
1. All normal buttons
2. Buttons in the bookmarks toolbar
3. The toolbar buttons in the error console
4. TreeView header cells

Don't work:
1. The important toolbar buttons (back, forward, reload, etc)
Maybe you could post a preliminary patch, we could look at
Assignee: twanno → ispence
Status: ASSIGNED → NEW
Attached patch Patch for viewing (obsolete) (deleted) — — Splinter Review 
There it is. It's rather straight forward.

The part that checks if it's an html button is untested, I wrote it a minute ago and for some reason I'm now getting a compile error about ambiguity with a function I don't even use.

Logic:
Check if it's a button
If it is and it's an XUL button, look up it's state.
If the button is depressed, or it's active and hovered, add positive padding to the top and left, remove same padding from bottom and right.
Comment on attachment 293684 [details] [diff] [review]
Patch for viewing

Your problem lies here:
>   if (aWidgetType == NS_THEME_BUTTON_FOCUS ||
>       aWidgetType == NS_THEME_TOOLBAR_BUTTON ||
>       aWidgetType == NS_THEME_TOOLBAR_DUAL_BUTTON) {
>     aResult->SizeTo(0, 0, 0, 0);
>     return PR_TRUE;
>   }
> 
>   return PR_FALSE;

This sets it back to (0, 0, 0, 0) for NS_THEME_TOOLBAR_BUTTON.
(In reply to comment #13)
> (From update of attachment 293684 [details] [diff] [review])
> Your problem lies here:
> >   if (aWidgetType == NS_THEME_BUTTON_FOCUS ||
> >       aWidgetType == NS_THEME_TOOLBAR_BUTTON ||
> >       aWidgetType == NS_THEME_TOOLBAR_DUAL_BUTTON) {
> >     aResult->SizeTo(0, 0, 0, 0);
> >     return PR_TRUE;
> >   }
> > 
> >   return PR_FALSE;
> 
> This sets it back to (0, 0, 0, 0) for NS_THEME_TOOLBAR_BUTTON.
> 

I'm sorry, of course I'm wrong here: I looked over those return statements.


(In reply to comment #10)

> Work:
> 1. All normal buttons
> 2. Buttons in the bookmarks toolbar
> 3. The toolbar buttons in the error console
> 4. TreeView header cells
> 
> Don't work:
> 1. The important toolbar buttons (back, forward, reload, etc)
> 

The padding for at least TreeView header cells and normal buttons is taken care of with CSS. Maybe it is for the other buttons too, and not for the important toolbar buttons.
Turns out it was some css defined. That css is removed since its only purpose was to make sure buttons were at least 57px wide. I don't know the reasoning behind it (32px icons + 25px?), but GTK paints buttons at the width they happen to be.  If the contents of the button causes it to be 3px wide, you get a 3px button. The only thing gtk does do is use an ellipsis to control max-width, but that is beyond the scope of this bug.
Attachment #293684 - Attachment is obsolete: true
Attachment #293759 - Flags: superreview?(roc)
Attachment #293759 - Flags: review?(roc)
x-axis and y-axis movement was reversed.
Attachment #293759 - Attachment is obsolete: true
Attachment #293767 - Flags: superreview?(roc)
Attachment #293767 - Flags: review?(roc)
Attachment #293759 - Flags: superreview?(roc)
Attachment #293759 - Flags: review?(roc)
Looks good but why exclude HTML buttons?
It causes reflow of the page if the button has been styled by css
I don't understand. Does it work for HTML buttons or not?
Originally I had it working for HTML buttons. It worked perfectly when the button was not styled by css, but when it was, the button got bigger when it was clicked, usually causing reflow. So I disabled it for HTML buttons. Thus with this patch, HTML buttons act as they always have, but XUL buttons get the nice depressed effect.
Getting bigger would have been a bug, right? Did you try to fix that directly?

Any change to the button padding will cause reflow, that's not a problem, and I think reflowing the button for every button press and release is fine. But if the actual button size changes then I think that would be a nasty-looking bug.
I was actually thinking (well, probably rationalizing why not to fix it) that it's probably best that the effect isn't used for HTML buttons.

Consider the case:

<input type="button"
       style="background: #F00; border: 1px solid #000;
              padding: 2px; color: #FFF;" />

The designer will expect a rectangle with 2px padding. Since we want identical rendering across platforms (especially for simple boxes), we'd need to give him that rectangle.

Let's say the GTK theme calls for the button contents to drop by 3px and move to the right 4px. I either make the box bigger or I paint the contents of the button partly outside of the button.

This is only my opinion, but I think it'd be weird replicating the actions of GTK buttons when we aren't replicating the appearance of the button (In other words, the designer went through a lot of work to make sure all buttons looked the same, so it kinda makes sense to have them act the same).
In that case we automatically disable native styling for that button because of its CSS styling, don't we?
Well I happen to visit a site with some weird css so this is why I'm being hesitant.

The buttons on these sites are styled only on hover.

Does IsWidgetStyled take into consideration state?
IsWidgetStyled looks at the current style, so yeah.
Well I've given up on trying to fix the hover issue after I noticed that I wasn't causing it.

I'm beginning to think it'd be best to disable the move for html elements.
Firefox already has CSS that causes a similar (but not perfect) movement when activated, and I can't guarantee that I won't move the layout when someone sets the padding too low.
(In reply to comment #20)
> [...] It worked perfectly when the
> button was not styled by css, but when it was, the button got bigger when it
> was clicked, usually causing reflow
> 
Testing showed me that the following happens with a button styled on hover:
(style rule: button:hover { background-color: green;})

In initial drawing the padding is 0px (from GetWidgetPadding). 

When you subsequently hover over the button, native theming is removed and the padding values change to the padding defined in forms.css: 0px 6px 0px 6px (DOMi, computed style), but the new padding is not updated for the visual representation of the button (the size does not change).

Then when you press the button, the button grows in size according to the new padding values. 

At the moment you release the button and move your mouse out, the button is natively themed again, its padding is updated to 0px (DOMi), but the size does not change back.

So to prevent this from happening, I think the initial padding values of native themed buttons should be the same as defined in CSS if that is possible. And those padding values need to be updated according to the child_displacement values when the button is pressed.
I noticed it works only on websites...

But what's strange (for Nodoka-GIT theme) - native GTK buttons have more top padding, while Firefox adds more left padding.
> GTK shifts the icon one pixel to the right and to the bottom when a button is
pressed.

One or more, depending on GTK theme.
I noticed issue when dragging a bookmark: http://www.vimeo.com/720341
That has nothing to do with this bug
Attachment #293767 - Flags: superreview?(roc)
Attachment #293767 - Flags: review?(roc)
Issue with sidebar's close button: http://www.vimeo.com/720385
With tabbar's dropdown button: http://www.vimeo.com/720397

PS: There is OGG video available to download. I've used gtk-recordmydesktop.
Nothing? It's issue with pressed state... Maybe not drawing it, but with this state.
Actually, what you are dragging is just an image, and therefore has nothing to do with this bug.
I filed another bugs. Ignore my comments with links.
No longer relevant with Photon theme.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: