405210 - Improve look of location bar dropdown button

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Michael Monreal [:micmon]]

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/2007112404 Minefield/3.0b2pre

Bug 398020 greatly improved the look of the url and search entries in the location bar. However, the button on the right side still used the old style and draws on top of the focus ring sometimes.

The best thing would be to implement a special -moz-appearance for such a button. I will attach a mockup how it should look like.

Reproducible: Always

Comment 1

[Michael Monreal [:micmon]]

Attachment: Mockup

Comment 2

[Michael Monreal [:micmon]]

Can something like this be done for FF3 or do we need to find some clever CSS hack to make it look as nice as possible?

Comment 3

[Michael Ventnor]

I think it would be hard, I still haven't found a way to draw just that button in either the documentation or GTK source code.

Comment 4

[Teune van Steeg]

(In reply to comment #3)
> I think it would be hard, I still haven't found a way to draw just that button
> in either the documentation or GTK source code.
> 

I have been looking at this too, this is what I have come up with thus far:
http://pastebin.mozilla.org/273353

It looks good for normal editable drop down menus, but it doesn't have the correct size.

You may take this, and work on it as you wish

Comment 5

[Teune van Steeg]

(In reply to comment #4)
> You may take this, and work on it as you wish

Never mind I'm working on this :), I have most of it working, some minor css tweaks are still needed.

Comment 6

[Teune van Steeg]

Attachment: styles editable drop down buttons - including autocomplete dropmarkers - natively

This patch introduces a focus ring painting problem in a very visible place: the location bar. This is the same problem as described in bug 409388. So that bug should probably be fixed first before this fix can go in.

Comment 8

[u294409]

I like the mockup.

Comment 9

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

Comment on attachment 294215 [details] [diff] [review]
styles editable drop down buttons - including autocomplete dropmarkers - natively

You may want to delay checking this in until that focus ring bug is fixed...

Comment 10

[Michael Ventnor]

Yeah don't check this in, the focus ring bug is pretty serious.

Comment 11

[Reed Loden [:reed]]

Bug 409636 changed toolkit/themes/[wp]instripe/global/autocomplete.css, so the new gnomestripe autocomplete.css needs the same change.

Comment 12

[u294409]

Hmmm, it looks like ComboBoxEntry GTK widget, but autocomplete elements are flat (sic!).

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 294215 [details] [diff] [review]
styles editable drop down buttons - including autocomplete dropmarkers - natively

>+.autocomplete-history-dropmarker {
>+  -moz-appearance: none !important;
>+  height: 20px;
>+  margin: 0px;
>+  border-top: none !important;
>+  border-bottom: none !important;
>+  border-left: 1px solid !important;
>+  -moz-border-left-colors: ThreeDShadow !important;
>+  border-right: none !important;
>+  padding: 0 !important;
>+  list-style-image: url("chrome://global/skin/icons/autocomplete-dropmark-arrow.png") !important;
>+  -moz-image-region: rect(0px, 16px, 20px, 0px) !important;
>+}

Remove this?
Without enablehistory, the dropmarker won't be displayed.

>+.autocomplete-history-dropmarker[enablehistory="true"] {
>+  -moz-appearance: menulist-button !important;
>+  -moz-binding: url("chrome://global/skin/globalBindings.xml#history-dropmarker")

What do you need the binding for?

Comment 14

[u294409]

How to test patched-behaviour?

Comment 15

[Teune van Steeg]

Attachment: 294215: styles editable drop down buttons - including autocomplete dropmarkers - natively (v2)

addresses comments #11 and #13.
This also changes the minimum size of the arrows to be 15px instead of 11px as according to the GTK spec, and it makes use of the arrow-scaling style property to scale the arrow.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 295864 [details] [diff] [review]
294215: styles editable drop down buttons - including autocomplete dropmarkers - natively (v2)

>+/* ::::: history button ::::: */
>+
>+.autocomplete-history-dropmarker[enablehistory="true"] {
>+  -moz-appearance: menulist-button !important;
>+}

I think it would be better to remove [enablehistory="true"]. In the :not([enablehistory="true"]) case, display is "none", so the appearance shouldn't hurt.
Is the important flag needed?

Comment 17

[Teune van Steeg]

Attachment: styles editable drop down buttons - including autocomplete dropmarkers - natively (v2.1)

addresses comment 16

Comment 18

[u294409]

Any screenshots?

Comment 19

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

Comment on attachment 295868 [details] [diff] [review]
styles editable drop down buttons - including autocomplete dropmarkers - natively (v2.1)

+    *width += 15 + GTK_MISC(gArrowWidget)->xpad * 2;

Make 15 a named constant

Comment 20

[Reed Loden [:reed]]

Comment on attachment 295868 [details] [diff] [review]
styles editable drop down buttons - including autocomplete dropmarkers - natively (v2.1)

Yay native theming.

Comment 21

[Reed Loden [:reed]]

Not landing yet, as per comment #9 and comment #19.

Comment 22

[Michael Monreal [:micmon]]

The bug mentioned in comment #9 should be fixed, right?

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> The bug mentioned in comment #9 should be fixed, right?

It's not.

And please don't ask for approval for patches that can't land yet.

Comment 24

[Reed Loden [:reed]]

(In reply to comment #23)
> (In reply to comment #22)
> > The bug mentioned in comment #9 should be fixed, right?
> 
> It's not.

Is there a bug filed tracking this?

> And please don't ask for approval for patches that can't land yet.

I don't agree with this. As long as the dependency chain is correct, I don't see any problem with getting approval for a patch while it waits on another bug. There is also prior precedent showing the same thing I did here.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > The bug mentioned in comment #9 should be fixed, right?
> > 
> > It's not.
> 
> Is there a bug filed tracking this?

Bug 409388 according to comment 6.

> > And please don't ask for approval for patches that can't land yet.
> 
> I don't agree with this. As long as the dependency chain is correct, I don't
> see any problem with getting approval for a patch while it waits on another
> bug.

Well, was it obvious that the patch is waiting for another bug? The fact that it's in the dependency tree isn't enough.
Let's assume we wait for the other bug until just before the Beta 3 freeze -- chances are that you would only get approval for landing this after the freeze.

Comment 26

[Teune van Steeg]

Attachment: styles editable drop down buttons - including autocomplete dropmarkers - natively (for checkin)

This patch addresses comment #19. 
I assume I don't have to ask for r/sr/a again, since that is given for the previous patch.

Comment 27

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

Correct, you don't.

Comment 28

[Teune van Steeg]

Attachment: followup patch: fix some glitches

This patch fixes arrow position in the drop down button on RTL layouts (widget:gtk part) and visual gap between drop down button and elements in the location bar (like the favicon container on RTL, toolkit theme part)

Screenshot of the problems: https://bugzilla.mozilla.org/attachment.cgi?id=296637

Asking review from Gavin on the toolkit part: the border of native themed text fields is 3px width and the right border (or left border in case of RTL) is replaced by GTK themes with a gap to connect the text field with the drop down button. With this patch the gap is 'filled' with the content of the text field.

Comment 29

[Teune van Steeg]

Comment on attachment 296868 [details] [diff] [review]
followup patch: fix some glitches

asking r+sr from roc on the widget:gtk part

Comment 30

[Dão Gottwald [:dao]]

Could be a txul win, thanks to getting rid of the bloated, theme-specific XBL binding.

Comment 31

[Reed Loden [:reed]]

attachment 296147 [details] [diff] [review]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.66; previous revision: 1.65
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.50; previous revision: 1.49
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.137; previous revision: 1.136
done
Checking in toolkit/themes/gnomestripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/themes/gnomestripe/global/menulist.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menulist.css,v  <--  menulist.css
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.26; previous revision: 1.25
done


attachment 296868 [details] [diff] [review]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.67; previous revision: 1.66
done
Checking in toolkit/themes/gnomestripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.13; previous revision: 1.12
done

Comment 32

[Michael Monreal [:micmon]]

Amazing work!

Now, first, I think, the same "trick" should also be used for the "button" in front of the search bar. Without the focus ring, it would look much cleaner and the button look (gradient etc) would help make it recognizable as a button. I'll attach a mockup for this in a minute.

Second thing... I think it would be possible to also use this for the left end of the URL bar, the "identity button". If I understand correctly, this is meant to look like a button anyway. This is the chance to make it look like one! The only tricky part I think is re-coloring the whole thing when it needs to be green for SSL/EV. But GTK can do this, there's a panel launcher called "Gimmie"¹ which uses the same thing to draw buttons using the theme style, but using different colors. The same re-coloring (yellow this time) can be used for the search button to reflect a found search engine.

I did mockups for this, too. The EV button looks GREAT this way, for search the color is to dark but I think the idea works great, too.

What do you think?

[1] http://www.beatniksoftware.com/gimmie

Comment 33

[Michael Monreal [:micmon]]

Attachment: Further mockup

Comment 34

[pantsgolem]

Attachment: misaligned arrow

gtk engine is QtCurve

On the subject of the focus ring, that opens up a whole new can of worms. The three-sided focus box looks out of place in engines other than Clearlooks.  Some themes (e.g. Glider and Crux) paint the ring outside of the dropdown.  Others (e.g. Industrial and QtCurve) paint it inside, but Clearlooks is the only one I've found (of the engines already installed on my system) that takes the three-sided approach.  Of course, this sounds like a job for another bug, if it's even feasible to do properly at all.

Comment 35

[Teune van Steeg]

(In reply to comment #34)
> Created an attachment (id=296963) [details]
> misaligned arrow
> 
> gtk engine is QtCurve
> 
> On the subject of the focus ring, that opens up a whole new can of worms. The
> three-sided focus box looks out of place in engines other than Clearlooks. 
> Some themes (e.g. Glider and Crux) paint the ring outside of the dropdown. 
> Others (e.g. Industrial and QtCurve) paint it inside, but Clearlooks is the
> only one I've found (of the engines already installed on my system) that takes
> the three-sided approach.  Of course, this sounds like a job for another bug,
> if it's even feasible to do properly at all.
> 

Could you file two seperate bugs for these issues (arrow misalignment and focus ring) and CC me on those?

I think we can fix the focus ring for the themes who draw it all around, but I expect it to be difficult to fix it for themes that draw the inside focus ring.

Comment 36

[Teune van Steeg]

(In reply to comment #32)
> I did mockups for this, too. The EV button looks GREAT this way, for search the
> color is to dark but I think the idea works great, too.
>
> What do you think?

It looks great in your mockups, but when I look at how drop down buttons in other themes are painted, I don't think this would be an option. 

Comment 37

[Michael Monreal [:micmon]]

(In reply to comment #36)
> It looks great in your mockups, but when I look at how drop down buttons in
> other themes are painted, I don't think this would be an option. 

I am a bit surprised... the only theme I have on my machine where the button is not correctly drawn is "Crux", and that's probably because the engine is quite old and was never cleanly ported to cairo etc. All the other engines that ship with GNOME seem to produce correct visuals. What engine are you testing with?

It's worth comparing how buttons actually look in GTK apps using the same theme, some engines really draw strange and unattractive buttons...

Comment 38

[Ian Spence]

The "arrow-scaling" style attribute was added in GTK 2.12.  It'd probably be best if you used some version checking to give a default value to those using GTK 2.10

Also, for some reason, if I changed GTK themes, the dropdown button does not change themes. Not sure if it is related to the gtk warning I get for the arrow-scaling attribute not existing.

Comment 39

[Teune van Steeg]

(In reply to comment #37)
> (In reply to comment #36)
> > It looks great in your mockups, but when I look at how drop down buttons in
> > other themes are painted, I don't think this would be an option. 
> 
> I am a bit surprised... the only theme I have on my machine where the button is
> not correctly drawn is "Crux", and that's probably because the engine is quite
> old and was never cleanly ported to cairo etc. All the other engines that ship
> with GNOME seem to produce correct visuals. What engine are you testing with?
> 
> It's worth comparing how buttons actually look in GTK apps using the same
> theme, some engines really draw strange and unattractive buttons...
> 

Yeah you are right, I shouldn't let my decision depend on the last theme I saw (Crux).
I say go ahead and file seperate bugs for the identity box and the search bar (let's give it a try at least). 

Comment 40

[Teune van Steeg]

(In reply to comment #38)
> The "arrow-scaling" style attribute was added in GTK 2.12.  It'd probably be
> best if you used some version checking to give a default value to those using
> GTK 2.10

Are you sure? I can't find that in the Index of new symbols in 2.12 
http://library.gnome.org/devel/gtk/stable/ix08.html (arow-size, which I'm not using because of that, is however)

> Also, for some reason, if I changed GTK themes, the dropdown button does not
> change themes. Not sure if it is related to the gtk warning I get for the
> arrow-scaling attribute not existing.
> 

Disabling getting arrow-scaling does not fix that problem. I wonder what can be causing that.

Comment 41

[u294409]

The focus now is quite odd.

Comment 42

[Reed Loden [:reed]]

(In reply to comment #38)
> The "arrow-scaling" style attribute was added in GTK 2.12.  It'd probably be
> best if you used some version checking to give a default value to those using
> GTK 2.10

Mind filing a bug about fixing that? We shouldn't be requiring anything that wasn't in GTK 2.10. We can use GTK 2.12 stuff, but it must be properly #ifdef'd with version checks and have appropriate fallbacks for older GTK versions.

Comment 43

[Mark Perris]

Attachment: Shows the bar ends on Ubuntu Feisty, default theme

This is great work, but i've noticed that the two ends of the bar don't quite 'balance' in terms of border color. The left one containing the site icon has a grey border, the right one containing the arrow has a black border. It's quite subtle but noticeable when close together.
I don't believe this issue is addressed in the updated work in comment #33.
This is on Ubuntu Feisty with the default theme.

Comment 44

[Reed Loden [:reed]]

(In reply to comment #43)
> This is great work, but i've noticed that the two ends of the bar don't quite
> 'balance' in terms of border color. The left one containing the site icon has a
> grey border, the right one containing the arrow has a black border. It's quite
> subtle but noticeable when close together.
> I don't believe this issue is addressed in the updated work in comment #33.
> This is on Ubuntu Feisty with the default theme.

File a new bug on this, please.

Comment 45

[u294409]

By the way, Ubuntulooks should die. Old and not updated from over a year.

Comment 46

[Dão Gottwald [:dao]]

(In reply to comment #34)
> Created an attachment (id=296963) [details]
> misaligned arrow

Teune, do you think this is because of the negative margin? Is "replaced by GTK themes with a gap" (comment 28) actually not as generally true as it sounds?

Comment 47

[Teune van Steeg]

(In reply to comment #46)
> (In reply to comment #34)
> > Created an attachment (id=296963) [details] [details]
> > misaligned arrow
> 
> Teune, do you think this is because of the negative margin? Is "replaced by GTK
> themes with a gap" (comment 28) actually not as generally true as it sounds?
> 

No, it isn't because of the margin, but I haven't got a clue to what is causing it. All GTK themes I've seen so far draw the text field with an open end towards the drop down button. And I would be surprised if I found one that draws a border there.

Comment 48

[Teune van Steeg]

(In reply to comment #47)
> All GTK themes I've seen so far draw the text field with an open end
> towards the drop down button. And I would be surprised if I found one that
> draws a border there.
> 

I stand corrected: I just saw a 'classic' GTK theme which just draws a text field with all borders and next to it a normal button.

Comment 49

[Michael Monreal [:micmon]]

(In reply to comment #48)
> I stand corrected: I just saw a 'classic' GTK theme which just draws a text
> field with all borders and next to it a normal button.

Those are the same old themes that don't have focus rings, right?

Comment 50

[Teune van Steeg]

(In reply to comment #49)
> Those are the same old themes that don't have focus rings, right?
> 

correct

Comment 51

[u294409]

@Mark Perris: same issue with Clearlooks.

Comment 52

[u294409]

Attachment: clearlooks problem, with zoomed arrow

By the way, locationbar's entry doesn't have focus indicator now...
The dropdown button also gets no focus...

Comment 53

[Teune van Steeg]

(In reply to comment #52)
> Created an attachment (id=297157) [details]
> clearlooks problem, with zoomed arrow
> 
> By the way, locationbar's entry doesn't have focus indicator now...
> The dropdown button also gets no focus...
> 

What do you mean with 'now'? 

Does the border color of the drop down button and entry differ from native apps?
And the same question for focus handling on the drop down button.
AFAICT there is no difference.

Comment 54

[Michael Monreal [:micmon]]

Attachment: Native dropbutton focus with clearlooks

This shows the NEW effect effect of Clearlooks in a native app. In Firefox, the button does not seem to show any focus effect. Other buttons in FF do this correctly now.

Comment 55

[u294409]

now - few days/weeks after some patch has been sent.

I don't know how it differs...
Same with focus...

Comment 56

[u294409]

@Michael Monreal: I'm using Clearlooks trunk...

Comment 57

[Michael Monreal [:micmon]]

Jakub: me too

Comment 58

[Teune van Steeg]

(In reply to comment #54)
> Created an attachment (id=297158) [details]
> Native dropbutton focus with clearlooks
> 
> This shows the NEW effect effect of Clearlooks in a native app. In Firefox, the
> button does not seem to show any focus effect. Other buttons in FF do this
> correctly now.
> 

You should probably file a new bug then (under Core: Disability Access APIs) and make it dependent to this one

Comment 59

[u294409]

Comment on attachment 297157 [details]
clearlooks problem, with zoomed arrow

Same behavior in GTK

Comment 60

[Justin Dolske [:Dolske]]

This caused a ~45K trace_malloc_leak increase on fxdbug-linux-tbox.

...
25551 2008:01:14:03:30:30	3729875	MOZ_CO_DATE=2008:01:14:03:07:00	--	
25552 2008:01:14:03:51:12	3774890	MOZ_CO_DATE=2008:01:14:03:31:00	--	
...

Comment 61

[Reed Loden [:reed]]

(In reply to comment #60)
> This caused a ~45K trace_malloc_leak increase on fxdbug-linux-tbox.

Did bug 412432 help with this?

Comment 62

[Justin Dolske [:Dolske]]

Nope. Lk is still at the same it was after this checkin... See http://build-graphs.mozilla.org/graph/query.cgi?tbox=fxdbug-linux-tbox.build.mozilla.org&testname=trace_malloc_leaks&units=bytes&autoscale=1&days=7&avg=1&

Comment 63

[pantsgolem]

Ooh, sorry I didn't follow up on my earlier posting.  I had a day-long internet outage shortly afterward, and I only just now realized I forgot to cc myself on this bug.  Do you still need bugs filed on the arrow misalignment and the focus ring?

Incidentally, firefox seems to have quite a few problems that are specific to QtCurve.  Would it be better to lump them all in one big bug, then determine which are actually firefox bugs, and which are qtcurve bugs?

Comment 64

[u294409]

QtCurve isn't really a quality GTK engine, but Qt→GTK port of Qt style (engine).

Comment 65

[Teune van Steeg]

(In reply to comment #63)
> Ooh, sorry I didn't follow up on my earlier posting.  I had a day-long internet
> outage shortly afterward, and I only just now realized I forgot to cc myself on
> this bug.  Do you still need bugs filed on the arrow misalignment and the focus
> ring?
> 
Yes, please file them.

> Incidentally, firefox seems to have quite a few problems that are specific to
> QtCurve.  Would it be better to lump them all in one big bug, then determine
> which are actually firefox bugs, and which are qtcurve bugs?
> 

I think the latter option would be best: put them in one bug for now, and then the actual Firefox bugs can be fixed in separate bugs if needed.

Comment 66

[Ori Avtalion (salty-horse)]

I filed a bug 415161 about the arrow misalignment. hoodedone's attachment 296963 [details] looks different, though.