Closed Bug 480898 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: robert.strong.bugs, Assigned: ehsan)

Details

(Keywords: rtl, verified1.9.1)

Attachments

(2 files, 9 obsolete files)

Attached patch possible patch (obsolete) — Splinter Review
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.
Attached image screenshots (obsolete) —
Attached patch patch rev1 (obsolete) — Splinter Review
Also sets the window size
Assignee: nobody → robert.bugzilla
Attachment #364858 - Flags: review?(mconnor)
Attached image screenshots (obsolete) —
Attachment #364854 - Attachment is obsolete: true
Attachment #364855 - Attachment is obsolete: true
This window will be completely changed by bug 480169. But I've no idea about any ETA.
Attachment #364858 - Attachment is obsolete: true
Attachment #364858 - Flags: review?(mconnor)
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
Keywords: rtl
(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...
Attached image screenshot w/ fix from bug 390734 (obsolete) —
The hbox for groupbox-title ends up extending beyond the groupbox itself which causes the caption to be in the incorrect position.
Attachment #364859 - Attachment is obsolete: true
Attached patch patch rev2 (obsolete) — Splinter Review
Hey Johnathan, this is still a tad hacky but I believe it is an improvement over the existing layout and it fixes this bug.
Attachment #365346 - Attachment is obsolete: true
Attachment #367303 - Flags: review?(johnath)
Attached image screenshots
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.
Attachment #367303 - Flags: review?(johnath) → review?(mconnor)
Attached patch patch rev3 (obsolete) — Splinter Review
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.
Attachment #367303 - Attachment is obsolete: true
Attachment #367593 - Flags: review?(mconnor)
Attachment #367303 - Flags: review?(mconnor)
mconnor, I've fixed the indentation locally
Attachment #367593 - Flags: review?(mconnor)
Assignee: robert.bugzilla → nobody
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.
OS: Windows Vista → All
Hardware: x86 → All
Version: unspecified → Trunk
(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.
Attached patch Reworked patch (obsolete) — Splinter Review
Assignee: nobody → ehsan.akhgari
Attachment #367593 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #376618 - Flags: review?(gavin.sharp)
Could you ask Johnathan and/or Drew (:adw) to review this instead? I think their review here is sufficient.
Comment on attachment 376618 [details] [diff] [review]
Reworked patch

Sure.
Attachment #376618 - Flags: review?(johnath)
Attachment #376618 - Flags: review?(gavin.sharp)
Attachment #376618 - Flags: review?(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.
Attachment #376618 - Flags: review?(adw) → review-
Attached patch New patch (obsolete) — Splinter Review
(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.
Attachment #376618 - Attachment is obsolete: true
Attachment #376959 - Flags: review?(johnath)
Attachment #376959 - Flags: review?(adw)
Attachment #376618 - Flags: review?(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?
Attached patch Revised patchSplinter Review
(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.  :-)
Attachment #376959 - Attachment is obsolete: true
Attachment #377639 - Flags: review?(johnath)
Attachment #377639 - Flags: review?(adw)
Attachment #376959 - Flags: review?(johnath)
Attachment #376959 - Flags: review?(adw)
Attachment #377639 - Flags: review?(johnath) → review+
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!
Attachment #377639 - Flags: review?(adw) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/16b00bbde2f8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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.
Attachment #377639 - Flags: approval1.9.1?
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.
The first semicolon delimits the entity, the second one delimits the CSS declaration.
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.
Status: RESOLVED → VERIFIED
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.
(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... :-)
Flags: in-litmus?
Flags: in-testsuite-
Flags: in-litmus-
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?
(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...
No longer depends on: 493485
(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.
(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...
(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 on attachment 377639 [details] [diff] [review]
Revised patch

a191=beltzner
Attachment #377639 - Flags: approval1.9.1? → approval1.9.1+
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
You need to log in before you can comment on or make changes to this bug.