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

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?

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.

(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

(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.

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.

I like the mockup.

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...

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

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

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

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?

How to test patched-behaviour?

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 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?

addresses comment 16

Any screenshots?

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 on attachment 295868 [details] [diff] [review] styles editable drop down buttons - including autocomplete dropmarkers - natively (v2.1) Yay native theming.

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

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

(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.

(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.

(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.

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.

Correct, you don't.

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 on attachment 296868 [details] [diff] [review] followup patch: fix some glitches asking r+sr from roc on the widget:gtk part

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

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

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

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.

(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.

(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.

(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...

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.

(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).

(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.

The focus now is quite odd.

(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.

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.

(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.

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

(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?

(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.

(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.

(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?

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

@Mark Perris: same issue with Clearlooks.

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

(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.

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.

now - few days/weeks after some patch has been sent. I don't know how it differs... Same with focus...

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

Jakub: me too

(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 on attachment 297157 [details] clearlooks problem, with zoomed arrow Same behavior in GTK

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 -- ...

(In reply to comment #60) > This caused a ~45K trace_malloc_leak increase on fxdbug-linux-tbox. Did bug 412432 help with this?

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&

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?

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

(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.

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