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

RESOLVED FIXED

Status

()

Core
XUL
--
trivial
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Stefan Borggraefe)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

4.11 KB, patch
neil@parkwaycc.co.uk
: review+
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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
(Assignee)

Comment 1

15 years ago
Taking bug.
Assignee: hyatt → borggraefe
(Assignee)

Comment 2

15 years ago
Created attachment 134320 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #134320 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Comment 3

15 years ago
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-
(Assignee)

Comment 4

15 years ago
Created attachment 134323 [details] [diff] [review]
Fix V1.1

Put the initial size on the tree.
Attachment #134320 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #134323 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Comment 5

15 years ago
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-
(Reporter)

Comment 6

15 years ago
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)
(Assignee)

Comment 7

15 years ago
Neil: All three test cases work as expected with my first patch "Fix".
(Reporter)

Comment 8

15 years ago
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.
(Assignee)

Comment 9

15 years ago
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.
(Assignee)

Comment 10

15 years ago
Created attachment 134899 [details] [diff] [review]
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.
Attachment #134323 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #134899 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

15 years ago
Attachment #134899 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 11

15 years ago
Created attachment 134901 [details] [diff] [review]
Fix V1.3

Replaces the <separator> with a <spacer>.
Attachment #134899 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #134901 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Updated

15 years ago
Attachment #134901 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Updated

15 years ago
Attachment #134901 - Flags: superreview?(dbaron)
Attachment #134901 - Flags: superreview?(dbaron) → superreview?(bugs)
(Reporter)

Comment 13

15 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

10 years ago
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.