Closed Bug 223294 Opened 21 years ago Closed 20 years ago

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

Categories

(Core :: XUL, defect)

x86
Windows 2000
defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: Stefan.Borggraefe)

Details

Attachments

(1 file, 3 obsolete files)

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
Taking bug.
Assignee: hyatt → borggraefe
Attached patch Fix (obsolete) — Splinter Review
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.
Attachment #134320 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #134320 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Fix V1.1 (obsolete) — Splinter Review
Put the initial size on the tree.
Attachment #134320 - Attachment is obsolete: true
Attachment #134323 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 134323 [details] [diff] [review]
Fix V1.1

Ah, now the problem here is the bookmark manager's file new bookmark dialog...
Attachment #134323 - Flags: review?(neil.parkwaycc.co.uk) → review-
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)
Neil: All three test cases work as expected with my first patch "Fix".
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.
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.
Attached patch Fix V1.2 (obsolete) — Splinter Review
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.
Attachment #134323 - Attachment is obsolete: true
Attachment #134899 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134899 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Fix V1.3Splinter Review
Replaces the <separator> with a <spacer>.
Attachment #134899 - Attachment is obsolete: true
Attachment #134901 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134901 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #134901 - Flags: superreview?(dbaron)
Attachment #134901 - Flags: superreview?(dbaron) → superreview?(bugs)
Fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: