490655 - Clear recent history dialog too wide [en-US]

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Drew Willcoxon :adw]

We took the tree out of the recent clear recent history dialog refresh (bug 480169) at the last minute.  The dialog is a little too wide as a result.

Although the dialog's width is set via a DTD entity, string freeze was a month before the decision to yank the tree, so it may be too wide in other locales also.

Comment 1

[Axel Hecht [:Pike]]

This change doesn't have an l10n impact aside from being noisy on the files that localizers read. Thus, no entity change, OK with me to take for 3.5 RC1.

Comment 2

[Drew Willcoxon :adw]

Attachment: patch

Not sure who's best to review this.  It's basically a visual change, so asking Alex.  28em is pretty much the minimum width that fully displays the duration dropdown in all three themes.

Comment 3

[Johnathan Nightingale [:johnath]]

Comment on attachment 375064 [details] [diff] [review]
patch

># HG changeset patch
># User Drew Willcoxon <[e-mail]>
># Date 1241024879 25200
># Node ID 2da9cfcd0892125a5f9f85ecee6b35a54c697c23
># Parent  3a27bee3d7b5b4e2447a1e578ed8436ae71063d8
>Bug 490655 - Clear recent history dialog too wide [en-US]
>
>diff --git a/browser/locales/en-US/chrome/browser/sanitize.dtd b/browser/locales/en-US/chrome/browser/sanitize.dtd
>--- a/browser/locales/en-US/chrome/browser/sanitize.dtd
>+++ b/browser/locales/en-US/chrome/browser/sanitize.dtd
>@@ -58,5 +58,5 @@ that require it.  -->
>      mockup at bug 480169 -->
> <!ENTITY sanitizeEverythingUndoWarning     "This action cannot be undone.">
> 
>-<!ENTITY dialog.width                 "32em">
>+<!ENTITY dialog.width                 "28em">
> <!ENTITY column.width                 "14em">

If it makes axel happy, it makes me happy.  Have you tested to ensure that things word wrap correctly when they need to?  (I know you added flex in the other bug - do these need to land together?)

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #3)
> If it makes axel happy, it makes me happy.  Have you tested to ensure that
> things word wrap correctly when they need to?  (I know you added flex in the
> other bug - do these need to land together?)

Things (by which I mean the warning text) don't wrap at all without the flex fix in bug 489700.  Big goof on my part.  However, even with the narrower dialog of this bug it's not a problem for en-US because the text is pretty short.  It's probably a problem for some other locales.

Since this bug is for en-US only, the two patches can land in either order.

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/066577acf064

Comment 6

[Luke Iliffe (Harlequin99)]

Based on comment 1 will this land on the 1.9.1 branch? AFAICT this doesn't have a requested 3.5 flag yet.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 375064 [details] [diff] [review]
patch

Requesting approval, very low risk, visual polish to CRH dialog, Axel says no l10n impact and fine with him to take in 3.5 (comment 1).

Comment 8

[(no longer active)]

FWIW, this has caused bug 490655.  Since we don't want to fix a polish problem here and cause another one there, this should happen for 1.9.1 alongside the fix to that bug.

Comment 9

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

Comment on attachment 375064 [details] [diff] [review]
patch

a191=beltzner

Comment 10

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

Comment on attachment 375064 [details] [diff] [review]
patch

Oops, missed that this caused a regression. I'll approve a roll-up

Comment 11

[Drew Willcoxon :adw]

Sorry Ehsan!  I think I've been making your life harder with these privacy and CRH bugs... :\

I'm not sure what "roll-up" means.  Bug 493485 is handling the regression on trunk, so given the fact this has been a-'ed, should the 1.9.1 patch go there as well?

Comment 12

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

Both patches should be combined for a 1.9.1 patch, yes.

Comment 13

[Johnathan Nightingale [:johnath]]

As whimboo said - the a- here is just because we don't like approving patches that need to land with other patches - those soft dependencies tend to get lost, and then everyone is sad. If you can combine the two co-dependent changes into 1 patch, we should get it reviewed and landed post-haste.

Comment 14

[Drew Willcoxon :adw]

Attachment: 1.9.1 patch

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 378436 [details] [diff] [review]
1.9.1 patch

Looks good to me on Mac

Comment 16

[Johnathan Nightingale [:johnath]]

Comment on attachment 378436 [details] [diff] [review]
1.9.1 patch

This is a 1 line polish change for 1.9.1.  Earlier comments made it look like this was tied to the patch in bug 493485, but really it isn't.  The trunk landing here made things too thin and bug 493485 was filed to fix it. This 1.9.1 only patch could live over there, but \/\/ - it should land. It's just the "fixed" version of the trunk patch in this bug.

Confused now?

Comment 17

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

Comment on attachment 378436 [details] [diff] [review]
1.9.1 patch

a191=beltzner

This is the first patch approval I have given due to hopeless confusion.

Comment 18

[Dão Gottwald [:dao]]

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

Comment 19

[Ton [:Tonnes]]

I noticed these values are being shared by the ‘Clear recent history’ and the ‘Settings for clearing history’ dialog. Wouldn’t it be wise to use separate values for the latter? That dialog could use smaller values for en-US already, probably for other locales as well. For Dutch/nl for instance, we currently need to use no less than 34/17 to make the lines in ‘Clear recent history’ fit on one line, which causes the Clear recent history dialog to use even more ‘white space’ than it currently does for en-US.

Comment 20

[Ton [:Tonnes]]

(In reply to comment #19)
> no less than 34/17 to make the lines in ‘Clear recent history’ fit on one line

I meant the lines in ‘Settings for clearing history’, of course.

Comment 21

[Drew Willcoxon :adw]

(In reply to comment #19)
> ‘Settings for clearing history’ dialog. Wouldn’t it be wise to use separate
> values for the latter?

Totally.  Filed bug 494210.

Comment 22

[Aakash Desai [:aakashd]]

How can this be verified?

Comment 23

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

Simply check bug 493485. This dialog is too narrow now on all platforms. But lets verify this one on trunk and 1.9.1.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre ID:20090526031623

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre ID:20090526031155