489700 - Focus ring is cut off for XUL dropdown in "Clear Recent History..." dialog

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Jesse Ruderman]

Attachment: screenshot

Comment 2

[Drew Willcoxon :adw]

browser/themes/pinstripe/browser/preferences/preferences.css adds 10 pixels to the bottom padding of the hbox containing the dropdown, pushing the dropdown up against the top of the hbox.

Comment 3

[Markus Stange [:mstange]]

Removing that doesn't fix it - that just removes the space under the dropdown. The fact that the focus ring is cut off is caused by the overflow styles on the .content-box and .paneDeckContainer of the prefpane / prefwindow. I'm not sure why they're there - one was added in bug 274712, the other in bug 422476.

Comment 4

[Drew Willcoxon :adw]

Actually when I remove this from preferences.css it looks fine:

#sanitizeDurationChoice {
  margin: 0; 
}

Comment 5

[Markus Stange [:mstange]]

Oh, yeah. I think that still cuts off one pixel, but we can live with that.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch v1

Moves some CSS that only applies to the CRH dialog from browser/themes/pinstripe/browser/preferences/preferences.css to the CRH's sanitizeDialog.css.  pinstripe's dropdown should have regular margins so as not to mess up its focus ring.  The dropdown on the other two themes should have 0 right margins so that they align with the dialog buttons and clear-everything warning box.  The sanitizeDurationLabel style is not needed anywhere.

Not sure if mconnor is the best person to bother for review.  If anyone knows better, please let me know.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 374530 [details] [diff] [review]
patch v1

>--- a/browser/themes/gnomestripe/browser/sanitizeDialog.css
>+++ b/browser/themes/gnomestripe/browser/sanitizeDialog.css

>+#sanitizeDurationChoice {
>+  margin-right: 0;
>+}

this should be -moz-margin-end

Comment 8

[Drew Willcoxon :adw]

Attachment: patch v2

Thanks Dão.  This patch replaces all occurrences of margin-left, padding-left, etc. with -moz-margin-start, etc.

It also flexes an inner box used in the warning.  Without this flex the warning text does not wrap.  That's not a problem with en-US, but it probably is for some locales.  I can spin out a new bug that points to this one if need be.

Comment 9

[Drew Willcoxon :adw]

Oh, and my comment in comment 6 about the sanitizeDurationLabel style is not correct; it's needed to align the start of label with the start of the other elements.  patch v2 adds this style back in.

Comment 10

[Dão Gottwald [:dao]]

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

>--- a/browser/themes/winstripe/browser/sanitizeDialog.css
>+++ b/browser/themes/winstripe/browser/sanitizeDialog.css
>@@ -1,8 +1,23 @@
>+#SanitizeDurationBox {
>+  padding-bottom: 10px;
>+}

You might want to add <separator class="thin"/> to the xul file.

> #sanitizeEverythingWarningDescBox {
>   padding: 0;
>   margin: 0;
>-  padding-left: 16px;
>+  -moz-padding-start: 16px;
>+  -moz-padding-end: 16px;
> }

padding: 0 16px;

>-/* Make the rightmost dialog button ("cancel") align wih right side of the
>+/* Align the end of the rightmost dialog button ("accept") with the end of the
>    warning box */

You accidentally changed "cancel" to "accept". How about just "last" instead of "end of the rightmost"?

Comment 11

[Drew Willcoxon :adw]

Attachment: patch v3

Addressed comment 10.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 375736 [details] [diff] [review]
patch v3

-> johnath, as he should be primary reviewer here :)

Comment 13

[Johnathan Nightingale [:johnath]]

Comment on attachment 375736 [details] [diff] [review]
patch v3

This looks fine to me, and I've tested it on Mac. You're moving styles from preferences.css to sanitize.css even though the preference-pane version of the sanitize dialog still uses the former, but none of the changes should hurt that, since they're duration-related.

>+++ b/browser/themes/gnomestripe/browser/preferences/preferences.css
>+++ b/browser/themes/gnomestripe/browser/sanitizeDialog.css
>+++ b/browser/themes/pinstripe/browser/preferences/preferences.css
>+++ b/browser/themes/pinstripe/browser/sanitizeDialog.css
>+++ b/browser/themes/winstripe/browser/preferences/preferences.css
>+++ b/browser/themes/winstripe/browser/sanitizeDialog.css

This bug was originally mac only, but you're changing windows and linux as well, and it's not just a straight copy (since you're removing padding styles).  The changes themselves look fine to me, but have you done a visual check on the other platforms just to ensure there's nothing surprising there?

Assuming they look okay, r=me.

Comment 14

[Drew Willcoxon :adw]

(In reply to comment #13)
> This looks fine to me, and I've tested it on Mac. You're moving styles from
> preferences.css to sanitize.css even though the preference-pane version of the
> sanitize dialog still uses the former, but none of the changes should hurt
> that, since they're duration-related.

Yes, the only chrome that uses those styles is the CRH dialog.  (Bug 480169 comment 64 was right.)

> This bug was originally mac only, but you're changing windows and linux as
> well, and it's not just a straight copy (since you're removing padding styles).

The padding-bottom on SanitizeDurationBox was removed in favor of a separator in the XUL, as suggested in comment 10.

>  The changes themselves look fine to me, but have you done a visual check on
> the other platforms just to ensure there's nothing surprising there?

Yes, I've checked Winstripe on Vista and Gnomestripe on Ubuntu.  They look good to me. :)

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a13202e6b62f

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 375736 [details] [diff] [review]
patch v3

Requesting approval, low risk, fixes focus ring issue on Pinstripe, makes the CSS better RTL-aware, fixes problem whereby warning text in dialog may not wrap.  Visually tested on all three themes.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090515 Minefield/3.6a1pre ID:20090515031031

Comment 18

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

Comment on attachment 375736 [details] [diff] [review]
patch v3

a191=beltzner

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd5445fe87c1

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre ID:20090520035357

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.