412773 - GTK Toolkit arrows instead of stock icons for find toolbar

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Michael Monreal [:micmon]]

Attachment: Current look

Perhaps the last place where native toolkit arrows would be nice:

The find bar currently uses gtk stock icons. Other GTK apps which have such a bar use gtk toolkit arrows instead. Looks very clean IMHO and improves platform integration.

Comment 1

[Michael Monreal [:micmon]]

Attachment: Native (gnome help browser, next button has mouse hovering)

Comment 2

[Michael Monreal [:micmon]]

This is probably very easy to do now that we have those native look arrows nearly everywhere... can someone please implement it?

Comment 3

[Teune van Steeg]

Attachment: Draw native arrows for the arrows in the find toolbar

This patch requires bug 416003 to be fixed and bug 416868 too.

Comment 4

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 305358 [details] [diff] [review]
Draw native arrows for the arrows in the find toolbar

I just skim through the patch for now since I don't have a lot of time...

First, I'm not qualified to review the skin part of the patch. I didn't see anything wrong, but you need a review from someone else (rflint, enndeakin ?)

Second, I see you still use gButtonArrowWidget which is created as an arrow pointing down; are you sure that the requisition will be always good regardless of the type of arrow you want (that is the arrow requisition is a square), and that every engine will draw the arrow correctly (this one is almost certainly a safe bet) ?

If you are sure that it's good, add a comment in the code, please.

I didn't find glaring implementation errors, I'll do a more thorough review soon.

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 305358 [details] [diff] [review]
Draw native arrows for the arrows in the find toolbar

>             aWidgetType == NS_THEME_TOOLBAR_DUAL_BUTTON ||
>             aWidgetType == NS_THEME_TOOLBAR_BUTTON_DROPDOWN ||
>             aWidgetType == NS_THEME_DROPDOWN ||
>             aWidgetType == NS_THEME_DROPDOWN_BUTTON) {
>-          if (aWidgetType == NS_THEME_TOOLBAR_BUTTON_DROPDOWN)
>-            aFrame = aFrame->GetParent();
>-
>           PRBool menuOpen = CheckBooleanAttr(aFrame, nsWidgetAtoms::open);
>           aState->depressed = IsCheckedButton(aFrame) || menuOpen;
>           // we must not highlight buttons with open drop down menus on hover.
>           aState->inHover = aState->inHover && !menuOpen;
>         }
> 

What is this for; is that related to this bug ? I'm puzzled.

Apart from that (and previous comments), I think the patch is good. I won't set review+ until you explain me why you can use an always down pointing arrow widget, though.

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

I may take over this bug if Teune's still away/busy.

Comment 7

[Teune van Steeg]

