223294 - <dialog>s don't need trailing <separator>s

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

As per bug 187466 if dialog buttons are too close to dialog content it's a theme
issue. However there are still old dialogs out there with trailing separators,
for example, commonDialog.xul

Comment 1

[Stefan Borggraefe]

Taking bug.

Comment 2

[Stefan Borggraefe]

Attachment: Fix

Removes the trailing separators.

There is more of this here:
calendar/resources/content/pref/pref.xul
extensions/irc/xul/content/prefpanel/appearance-previewCSS.xul
toolkit/mozapps/downloads/content/editAction.xul
browser/components/prefwindow/content/pref.xul
browser/components/prefwindow/content/plugins.xul
browser/components/prefwindow/content/editAction.xul
browser/components/bookmarks/content/selectBookmark.xul
browser/components/bookmarks/content/addBookmark.xul

But I guess this needs to be handled in separate patches/bugs in order to get
the right reviewers.

Comment 3

[[e-mail]]

Comment on attachment 134320 [details] [diff] [review]
Fix

>@@ -42,6 +42,7 @@
>         title="&newBookmark.title;" selectFolderTitle="&selectFolder.title;"
>         onload="Startup();"
>         onunload="persistTreeSize();"
>+        style="width: 36em;"
>         persist="screenX screenY"
>         screenX="24" screenY="24">
>    
>@@ -106,7 +107,5 @@
>               oncommand="createNewFolder();"/>
>     </hbox>
>   </vbox>
>-
>-  <separator style="width: 36em"/>
> 
> </dialog>
Sorry, this doesn't work. This dialog needs special care because the size is
persisted on the tree rather than on the dialog, so that you can't put size on
the dialog.

Comment 4

[Stefan Borggraefe]

Attachment: Fix V1.1

Put the initial size on the tree.

Comment 5

[[e-mail]]

Comment on attachment 134323 [details] [diff] [review]
Fix V1.1

Ah, now the problem here is the bookmark manager's file new bookmark dialog...

Comment 6

[[e-mail]]

So, basically there are three tests:
1. In Navigator, Bookmarks/File Bookmark (resizeable persist)
2. In Bookmarks Manager, File/New/Bookmark (fixed width 36em)
3. In Bookmarks Maneger, Move Bookmark (resizeable persist)

Comment 7

[Stefan Borggraefe]

Neil: All three test cases work as expected with my first patch "Fix".

Comment 8

[[e-mail]]

Sorry, but with your first patch, the window is always 36em wide, and with your
second patch, the second window isn't 36em wide when it should be.

Comment 9

[Stefan Borggraefe]

Neil: You are right. Sorry for the fuss. :-( I only tried to make the window
larger, not smaller. This worked with the first patch and so I didn't realise
the problem here.

Comment 10

[Stefan Borggraefe]

Attachment: Fix V1.2

I think the <separator> in addBookmark.xul has to stay then since all other
elements inside the <dialog> are hidden in some situations. So I just gave the
<separator> a height of 0em and added a comment to it.

Comment 11

[Stefan Borggraefe]

Attachment: Fix V1.3

Replaces the <separator> with a <spacer>.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 134901 [details] [diff] [review]
Fix V1.3

sr=[e-mail]

Comment 13

[[e-mail]]

Fix checked in.