Closed
Bug 351195
Opened 18 years ago
Closed 18 years ago
Address bar should be non-native on Linux
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
RESOLVED
FIXED
Firefox 2
People
(Reporter: pkasting, Assigned: myk)
References
Details
(Keywords: fixed1.8.1, Whiteboard: [Fx2 theme change])
Attachments
(6 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mconnor
:
review+
mconnor
:
approval1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
On Linux, the search bar is already (apparently) non-native and looks fine with the endcap buttons. However, the address bar appears to be natively themed, which looks very bizarre, since it's typically taller than the go button endcap (and the endcap doesn't currently scale), and often has rounded ends that look very odd with the endcap or in comparison to the search bar.
See the right column of https://bugzilla.mozilla.org/attachment.cgi?id=236546 , which shows both bars with a variety of different Linux themes.
I think we should bite the bullet and make the address bar non-native on Linux like the search bar appears to already be. Note that bug 347616 would make the dropmarker non-native.
Reporter | ||
Updated•18 years ago
|
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2
Reporter | ||
Comment 1•18 years ago
|
||
The second post on this page:
http://forums.mozillazine.org/viewtopic.php?t=456747&start=45
Proposes some CSS changes (with before/after pix) that might clean up the look of things a bit on Windows Classic. Not sure whether I want to morph this bug into that, but certainly the "address bar is too tall for Go button" gets fixed by those changes.
Reporter | ||
Comment 2•18 years ago
|
||
(In reply to comment #1)
> The second post on this page:
>
> http://forums.mozillazine.org/viewtopic.php?t=456747&start=45
>
> Proposes some CSS changes (with before/after pix) that might clean up the look
> of things a bit on Windows Classic. Not sure whether I want to morph this bug
> into that, but certainly the "address bar is too tall for Go button" gets fixed
> by those changes.
See also bug 351157 comment 16. This border style looks a lot better...
Comment 3•18 years ago
|
||
taking to investigate for now
Assignee: nobody → mconnor
Flags: blocking-firefox2? → blocking-firefox2+
Comment 4•18 years ago
|
||
That border style really didn't play so well with classic, especially darker color schemes, I can post some screenshots. I'd love to see this on Linux with Pam's patch.
Reporter | ||
Comment 5•18 years ago
|
||
(In reply to comment #4)
> That border style really didn't play so well with classic, especially darker
> color schemes, I can post some screenshots.
I believe the screenshots here: https://bugzilla.mozilla.org/attachment.cgi?id=236954 ...are styled using ThreeDShadow instead of ThreeDDarkShadow, but I'm not sure. That seems better than what we have now, at least.
Comment 6•18 years ago
|
||
(In reply to comment #5)
> I believe the screenshots here:
> https://bugzilla.mozilla.org/attachment.cgi?id=236954 ...are styled using
> ThreeDShadow instead of ThreeDDarkShadow, but I'm not sure.
Yes, they are. Attachment 236722 [details] also uses ThreeDShadow.
Assignee | ||
Comment 7•18 years ago
|
||
Here's a patch that removes native styling on Linux for autocomplete widgets. It accomplishes this by simply removing autocomplete.css from gnomestripe, which then falls back on the autocomplete.css in winstripe.
autocomplete.css is almost exactly the same in gnomestripe as it is in winstripe once you remove native styling. The only remaining difference between the two files is the fix for bug 270993, which this patch regresses. But it's unclear why we ever fixed that bug in the first place, and if we do want that fix, then I suspect we're better off doing %ifdef MOZ_WIDGET_GTK2 in winstripe's version of the file than maintaining two copies of the file for it.
Besides fixing this bug, this patch also basically fixes the portion of bug 351618 which is a followup to bug 347616, since bug 347616's changes to winstripe's autocomplete.css get applied to Linux with this patch applied.
I should note, however, that although bug 347616's new autocomplete widget looks good on Linux at the default font size with this patch applied, it contains an artifact at larger sizes, so there may still be a bug in bug 347616's implementation on Linux.
Reporter | ||
Comment 8•18 years ago
|
||
https://bugzilla.mozilla.org/attachment.cgi?id=237236&action=view shows a screenshot of this patch on Linux, right? So how come the right edge of the bar is still rounded (and thus doesn't match up with the Go endcap)? Is there more needed here in order to make the box corner(s) square?
Reporter | ||
Comment 9•18 years ago
|
||
(In reply to comment #8)
> https://bugzilla.mozilla.org/attachment.cgi?id=237236&action=view shows a
> screenshot of this patch on Linux, right? So how come the right edge of the
> bar is still rounded (and thus doesn't match up with the Go endcap)? Is there
> more needed here in order to make the box corner(s) square?
I tested out the combination of this patch and the patch on bug 348138, and one of them (I think this one) has caused some goofiness.
My search box used to look good -- the endcaps linked up to a square-ended input box. Now my search box is rounded just like my URL bar, so the endcaps no longer join properly. When I tried other themes, I found my search and URL bars got border effects from the themes, like Classic-esque "sunken" bevels and other things; also, the box size didn't always match the size the endcaps scaled to-- it was frequently off by a pixel on the top and bottom.
So I don't think this patch does what I wanted.
HOWEVER, dropping the second post on this page: http://forums.mozillazine.org/viewtopic.php?t=456747&start=45
into my userChrome.css atop these patches DID work. I imagine it probably would also work without having applied this patch.
Comment 10•18 years ago
|
||
That patch affects all autocomplete text boxes, right (including the search bar)? I'm not sure making such a change for all users of the toolkit is a good idea, especially when we're doing it just to make the Firefox location bar look better. How does this affect the autocomplete text box in the File->Open Location dialog, for example (you need to hide the Navigation toolbar bar to see it)?
I was thinking that http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.44#974 was the code that needed to be tweaked for this, since it only applies to the location bar, and comment 0 says that the search bar is already "OK", but I guess you'd then need to be careful to not regress bug 263750.
Assignee | ||
Comment 11•18 years ago
|
||
(In reply to comment #10)
> That patch affects all autocomplete text boxes, right (including the search
> bar)?
Yes, I believe it does.
> I'm not sure making such a change for all users of the toolkit is a good
> idea, especially when we're doing it just to make the Firefox location bar
> look better.
An autocomplete widget looks roughly like this (with everything inside the textbox being anonymous content):
<textbox type="autocomplete" ... />
<hbox class=".autocomplete-textbox-container" ... />
<dropmarker ... />
<popupset ... />
</textbox>
There are four differences in autocomplete.css between winstripe and gnomestripe:
1. winstripe leaves the default style for the textbox and hbox:
outer textbox: -moz-appearance: textfield
inner hbox: -moz-appearance: none
while gnomestripe overrides it with custom style:
outer textbox: -moz-appearance: none
inner hbox: -moz-appearance: menulist-textfield
2. winstripe makes .autocomplete-textbox-container be -moz-box-align: center, while gnomestripe does not;
3. winstripe contains Pam's dropmarker fixes from bug 347616, while gnomestripe does not;
4. winstripe makes the .autocomplete-tree tree anonymous element (not shown above) be background-color: transparent, while gnomestripe makes it be background-color: -moz-field.
Taking these in reverse order:
#4 is a questionable fix for bug 270993. It's unclear why this change was made, as the bug never justifies it. I think we're ok reverting it, although we could also leave it in via an %ifdef MOZ_WIDGET_GTK section to winstripe's autocomplete.css.
#3 is the dropmarker fix from bug 347616. We want this for gnomestripe. Pam filed followup bug 351618 on it.
#2 was checked in to winstripe with the comment "Landing the Aviary Branch (Toolkit sections excluding toolkit/content)." Presumably it was part of a much larger checkin; perhaps it should have landed in gnomestripe, too. In any case, it seems harmless to apply to gnomestripe, and it doesn't cause any problems in my tests.
#1 is a small but significant difference between the two themes. It's unclear why gnomestripe overrides styles this way (the checkin comment from 2004-06-18 says merely "More Winstripe/Pinstripe Restructuring"), but it makes applying Pam's dropmarker fixes harder, since those fixes assume that the dropmarker is inside the box with borders, but these changes make the dropmarker be next to the box with borders.
We basically have two options here to get both Pam's dropmarker changes and the styling this bug is about:
1. Make gnomestripe use winstripe's autocomplete.css and then apply the style fixes from the mozillazine forum to the location and search bars.
2. Apply Pam's changes to gnomestripe's autocomplete.css (but perhaps with some changes), apply the style fixes from the mozillazine forum to the location and search bars for Windows only, and then apply the same style fixes to different elements for Linux only.
To me it sounds like #1 is the safer, simpler option.
> How does this affect the autocomplete text box in the File->Open
> Location dialog, for example (you need to hide the Navigation toolbar bar to
> see it)?
It makes it look pretty.
Assignee | ||
Comment 12•18 years ago
|
||
> So I don't think this patch does what I wanted.
Right. It was quite incomplete. Here's a patch that does what I think you want. Note that I haven't tested it on Windows yet (I don't have access to a Windows machine today), and I also haven't tested it with RTL; so it may still need a few tweaks. What it does is implement option #1 from comment 11, i.e.:
1. as with the previous patch, removes gnomestripe's autocomplete.css so that gnomestripe uses winstripe's version of that file;
2. applies the changes suggested in the mozillazine forum to the location bar and the search bar, except that it uses the color ThreeDShadow instead of ThreeDDarkShadow as the border color.
For some reason I had to move dist/ out of the way when rebuilding to get my build to apply these changes.
Attachment #237218 -
Attachment is obsolete: true
Attachment #237361 -
Flags: review?(mconnor)
Attachment #237218 -
Flags: review?(mconnor)
Reporter | ||
Comment 13•18 years ago
|
||
Bug 263750 comment 7 makes me worry about the Mozillazine forum fixes. Does this fix still work properly on secure sites?
Also, when I tested the ThreeDShadow stuff locally, it looked odd, because the endcap images have a particular color/opacity, and "ThreeDShadow" didn't match it; so where the endcap images met the text boxes, things looked wrong. Instead of using ThreeDShadow we should presumably be using the same color/opacity as the endcap images.
Finally, I'm concerned about changing the way autocomplete widgets look in other places when our goal was to fix theme in these two specific spots.
I'm still not sure why the short-term fix can't be for Gnomestripe's location bar to get styled like its search box is. I don't think that's quite as all-inclusive as your "fix #2" above, but maybe I'm wrong.
Assignee | ||
Comment 14•18 years ago
|
||
Here's a screenshot showing what the location bar, search bar, and "Open Web Location" dialog look like before and after the patch.
Assignee | ||
Updated•18 years ago
|
Attachment #237364 -
Attachment is patch: false
Attachment #237364 -
Attachment mime type: text/plain → image/png
Assignee | ||
Comment 15•18 years ago
|
||
Ben, Brian,
Can you comment on why gnomestripe styles autocomplete widgets differently from winstripe (specifically difference #1 from comment 11 in this bug)? We're trying to figure out if we can change gnomestripe to style such widgets the same way winstripe (and pinstripe, I think) style them.
Assignee | ||
Comment 16•18 years ago
|
||
Ben, Brian,
Can you comment on why gnomestripe styles autocomplete widgets differently from winstripe (specifically difference #1 from comment 11 in this bug)? We're trying to figure out if we can change gnomestripe to style such widgets the same way winstripe (and pinstripe, I think) style them.
Comment 17•18 years ago
|
||
(In reply to comment #13)
> Also, when I tested the ThreeDShadow stuff locally, it looked odd, because the
> endcap images have a particular color/opacity, and "ThreeDShadow" didn't match
> it; so where the endcap images met the text boxes, things looked wrong.
> Instead of using ThreeDShadow we should presumably be using the same
> color/opacity as the endcap images.
Would be no problem on the trunk, but Gecko 1.8 doesn't support rgba(). But I wouldn't do that anyway.
Assignee | ||
Comment 18•18 years ago
|
||
Ok, here's the minimal fix as called for by comment 13. I also replaced ThreeDShadow with the color we're currently using to style the borders of the non-native search bar: #96969D.
With this patch, the location bar's textbox looks non-native, but its dropmarker still looks native, i.e. ugly.
Assignee | ||
Comment 19•18 years ago
|
||
Reporter | ||
Comment 20•18 years ago
|
||
(In reply to comment #19)
> Created an attachment (id=237401) [edit]
> screenshot of v3 in action on Linux
This means the dropdown will look "next to" the location bar rather than "inside", right? bug 263750 comment 7 suggests that maybe the way out is to give the location bar a -moz-appearance: textfield. Although maybe at this point I'm really reversing what I said in comment 13...
Really, it seems like a hand-wavingly ideal solution would:
* Make the Linux and Windows location bars be structured the same
* Make sure that structure is fully styled by us with the border size/color you specified in your patch, etc. so there are no strange bevels that appear
* Not screw with random other non-location-bar bars like the "Open Web Location" thing
But maybe I'm wrong about that last point, and your patch v2 (except using the hardcoded colors from v3) is in fact the way to go, since presumably it handles points 1 and 2...
Assignee | ||
Comment 21•18 years ago
|
||
> This means the dropdown will look "next to" the location bar rather than
> "inside", right?
Right.
> bug 263750 comment 7 suggests that maybe the way out is to
> give the location bar a -moz-appearance: textfield. Although maybe at this
> point I'm really reversing what I said in comment 13...
It's partly a reversal of comment 13, except that we could do this just for the location bar rather than for autocomplete widgets generally. But while this would get the dropmarker inside the location bar rather than next to it, it wouldn't make the location bar non-native. It would just change which widget is being natively styled.
> Really, it seems like a hand-wavingly ideal solution would:
> * Make the Linux and Windows location bars be structured the same
> * Make sure that structure is fully styled by us with the border size/color
> you specified in your patch, etc. so there are no strange bevels that appear
> * Not screw with random other non-location-bar bars like the "Open Web
> Location" thing
>
> But maybe I'm wrong about that last point, and your patch v2 (except using
> the hardcoded colors from v3) is in fact the way to go, since presumably it
> handles points 1 and 2...
Yes, patch v2 w/hardcoded colors from v3 does indeed handle points 1 and 2.
Given that Pam has already landed her dropmarker fixes into toolkit/themes/winstripe/global/autocomplete.css, I suspect we're better off making gnomestripe reuse winstripe style for autocomplete widgets, since it means simpler code with fewer regressions which are easier to track down and fix.
But if necessary, I can make a patch which applies the fixes only to the location bar.
Comment 22•18 years ago
|
||
(In reply to comment #20)
> But maybe I'm wrong about that last point, and your patch v2 (except using the
> hardcoded colors from v3) is in fact the way to go
Could you post a screenshot, showing how ThreeDShadow looks odd with your setting?
At least in attachment 237364 [details], the color difference would never bother me, since the endcaps are relatively small. Contrary, I can easily imagine a bluish border for the whole address and search bars looking out of place in terms of OS integration.
Assignee | ||
Comment 23•18 years ago
|
||
Here's the complete fix from v2 but using the same colors that the search bar is currently using (presumably it's the same color as the border on the Go and Search buttons).
This patch also fixes a regression in v2 that drew borders both to the left and to the right of the search textbox (the current theme only draws a border to the right of the textbox).
And I throw in a fix for bug 350788, so that the border switches to the left of the textbox in RTL mode.
Attachment #237361 -
Attachment is obsolete: true
Attachment #237432 -
Flags: review?(mconnor)
Attachment #237361 -
Flags: review?(mconnor)
Assignee | ||
Comment 24•18 years ago
|
||
The current patch up for review in this bug will also fix bug 350788 and will partly fix bug 351618, so making those bugs depend on this one.
Comment 25•18 years ago
|
||
Comment 26•18 years ago
|
||
Also note that the Go button doesn't actually use a color like #96969D but a 80% opaque grey.
Reporter | ||
Comment 27•18 years ago
|
||
(In reply to comment #26)
> Also note that the Go button doesn't actually use a color like #96969D but a
> 80% opaque grey.
That's more what I'm worried about; we should make the box borders do whatever the Go border is doing, which is definitely not ThreeDShadow, but sounds like isn't 96969D either.
Comment 28•18 years ago
|
||
(In reply to comment #27)
> That's more what I'm worried about; we should make the box borders do whatever
> the Go border is doing, which is definitely not ThreeDShadow, but sounds like
> isn't 96969D either.
The Go button should rather try to match the theme, and I think darkening the toolbar background is the best we can do. On the trunk, using a ThreeDShadow CSS border and border-radius for the Go button might be an option, but without anti-aliasing that's ugly.
Now since the Go button's does darken the toolbar background, there is no single color you can apply to the urlbar. You would need rgba() for that.
Comment 29•18 years ago
|
||
(In reply to comment #28)
> Now since the Go button's does darken the toolbar background, there is no
> single color you can apply to the urlbar. You would need rgba() for that.
Just for the record, I think using rgba() would be far from perfect, too. It would match the Go button, but also inherit its problems with dark OS themes where the border disappears.
Comment 30•18 years ago
|
||
(In reply to comment #28)
> On the trunk, using a ThreeDShadow CSS border and border-radius for the Go
> button might be an option, but without anti-aliasing that's ugly.
Hmm ... maybe worth a try even without anti-aliasing, if the radius is not too big. 1 or 3px could work, depends on the image.
Comment 31•18 years ago
|
||
Would be great if it always looked like this. Unfortunately, resizing the window makes the border go crazy.
Comment 32•18 years ago
|
||
Comment on attachment 237432 [details] [diff] [review]
patch v4: complete fix with the right color
Looks good from here. There's a potential for color mismatch given the slight opacity on the image borders, but we may get new images to fix that.
Attachment #237432 -
Flags: review?(mconnor)
Attachment #237432 -
Flags: review+
Attachment #237432 -
Flags: approval1.8.1+
Updated•18 years ago
|
Whiteboard: [Fx2 theme change][checkin needed (1.8 branch)]
Comment 33•18 years ago
|
||
End cap borders are much, *much* lighter now. Again, I advocate ThreeDShadow.
Assignee | ||
Comment 34•18 years ago
|
||
Fix checked in to the branch. Note that mconnor's rollup checkin earlier today included the browser.css and searchbar.css fixes in the patch for this bug, so my checkin was just for the autocomplete.css and jar.mn fixes. Also note that you have to do a clobber build for these fixes to take effect.
Assignee | ||
Updated•18 years ago
|
Whiteboard: [Fx2 theme change][checkin needed (1.8 branch)] → [Fx2 theme change]
You need to log in
before you can comment on or make changes to this bug.
Description
•