Dão Gottwald [:dao] Assignee
	Description • 17 years ago

      No description provided.

	Ryan Flint [:rflint] (ping via IRC for reviews)
	Comment 2 • 17 years ago

mozilla/browser/base/content/browser-places.js 	       1.55
mozilla/browser/base/content/browser.js 	       1.855
mozilla/browser/base/content/browser.xul               1.379
mozilla/browser/components/search/content/search.xml   1.102
mozilla/browser/themes/winstripe/browser/browser.css   1.103
mozilla/browser/themes/winstripe/browser/searchbar.css 1.20

	Ryan Flint [:rflint] (ping via IRC for reviews)
	Comment 3 • 17 years ago

Backed out. This makes the endcaps too tall and causes test failures.

	Dão Gottwald [:dao] Assignee
	Comment 4 • 17 years ago

Ryan, would it be possible for you to attach a screenshot? I didn't see anything wrong.

	Dão Gottwald [:dao] Assignee
	Comment 5 • 17 years ago

I'm downloading an old hourly now and will test with a clean profile.

Could it be that the failure was on Mac?

	Dão Gottwald [:dao] Assignee
	Comment 6 • 17 years ago

(In reply to comment #5)
> I'm downloading an old hourly now and will test with a clean profile.

Looks correct with the 20070929_1940 build.

	Dão Gottwald [:dao] Assignee
	Comment 7 • 17 years ago

I'm not seeing a difference in text mode either.

	Phil Ringnalda (:philor)
	Comment 8 • 17 years ago

No, not Mac - that was where you passed. Windows and Linux, you were failing http://lxr.mozilla.org/seamonkey/source/docshell/test/test_bug344861.html which opens a popup and checks the innerHeight and innerWidth, and since it's a no-toolbars popup that gets the readonly locationbar, I'm not sure whether you made that 2px too big, or whether the toolbar doesn't count against innerHeight, and you made it 2px too small so the content wound up with 2px it didn't expect to get.

	Dão Gottwald [:dao] Assignee
	Comment 9 • 17 years ago

This should fix the appearance and the reftest. I've removed the empty space from the icons so that the dimensions can be controlled in the stylesheet. Also, I'm using the "chromeclass-toolbar-additional" class which should be semantically correct and allow the window height to be calculated independently from the endcap, as the appropriate xul.css rules apply earlier than the delayedStartup (browser.js) code that did the hiding.

	Dão Gottwald [:dao] Assignee
	Comment 10 • 17 years ago

Comment on attachment 282931 [details] [diff] [review]
patch v2

umm, wrong patch.

	Dão Gottwald [:dao] Assignee
	Comment 16 • 17 years ago

needs testing

	Dão Gottwald [:dao] Assignee
	Comment 20 • 17 years ago

Tested on XP and Linux (including the reftest that broke with the first patch); hopefully it doesn't explode on Mac. It will, however, most likely look like a visual regression on Mac. There's a new theme coming though, so I don't think we should care.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 22 • 17 years ago

Comment on attachment 285218 [details]
screenshot with upcoming patch

I'm up with killing the endcaps, but please make sure that it's still possible to make the search-engine selection stylable as a button with a drop-down; we're going to want that as a button for Windows XP,  a hover state for Windows Vista and a button for OSX. Not sure what the right thing is for Linux, probably no-button.

	Dão Gottwald [:dao] Assignee
	Comment 23 • 17 years ago

(In reply to comment #22)
> I'm up with killing the endcaps, but please make sure that it's still possible
> to make the search-engine selection stylable as a button with a drop-down;
> we're going to want that as a button for Windows XP,  a hover state for Windows
> Vista and a button for OSX. Not sure what the right thing is for Linux,
> probably no-button.

Should be possible. If plain CSS (borders + a few background images) doesn't suffice, a theme can add a binding (hopefully a better one than the current stack).

	Dão Gottwald [:dao] Assignee
	Comment 24 • 17 years ago

this aligns the autocomplete popup with the input field

	Dão Gottwald [:dao] Assignee
	Comment 25 • 17 years ago

this should bring back the rounded border on mac

	Dão Gottwald [:dao] Assignee
	Comment 26 • 17 years ago

updated to trunk, removed the "dropongobutton" locale property

	ronin.achilles
	Comment 27 • 17 years ago

Sorry I might sound dumb, but does 'updated to trunk' mean checkin? or something else?

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 17 years ago

It means that a patch is static while the trunk is constantly changing. As a result, the patch needs to be updated for the changes happening around it on a constant basis. This is also known as bitrot.

	Dão Gottwald [:dao] Assignee
	Comment 29 • 17 years ago

I believe this should target M9, because it's relevant for the upcoming Gnomestripe fork and Pinstripe makeover. If we miss either of them, I could of course update the patch, but especially for Pinstripe that could be significantly harder for me to do, as I still don't have access to a Mac.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 30 • 17 years ago

Dao: for the site button, here are the ratios Beltzner and I would like to use on each platform, to give the theme a subtle but through sense of visual integration.

Height of intersecting circle to height of square:
OS X, 1 : 1
Vista: 1.2666 : 1
XP: 1.8666 : 1

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 31 • 17 years ago

I should note that these ratios are also going to set the curve on the search button, and the right side of the forward button.

	Reed Loden [:reed]
	Comment 32 • 17 years ago

What about Linux?

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 33 • 17 years ago

>What about Linux?

We are going to present our plans for the other platforms to the team of contributers creating our linux theme and see what they think is the best way to go.  Maybe just make the end-caps square, or add slight curves on just the corners?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 34 • 17 years ago

Blocking, though not blocking beta, though if you get a review in time, feel free to nom for approvalM9

	Michael Ventnor
	Comment 35 • 17 years ago

Dao, you'll need to update this patch to include the now-forked Gnomestripe.

	Dão Gottwald [:dao] Assignee
	Comment 36 • 17 years ago

updated to trunk

mac untested.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 38 • 17 years ago

That screenshot looks great!

	Dão Gottwald [:dao] Assignee
	Comment 39 • 17 years ago

updated to trunk

	Mike Connor [:mconnor]
	Comment 41 • 17 years ago

Comment on attachment 288852 [details] [diff] [review]
patch v4.1


>+  if (gURLBar.getAttribute("pageproxystate") != aState) {
>+    gURLBar.setAttribute("pageproxystate", aState);
>+#ifdef MOZ_WIDGET_GTK2
>+    // redraw gtk focus ring
>+    if (gURLBar.focused)
>+      gURLBar.focus();
>+#endif

Is there a bug on the core issue here?

this looks ok on Mac, we're doing a bunch of new stuff with Proto so I'm not going to worry about it right at this moment, I'll let Kevin and Stephen know to look at how to style this within that context properly.

Sorry for the delay here.

	Reed Loden [:reed]
	Comment 42 • 17 years ago

I think I got all the added/removed files. If you see something I missed, please let me know.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.900; previous revision: 1.899
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.394; previous revision: 1.393
done
Checking in browser/components/search/content/search.xml;
/cvsroot/mozilla/browser/components/search/content/search.xml,v  <--  search.xml
new revision: 1.107; previous revision: 1.106
done
Checking in browser/components/search/content/searchbarBindings.css;
/cvsroot/mozilla/browser/components/search/content/searchbarBindings.css,v  <--  searchbarBindings.css
new revision: 1.6; previous revision: 1.5
done
Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.78; previous revision: 1.77
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.52; previous revision: 1.51
done
Checking in browser/themes/gnomestripe/browser/Go-arrow-rtl.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Go-arrow-rtl.png,v  <--  Go-arrow-rtl.png
new revision: 1.5; previous revision: 1.4
done
Checking in browser/themes/gnomestripe/browser/Go-arrow.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Go-arrow.png,v  <--  Go-arrow.png
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-addengines.png,v
done
Checking in browser/themes/gnomestripe/browser/Search-addengines.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-addengines.png,v  <--  Search-addengines.png
initial revision: 1.1
done
Checking in browser/themes/gnomestripe/browser/Search-glass-rtl.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-glass-rtl.png,v  <--  Search-glass-rtl.png
new revision: 1.3; previous revision: 1.2
done
Checking in browser/themes/gnomestripe/browser/Search-glass.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-glass.png,v  <--  Search-glass.png
new revision: 1.3; previous revision: 1.2
done
Removing browser/themes/gnomestripe/browser/Search-provider-bkgnd.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-provider-bkgnd.png,v  <--  Search-provider-bkgnd.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/gnomestripe/browser/Search-provider-mid-bottom.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-provider-mid-bottom.png,v  <--  Search-provider-mid-bottom.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/gnomestripe/browser/Search-provider-mid-top.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/Search-provider-mid-top.png,v  <--  Search-provider-mid-top.png
new revision: delete; previous revision: 1.2
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.137; previous revision: 1.136
done
Removing browser/themes/gnomestripe/browser/endcap-bkgnd-hover.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/endcap-bkgnd-hover.png,v  <--  endcap-bkgnd-hover.png
new revision: delete; previous revision: 1.5
done
Removing browser/themes/gnomestripe/browser/endcap-bkgnd.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/endcap-bkgnd.png,v  <--  endcap-bkgnd.png
new revision: delete; previous revision: 1.1
done
Checking in browser/themes/gnomestripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.63; previous revision: 1.62
done
Checking in browser/themes/gnomestripe/browser/searchbar.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/searchbar.css,v  <--  searchbar.css
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/Go-arrow.png,v
done
Checking in browser/themes/pinstripe/browser/Go-arrow.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Go-arrow.png,v  <--  Go-arrow.png
initial revision: 1.1
done
Removing browser/themes/pinstripe/browser/Go-rtl.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Go-rtl.png,v  <--  Go-rtl.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/pinstripe/browser/Go.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Go.png,v  <--  Go.png
new revision: delete; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/Search-addengines.png,v
done
Checking in browser/themes/pinstripe/browser/Search-addengines.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Search-addengines.png,v  <--  Search-addengines.png
initial revision: 1.1
done
Removing browser/themes/pinstripe/browser/Search-go-rtl.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Search-go-rtl.png,v  <--  Search-go-rtl.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/pinstripe/browser/Search-go.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Search-go.png,v  <--  Search-go.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/pinstripe/browser/Search-provider-bkgnd-rtl.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Search-provider-bkgnd-rtl.png,v  <--  Search-provider-bkgnd-rtl.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/pinstripe/browser/Search-provider-bkgnd.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/Search-provider-bkgnd.png,v  <--  Search-provider-bkgnd.png
new revision: delete; previous revision: 1.2
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.96; previous revision: 1.95
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.65; previous revision: 1.64
done
Checking in browser/themes/pinstripe/browser/searchbar.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/searchbar.css,v  <--  searchbar.css
new revision: 1.18; previous revision: 1.17
done
Checking in browser/themes/winstripe/browser/Go-arrow-rtl.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Go-arrow-rtl.png,v  <--  Go-arrow-rtl.png
new revision: 1.3; previous revision: 1.2
done
Checking in browser/themes/winstripe/browser/Go-arrow.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Go-arrow.png,v  <--  Go-arrow.png
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/Search-addengines.png,v
done
Checking in browser/themes/winstripe/browser/Search-addengines.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-addengines.png,v  <--  Search-addengines.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/Search-glass-rtl.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-glass-rtl.png,v  <--  Search-glass-rtl.png
new revision: 1.3; previous revision: 1.2
done
Checking in browser/themes/winstripe/browser/Search-glass.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-glass.png,v  <--  Search-glass.png
new revision: 1.3; previous revision: 1.2
done
Removing browser/themes/winstripe/browser/Search-provider-bkgnd.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-provider-bkgnd.png,v  <--  Search-provider-bkgnd.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/winstripe/browser/Search-provider-mid-bottom.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-provider-mid-bottom.png,v  <--  Search-provider-mid-bottom.png
new revision: delete; previous revision: 1.2
done
Removing browser/themes/winstripe/browser/Search-provider-mid-top.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/Search-provider-mid-top.png,v  <--  Search-provider-mid-top.png
new revision: delete; previous revision: 1.2
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.136; previous revision: 1.135
done
Removing browser/themes/winstripe/browser/endcap-bkgnd-hover.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/endcap-bkgnd-hover.png,v  <--  endcap-bkgnd-hover.png
new revision: delete; previous revision: 1.3
done
Removing browser/themes/winstripe/browser/endcap-bkgnd.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/endcap-bkgnd.png,v  <--  endcap-bkgnd.png
new revision: delete; previous revision: 1.3
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.62; previous revision: 1.61
done
Checking in browser/themes/winstripe/browser/searchbar.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/searchbar.css,v  <--  searchbar.css
new revision: 1.23; previous revision: 1.22
done

	Reed Loden [:reed]
	Comment 43 • 17 years ago

(In reply to comment #41)
> (From update of attachment 288852 [details] [diff] [review])
> 
> >+  if (gURLBar.getAttribute("pageproxystate") != aState) {
> >+    gURLBar.setAttribute("pageproxystate", aState);
> >+#ifdef MOZ_WIDGET_GTK2
> >+    // redraw gtk focus ring
> >+    if (gURLBar.focused)
> >+      gURLBar.focus();
> >+#endif
> 
> Is there a bug on the core issue here?

Dão, please file a core :: widget: gtk bug on this if there isn't one.

	Dão Gottwald [:dao] Assignee
	Comment 44 • 17 years ago

filed bug 405189

	Bill Gianopoulos [:WG9s]
	Comment 45 • 17 years ago

This cased the regression in bug 405212 under Linux.

	Ria Klaassen (not reading all bugmail)
	Comment 46 • 17 years ago

I see no Go button in a new profile on Windows. I see it for a short moment, then it is replaced by the star.

	Gabriel Chadwick
	Comment 47 • 17 years ago

This patch works for me with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre ID:2007112405. But it introduce a usability problem. When the go button appears there is no way to make it disapear. Even when focus is given to an other item the go button persists. Filed bug #405244.

	Martijn Wargers (dead)
	Comment 48 • 17 years ago

Fwiw, I used the "Go" button as a nice way of going back to the anchor position of urls with a hash part.
Also, I liked the "reload" behavior of the "Go" button regarding pages with frames.

	Ria Klaassen (not reading all bugmail)
	Comment 49 • 17 years ago

The show-and-hide behaviour is really a good feature but maybe there also should be a pref to always show the go-button?

	John A. Bilicki III
	Comment 50 • 17 years ago

This behavior is not acceptable for regular surfers who associate the go button with the address bar. Additionally a green triangle will only clue some people in that it is a go button. I also should no longer refer to it as a button since it's just an icon with this change. Microsoft screwed up IE's GUI, Mozilla has to start  doing so too? If the wheel is round it works, any enhancements should be optional and off by default. The people who know how to turn it on will and the people who need a go button won't be without one by default. Consistency is a critical point in design.

	Martijn Wargers (dead)
	Comment 51 • 17 years ago

John, fwiw, I think I agree with you a lot of what you say.
It's probably best to start a discussion about that in the dev apps Firefox mailing list, as bugzilla is not meant for this kind of discussions:
https://lists.mozilla.org/listinfo/dev-apps-firefox

	Tony Chung [:tchung]
	Comment 52 • 17 years ago

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120704 Minefield/3.0b2pre.   Also verified on windows and linux nightly builds.