352682 - Strange background for Go button in text-only mode

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marek Stępień [:marcoos, inactive]]

The "Go" button in text-only mode on Linux has some strange backround,
that makes it harder to read the "Go" word.

This is probably an artifact of the shiny new non-native Go end-cap background. ;-)

Part of this artifact disappears on hover, but a part of it still persists.

Comment 1

[Marek Stępień [:marcoos, inactive]]

Attachment: Screenshot

Comment 2

[Simon Bünzli]

IIRC the Go button should never show up as text button in the first place. This is a regression from bug 351618 I assume.

Comment 3

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #2)
> IIRC the Go button should never show up as text button in the first place.

Why shouldn't it? There was another discussion in another bug that it actually should. 

Comment 4

[Pam Greene]

It should.  But it shouldn't have that stray background image.  Patch coming.

Comment 5

[Pam Greene]

Attachment: Default to empty background image

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 238486 [details] [diff] [review]
Default to empty background image

hover feedback might be nice, but not critical at all

Comment 7

[Peter Kasting]

(In reply to comment #6)
> (From update of attachment 238486 [details] [diff] [review] [edit])
> hover feedback might be nice, but not critical at all
> 

By "hover feedback" I assume you mean "bevel"?

I believe the patch for bug 347754 attempts (or is going to attempt) that.

Comment 8

[Simon Bünzli]

(In reply to comment #7)
> I believe the patch for bug 347754 attempts (or is going to attempt) that.

That patch would at least make it much simpler, since we could style the Go button per default as any other toolbar button and only apply special styling (and a specialized binding) when it's next to the address bar and we're not in text-only mode. Unfortunately that isn't going to happen for Firefox 2.

BTW: Is there any bug to track which parts of the visual refresh should better not be landed on Trunk as is (such as the theme specific image stack which caused this bug and doesn't belong into browser.xul at all)?

Comment 9

[Peter Kasting]

(In reply to comment #8)
> BTW: Is there any bug to track which parts of the visual refresh should better
> not be landed on Trunk as is (such as the theme specific image stack which
> caused this bug and doesn't belong into browser.xul at all)?

In order to not break people's heads, the merge should make the trunk update to include all changes from the branch, "good idea" or not; then we should fix things as needed on the trunk.

Comment 10

[Peter Kasting]

Fixed on branch.

/mozilla/browser/themes/winstripe/browser/browser.css 1.17.2.62

Comment 11

[Peter Kasting]

Clearing blocking request

Comment 12

[Pam Greene]

Attachment: Adds hover bevels to Go and Search buttons in text mode

(In reply to comment #6)
> (From update of attachment 238486 [details] [diff] [review] [edit])
> hover feedback might be nice, but not critical at all

Sure, here you go!

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 238549 [details] [diff] [review]
Adds hover bevels to Go and Search buttons in text mode

>Index: winstripe/browser/browser.css

>+toolbar[mode="text"] #go-button {
>+  -moz-binding: url(chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton);
>+  -moz-appearance: toolbarbutton;
>+}

Why is the -moz-binding rule necessary? Nothing overrides the base toolbarbutton rule from what I can see.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> >+toolbar[mode="text"] #go-button {
> >+  -moz-binding: url(chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton);
> >+  -moz-appearance: toolbarbutton;
> >+}
> 
> Why is the -moz-binding rule necessary? Nothing overrides the base
> toolbarbutton rule from what I can see.

In other words, the toolbarbutton is already bound to that binding by the toolbarbutton rules in xul.css (DOMi with a current branch build confirms that, it wouldn't work as a button otherwise), so it shouldn't be necessary to specify it again here.

Comment 15

[Pam Greene]

(In reply to comment #14)
> In other words, the toolbarbutton is already bound to that binding by the
> toolbarbutton rules in xul.css (DOMi with a current branch build confirms that,
> it wouldn't work as a button otherwise), so it shouldn't be necessary to
> specify it again here.

Curious.  Before I added that line, the button grew to an unreasonable height, and re-applying the binding (perhaps to override a different rule?) was the simplest solution I found.  But now I can't reproduce the problem; perhaps my build was out of date.

However, I'm now realizing that the behavior I saw on Linux and thought was correct was in fact not right: the urlbar and searchbar stretch too tall in text mode.  (It looks okay to my eye, but it doesn't match the other modes, so it's disconcerting.)

So I'm withdrawing this second patch for FF2. (The bug still stays marked FIXED, since the background-image issue was fixed by the previous patch.)

Comment 16

[Peter Kasting]

Comment on attachment 238549 [details] [diff] [review]
Adds hover bevels to Go and Search buttons in text mode

I'm obsoleting Pam's patch to make it clear that it did not, and should not, go in.