GTK Toolkit arrows instead of stock icons for find toolbar

RESOLVED FIXED in mozilla2.0b3

Status

()

defect
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: micmon, Assigned: ispence)

Tracking

Trunk
mozilla2.0b3
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

12 years ago
Posted image 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.
Reporter

Comment 2

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

Updated

11 years ago
Assignee: nobody → twanno
Depends on: 416003

Updated

11 years ago
Status: NEW → ASSIGNED

Updated

11 years ago
Component: Find Toolbar / FastFind → Widget: Gtk
Product: Firefox → Core
QA Contact: fast.find → gtk

Comment 3

11 years ago
This patch requires bug 416003 to be fixed and bug 416868 too.
Attachment #305358 - Flags: review?(frnchfrgg-mozbugs)

Updated

11 years ago
Depends on: 416868
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 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.
I may take over this bug if Teune's still away/busy.

Comment 7

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

Reporter

Comment 8

11 years ago
_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?
I've absolutely no free time until thursday evening (French time), sorry.
Reporter

Comment 10

11 years ago
Any chance this could still make it into 3.0?
Not if you count on me, I have no time currently.
Assignee

Comment 12

11 years ago
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)
Assignee: twanno → ispence
Attachment #305358 - Attachment is obsolete: true
Attachment #320317 - Flags: superreview?(roc)
Attachment #320317 - Flags: review?(roc)
Attachment #305358 - Flags: review?(frnchfrgg-mozbugs)
Assignee

Updated

11 years ago
Attachment #320317 - Flags: review?(gavin.sharp)
Assignee

Comment 13

11 years ago
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.
Assignee

Comment 15

11 years ago
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

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

Comment 17

11 years ago
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)
Assignee

Updated

11 years ago
Blocks: 415436
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.
Attachment #320317 - Flags: superreview?(roc)
Attachment #320317 - Flags: review?(roc)
Attachment #320317 - Flags: review?(gavin.sharp)
Reporter

Comment 19

9 years ago
...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 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!
Attachment #320317 - Flags: review+
Assignee

Comment 21

9 years ago
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
Reporter

Comment 22

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

Comment 23

9 years ago
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 on attachment 458217 [details] [diff] [review]
Patch updated for latest central

I already reviewed it. You can land this.
Attachment #458217 - Flags: review+
(In reply to comment #24)
> You can land this.

The theme stuff still needs review.
Reporter

Comment 26

9 years ago
(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
Attachment #458217 - Flags: review?(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 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.
Attachment #458217 - Flags: ui-review?(faaborg)
Attachment #458217 - Flags: review?(dao)
Attachment #458217 - Flags: review+
Reporter

Comment 29

9 years ago
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.
Attachment #458217 - Flags: ui-review?(faaborg) → ui-review+
Attachment #459007 - Flags: approval2.0?
Comment on attachment 459007 [details] [diff] [review]
Patch updated again (Dao's u-r)

a=beltzner
Attachment #459007 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/f611fdc2b730
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
You need to log in before you can comment on or make changes to this bug.