349055 - [linux only] Hover state of Go button highlight extends outside of Go button

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tracy Walker [:tracy]]

Bon echo nightly of 2006081704 on Linux.

-Hover over the Go button

tested results:
The highlight extends outside of the button itself.

expected results:
Only the Go button is highlighted.

Comment 1

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

Hrm, the patch that landed from bug 347416 should have fixed this...

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Tracy, can you verify that this is still a problem and renom if it is, as per Gavin's comment above?

Comment 3

[Tracy Walker [:tracy]]

This is still present with todays (20060818) Bon Echo build on linux.

renom 

Comment 4

[Mike Connor [:mconnor]]

-> myk

Comment 5

[Michael Wu [:mwu]]

Attachment: Remove background color on toolbarbutton hover

myk: Does this patch help?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

I'm not seeing this anymore with the latest nightlies.  Tracy, are you still seeing it?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

After switching to the "Smokey Blue" GNOME theme, I can once again reproduce the problem.  I didn't see it before because it doesn't manifest with the Ubuntu default theme (Human).

mwu's patch fixes the problem.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

As mwu noted on IRC, however, his patch partly regresses the accessibility fix for bug 251492, since it eliminates the native background that makes high-contrast themes work.  With the patch applied, the go (and also the search) button remain low-contrast even with a high-contrast theme applied.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: -padding +moz-border-radius to trim hover background

The button has padding, which makes its total size be larger than the image which now draws its borders.  This patch removes that padding.

That mostly solves the problem, but the hover highlight still extends outside the image-drawn border in the corners where the border is rounded.  So the patch also adds sufficient moz-border-radius to those corners to make the hover highlight fit.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 235777 [details] [diff] [review]
-padding +moz-border-radius to trim hover background

Erm, original title reflected earlier version of patch.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 235777 [details] [diff] [review]
-padding +moz-border-radius to trim hover background

do we really need the :not selector?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: better fix, also handles Search button

> do we really need the :not selector?

We could define border radius on the buttons without using a :not selector, but then we'd have to undefine those values when setting the border radius for RTL, i.e.:

#go-button {
   list-style-image: url("chrome://browser/skin/Go.png");
  -moz-border-radius-topright: 9px;
  -moz-border-radius-bottomright: 9px;
}
#go-button[chromedir="rtl"] {
   list-style-image: url("chrome://browser/skin/Go-rtl.png");
  -moz-border-radius-topright: 0;
  -moz-border-radius-bottomright: 0;
  -moz-border-radius-topleft: 9px;
  -moz-border-radius-bottomleft: 9px;
}

Using a :not rule seems cleaner.


This patch does the following:

1. Integrates the fix in mwu's patch.  Toolkit CSS sets the hover background color of all toolbar buttons to the system hover background color, but the only buttons which actually need that rule are those containing an image that draws its own button (i.e. the Go and Search Go buttons), and mwu thinks we'll be less prone to whack-a-mole background problems in the future if we specify the rule only on those buttons that need it.

Fixed by removing the rule from toolkit CSS and adding it to the CSS for the Go and Search Go buttons.

2. Fixes the Search Go button in addition to the URL Go button.  The Search Go button extra space is more complicated, since it isn't due to padding on the button itself.  It turns out to be a combination of align="stretch" (the default setting) on the button container, which creates the extra vertical space, and a "margin-end-value: 2px" rule for .toolbarbutton-icon in toolbarbutton.css, which creates the extra horizontal space.

Fixed by setting align="center" on the button container and specifying a 0 margin for .toolbarbutton-icon elements inside Search Go buttons.

3. Removes references to the "go-container", since there is no longer any element with that ID.  Also removes some conflicting rules for the Go button in pinstripe, preferring new rules which mconnor and mano landed recently to older rules that conflict with them.

4. Makes the Search Go button point to the left in RTL mode.  The CSS selector which determines RTL mode checks for chromedir="rtl", but the "chromedir" attribute isn't getting set on the button tag, so the regular LTR button gets used in RTL mode.

Fixed by setting the attribute on the tag.

5. Uses a slightly larger border radius setting for the two buttons.  I noticed some artifacts when hovering/activating at the 8px setting for both the Go and the Search Go buttons, probably because the radius isn't lining up exactly with the anti-aliased curved border in the button image itself.

Fixed by setting the radius to 10px for the Go button and 9px for the Search Go button (couldn't make them consistent because each exhibits artifacts at the other setting).

I've built and tested with this patch on Linux and Mac.  I haven't tried Windows yet, but I'll do so tomorrow when I'm in front of a Windows box at the office.

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 235543 [details] [diff] [review]
Remove background color on toolbarbutton hover

This fixes a number of gtk2 theming bugs.

Comment 14

[Peter Kasting]

If we're removing the hoverface color, shouldn't we also remove hovertext?  I'm worried that we could get strange results otherwise.

Comment 15

[Michael Wu [:mwu]]

(In reply to comment #14)
> If we're removing the hoverface color, shouldn't we also remove hovertext?  I'm
> worried that we could get strange results otherwise.
> 

No, because:
1. The background color stuff isn't actually getting removed for buttons that still use the native theme engine. The native theme engine has always painted over the background color that we pick. So we want the text color to continue to match. And for buttons which have the native theme engine turned off, we really want the background color turned off.
2. In practice for most themes, the hovertext color isn't different from the normal text color.

There is the case where the color of the text will be different for buttons which have text and also have native theming turned off. They already looked weird with background color. This won't make things any worse, and fixes a number of visual issues people actually care about.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 235543 [details] [diff] [review]
Remove background color on toolbarbutton hover

r+a=me on this branch theme bug

Comment 17

[Michael Wu [:mwu]]

Trunk:
Checking in toolkit/themes/gnomestripe/global/toolbarbutton.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/toolbarbutton.css,v  <--  toolbarbutton.css
new revision: 1.13; previous revision: 1.12
done

Branch:
Checking in toolkit/themes/gnomestripe/global/toolbarbutton.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/toolbarbutton.css,v  <--  toolbarbutton.css
new revision: 1.10.2.1; previous revision: 1.10
done

Comment 18

[Michael Wu [:mwu]]

*** Bug 348939 has been marked as a duplicate of this bug. ***

Comment 19

[Tracy Walker [:tracy]]

verified with 2.0.0.4 on linux