(In reply to comment #6)
> I may take over this bug if Teune's still away/busy.
> 
Yes please do, I'm sorry but I'm really busy atm, and this bug will probably be fixed much faster when you're working on it.

To answer your comments:
(In reply to comment #4)
> Second, I see you still use gButtonArrowWidget which is created as an arrow
> pointing down; are you sure that the requisition will be always good regardless
> of the type of arrow you want (that is the arrow requisition is a square), and
> that every engine will draw the arrow correctly (this one is almost certainly a
> safe bet) ?
>
Yes, this will always be correct: the requisition of the arrow is always the same for different arrow types, it is the sum of MIN_ARROW_SIZE defined in gtkarrow.c (always a square) and the padding (MISC->xpad / MISC->ypad) which can not be different for different arrow types AFAIK.

(In reply to comment #5)
> (From update of attachment 305358 [details] [diff] [review])
> >             aWidgetType == NS_THEME_TOOLBAR_DUAL_BUTTON ||
> >             aWidgetType == NS_THEME_TOOLBAR_BUTTON_DROPDOWN ||
> >             aWidgetType == NS_THEME_DROPDOWN ||
> >             aWidgetType == NS_THEME_DROPDOWN_BUTTON) {
> >-          if (aWidgetType == NS_THEME_TOOLBAR_BUTTON_DROPDOWN)
> >-            aFrame = aFrame->GetParent();
> >-
> >           PRBool menuOpen = CheckBooleanAttr(aFrame, nsWidgetAtoms::open);
> >           aState->depressed = IsCheckedButton(aFrame) || menuOpen;
> >           // we must not highlight buttons with open drop down menus on hover.
> >           aState->inHover = aState->inHover && !menuOpen;
> >         }
> > 
> 
> What is this for; is that related to this bug ? I'm puzzled.
> 
A few lines above this, the following code is added:
> stateFrame = aFrame = aFrame->GetParent(); 
That is necessary to get the correct disabled state and eventState for TOOLBAR_BUTTON_DROPDOWN as well as the new arrows, from their parents. So aFrame is now already set to aFrame->GetParent().

Comment 8

[Michael Monreal [:micmon]]

_FrnchFrgg_, I'M currently going over all my open bugs and I noticed that all the dependencies for this bug have been solved by now. Any chance you will be able to get this in for 3.0?

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

I've absolutely no free time until thursday evening (French time), sorry.

Comment 10

[Michael Monreal [:micmon]]

Any chance this could still make it into 3.0?

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Not if you count on me, I have no time currently.

Comment 12

[Ian Spence]

Attachment: Adds 4 directions of native arrows for people to use as they please. Also fixes this bug

Finally had some free time and since it seems like today is a cutoff point, wanted to get this done.

This patch also improves the look for bug 419617 (without technically solving it)

Comment 13

[Ian Spence]

Attachment: Screenshot of the patches fix for places

Comment 14

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

This seems more suited to stock icons. There are no equivalent stock icons to these themed arrows?

BTW it's definitely too late to take this for Firefox 3.

Comment 15

[Ian Spence]

We currently use stock back and forward images (see attachment 297534 [details])
However, in the only example we have of a similar find bar (Epiphany), the gtk widget arrows are used.

Comment 16

[Michael Ventnor]

IMO I think stock icons look better, plus it would fit better with other DEs.

Comment 17

[Michael Monreal [:micmon]]

I guess the "stock icons look better" part is very subjective (I don't really have any opinion regarding this but I am used to see the toolkit < > in other apps).

Well, even if this is not changed for the find bar, having the 4 direction arrows in for addon themes would be nice. Also, while not perfect, the add bookmark dialog looks a lot better as shown above compared to the broken look it has ATM (though we should look for a better solution for FX3+1)

Comment 18

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

Comment on attachment 320317 [details] [diff] [review]
Adds 4 directions of native arrows for people to use as they please. Also fixes this bug

We don't seem to have a compelling reason to take this at the moment. If someone comes up with one, we can revive the patch --- there's probably nothing wrong with the code.

Comment 19

[Michael Monreal [:micmon]]

...years later...

>> We don't seem to have a compelling reason to take this at the moment. 
>> If someone comes up with one, we can revive the patch --- there's 
>> probably nothing wrong with the code.

GNOME has moved to a "no icons on buttons" default look and Fx4 respects this setting... on all buttons, except the find bar buttons. This looks weird.

I guess we could make those buttons play by the GNOME rules but that would mean having text labels ONLY I guess (done this way by Tomboy notes btw, I don't really like it). Reviving the patch and using the "<" and ">" toolkit arrows would solve the problem in a more elegant way I think.

Comment 20

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

Comment on attachment 320317 [details] [diff] [review]
Adds 4 directions of native arrows for people to use as they please. Also fixes this bug

okay then!

Comment 21

[Ian Spence]

Michael, can you take this bug over? I haven't had a source base for a couple years, so I'm not able to verify if this patch would still apply smoothly, since it's quite possible the source has changed a bit in 2 years.

Honestly, I don't even remember if this is the part where I request sr or not

Comment 22

[Michael Monreal [:micmon]]

Ian, the patch does in fact not apply anymore but I should not have a lot of problems getting it to work again.

Comment 23

[Michael Monreal [:micmon]]

Attachment: Patch updated for latest central

Turns out nearly nothing changed during those years so this was trivial. Patch tested and does what it is supposed to do.

Do I have to ask for review now?

Comment 24

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

Comment on attachment 458217 [details] [diff] [review]
Patch updated for latest central

I already reviewed it. You can land this.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> You can land this.

The theme stuff still needs review.

Comment 26

[Michael Monreal [:micmon]]

(In reply to comment #25)
> The theme stuff still needs review.

So the correct approach is to request ui-request from (according to 1), you? :)  

[1] http://www.mozilla.org/projects/firefox/review.html

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Created attachment 297536 [details]
> Native (gnome help browser, next button has mouse hovering)

The help browser doesn't seem to have these buttons anymore.

(In reply to comment #19)
> >> We don't seem to have a compelling reason to take this at the moment. 
> >> If someone comes up with one, we can revive the patch --- there's 
> >> probably nothing wrong with the code.
> 
> GNOME has moved to a "no icons on buttons" default look and Fx4 respects this
> setting... on all buttons, except the find bar buttons. This looks weird.

We respect it for buttons but generally not for toolbarbuttons. That matches the intent of the GNOME setting, I think.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 458217 [details] [diff] [review]
Patch updated for latest central

>+.expander-up > hbox {
>+	-moz-appearance: button-arrow-up;
> }
...
>+.expander-down > hbox {
>+  -moz-appearance: button-arrow-down;
> }

Use .button-box instead of hbox and replace the tab with two spaces.

The findbar change should get ui-reviewed.

Comment 29

[Michael Monreal [:micmon]]

Attachment: Patch updated again (Dao's u-r)

The find bar may really be a corner case. Personally, I see it as a place which does *not* fall under the "use button icons" category... historicly, this has been a dialog window where "previous" and "next" were real (= no icons)
buttons.

In the end, doing what the official GNOME web browser (Epiphany) does can't be
a bad thing, integration wise.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 459007 [details] [diff] [review]
Patch updated again (Dao's u-r)

a=beltzner

Comment 31

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f611fdc2b730