480898 - Both sanitize.xul use pseudo columns in em which mess with bounding elements

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Robert Strong (they/them - no direct email)]

Attachment: possible patch

Found while working on bug 390734

Rhe Sanitize dialogs uses 14em to create psuedo columns inside its groupbox along with a width of 30em... the 14em is way too large for this considering the window is only 2em larger and there is also a groupbox as the container for the columns.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: screenshots

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Also sets the window size

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: screenshots

Comment 4

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

This window will be completely changed by bug 480169. But I've no idea about any ETA.

Comment 5

[Robert Strong (they/them - no direct email)]

Ehsan, just a heads up that these psuedo columns mess up the width for the groupbox caption in rtl mode which is much more obvious with the patch from Bug 390734

Comment 6

[(no longer active)]

(In reply to comment #5)
> Ehsan, just a heads up that these psuedo columns mess up the width for the
> groupbox caption in rtl mode which is much more obvious with the patch from Bug
> 390734

Robert, can you please explain what exactly is different in this regard between LTR and RTL layouts?

And Johnath, is there any particular reason why you didn't use a grid here?  I think a grid would guarantee the correct layout, and should be the natural choice for this kind of layout, unless I'm missing something...

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: screenshot w/ fix from bug 390734

The hbox for groupbox-title ends up extending beyond the groupbox itself which causes the caption to be in the incorrect position.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Hey Johnathan, this is still a tad hacky but I believe it is an improvement over the existing layout and it fixes this bug.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: screenshots

Comment 10

[Johnathan Nightingale [:johnath]]

Comment on attachment 367303 [details] [diff] [review]
patch rev2

(In reply to comment #8)
> Created an attachment (id=367303) [details]
> patch rev2
> 
> Hey Johnathan, this is still a tad hacky but I believe it is an improvement
> over the existing layout and it fixes this bug.

I agree that it's an improvement, definitely.  I also agree that grid is the obvious way to do this, so I'm having a lot of trouble remembering why I used pseudo columns instead.

Henrik's right that bug 480169 has the potential to change this (at least for one of the two dialogs, the preferences one would likely remain unchanged) but since we don't have an ETA on that, this still feels worthwhile (and in any case is certainly worthwhile for the pref version).

You can have my review if you want it, but I'm not a module peer here, so it should probably get mconnor's eyes, since he reviewed the original.  Shifting the request to him.

>diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul
>--- a/browser/base/content/sanitize.xul
>+++ b/browser/base/content/sanitize.xul
...
>         </menulist>
>         <label value="&clearDuration.suffix;" flex="1"/>
>       </hbox>
>       <hbox>
>-        <vbox style="width: &column.width;">
>-          <checkbox id="history-checkbox"

Is it deliberate that you left the wrapper hbox here?  I think we can drop it now that the columns live in a grid tag, no?

>+        <grid flex="1">
>+          <columns>
>+            <column style="width: &column.width;"/>
>+            <column flex="1"/>
>+          </columns>

I guess resizing the dialog will only add space to the second column? Shouldn't be a problem, since the reason I'd be resizing would be to fit the labels more appropriately in other locales, and the column width is locale-dependent already.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

Thanks Johnathan

(In reply to comment #10)
>...
> >diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul
> >--- a/browser/base/content/sanitize.xul
> >+++ b/browser/base/content/sanitize.xul
> ...
> >         </menulist>
> >         <label value="&clearDuration.suffix;" flex="1"/>
> >       </hbox>
> >       <hbox>
> >-        <vbox style="width: &column.width;">
> >-          <checkbox id="history-checkbox"
> 
> Is it deliberate that you left the wrapper hbox here?  I think we can drop it
> now that the columns live in a grid tag, no?
You are correct... thanks

> >+        <grid flex="1">
> >+          <columns>
> >+            <column style="width: &column.width;"/>
> >+            <column flex="1"/>
> >+          </columns>
> 
> I guess resizing the dialog will only add space to the second column? Shouldn't
> be a problem, since the reason I'd be resizing would be to fit the labels more
> appropriately in other locales, and the column width is locale-dependent
> already.
Correct but the locales can still control this with the window width and column width properties.

Comment 12

[Robert Strong (they/them - no direct email)]

mconnor, I've fixed the indentation locally

Comment 13

[(no longer active)]

Robert, is your patch still applicable for browser/components/preferences/sanitize.xul?  I don't see this problem on Mozilla/5.0 (Windows; U; Windows NT 6.0; fa; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4.

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to comment #13)
> Robert, is your patch still applicable for
> browser/components/preferences/sanitize.xul?  I don't see this problem on
> Mozilla/5.0 (Windows; U; Windows NT 6.0; fa; rv:1.9.1b4) Gecko/20090423
> Firefox/3.5b4.
No idea and I doubt I'll have time to look at this again in the near future. I suspect that fa provides a decent column width which would prevent this bug from happening.

Comment 15

[(no longer active)]

Attachment: Reworked patch

Comment 16

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

Could you ask Johnathan and/or Drew (:adw) to review this instead? I think their review here is sufficient.

Comment 17

[(no longer active)]

Comment on attachment 376618 [details] [diff] [review]
Reworked patch

Sure.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 376618 [details] [diff] [review]
Reworked patch

Looks OK, but the patch doesn't apply cleanly because of all the string changes to sanitize.xul in bug 480169.  Should be pretty simple to fix.

Comment 19

[(no longer active)]

Attachment: New patch

(In reply to comment #18)
> (From update of attachment 376618 [details] [diff] [review])
> Looks OK, but the patch doesn't apply cleanly because of all the string changes
> to sanitize.xul in bug 480169.  Should be pretty simple to fix.

Ah, I'm sorry, apparently I attached this patch before qrefreshig or something.

Comment 20

[Johnathan Nightingale [:johnath]]

Comment on attachment 376959 [details] [diff] [review]
New patch

>     <groupbox orient="horizontal">
>-      <caption label="&historySection.label;"/>
...

>     <groupbox orient="horizontal">
>-      <caption label="&dataSection.label;"/>

Why are you removing the groupbox captions for the two sections?

Comment 21

[(no longer active)]

Attachment: Revised patch

(In reply to comment #20)
> (From update of attachment 376959 [details] [diff] [review])
> >     <groupbox orient="horizontal">
> >-      <caption label="&historySection.label;"/>
> ...
> 
> >     <groupbox orient="horizontal">
> >-      <caption label="&dataSection.label;"/>
> 
> Why are you removing the groupbox captions for the two sections?

My bad.  This should hopefully be the last revision.  :-)

Comment 22

[Johnathan Nightingale [:johnath]]

Comment on attachment 377639 [details] [diff] [review]
Revised patch

Looks good to me - tested on my mac build and it looks the same visually.  Thanks!

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 377639 [details] [diff] [review]
Revised patch

The landing of bug 489700 bitrotted your patch. :) Just the preferences.css part.  Other than that, looks good.

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/16b00bbde2f8

Comment 25

[(no longer active)]

Comment on attachment 377639 [details] [diff] [review]
Revised patch

This is a very low risk fix for RTL locales, nominating to land for 1.9.1.

Comment 26

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

Comment on attachment 377639 [details] [diff] [review]
Revised patch

>--- a/browser/components/preferences/sanitize.xul
>+++ b/browser/components/preferences/sanitize.xul
>+            style="width: &dialog.width;;"

Nit: One semicolon should be enough.

Comment 27

[Dão Gottwald [:dao]]

The first semicolon delimits the entity, the second one delimits the CSS declaration.

Comment 28

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

And the second one we don't need because it's only one attribute we are specifying.

Verified fixed on all platforms: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre ID:20090517031025

Ehsan, do you really think that we need a Litmus test here? It's a one-time change and should not regress.

Comment 29

[Dão Gottwald [:dao]]

More to the point, it's the last declaration (the number of preceding declarations is irrelevant). That's why the semicolon isn't mandatory. Yet it's conventional in Mozilla code.

Comment 30

[(no longer active)]

(In reply to comment #28)
> Ehsan, do you really think that we need a Litmus test here? It's a one-time
> change and should not regress.

OK, if you don't think a Litmus test is necessary for this, then I guess the verification alone should be enough... :-)

Comment 31

[AKIHIRO Misaki (a.k.a Kuden)]

http://hg.mozilla.org/mozilla-central/annotate/38d50cc03a72/browser/base/content/sanitize.xul#l45
> <?xml-stylesheet href="chrome://browser/skin/preferences/preferences.css" type="text/css"?> 

Is this line still necessary?

Comment 32

[(no longer active)]

(In reply to comment #31)
> http://hg.mozilla.org/mozilla-central/annotate/38d50cc03a72/browser/base/content/sanitize.xul#l45
> > <?xml-stylesheet href="chrome://browser/skin/preferences/preferences.css" type="text/css"?> 
> 
> Is this line still necessary?

Yes, there are a few rules there which are still being used...

Comment 33

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #32)
> Yes, there are a few rules there which are still being used...

Oh, I see.. Gnomestripe and Pinstripe still use skin/preferences/preferences.css. 

sanitize.xul uses skin/sanitizeDialog.css now. 
The necessity that uses skin/preferences/preferences.css is not understood. 
When the cleanup is done, I hope those rules move to skin/sanitizeDialog.css.

Comment 34

[(no longer active)]

(In reply to comment #33)
> (In reply to comment #32)
> > Yes, there are a few rules there which are still being used...
> 
> Oh, I see.. Gnomestripe and Pinstripe still use
> skin/preferences/preferences.css. 
> 
> sanitize.xul uses skin/sanitizeDialog.css now. 
> The necessity that uses skin/preferences/preferences.css is not understood. 
> When the cleanup is done, I hope those rules move to skin/sanitizeDialog.css.

This is a valid point, but you should probably file a new bug about it...

Comment 35

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #34)
> This is a valid point, but you should probably file a new bug about it...

Thank you. I filed Bug 493738.

Comment 36

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

Comment on attachment 377639 [details] [diff] [review]
Revised patch

a191=beltzner

Comment 37

[(no longer active)]

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

Comment 38

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

Verfied fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214