customize toolbar dialog should be a "sheet"

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: pbaker, Unassigned)

Tracking

(Blocks: 2 bugs, {polish})

unspecified
PowerPC
Mac OS X
polish
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: see bug 350129)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030521 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030521 Mozilla Firebird/0.6

The Customize Toolbar should not be displayed as a separate dialog but instead
as a "sheet" that slides down from the tool bar. Preferenecs on the other hand
is done as a "sheet" but should in fact be a separate dialog window. I will
create another bug for that.

Reproducible: Always

Steps to Reproduce:
1. Choose Customize... from the View->Toolbars menu.

Actual Results:  
Customize Toolbar is presented as a dialog which most likely obscures the
toolbar you are trying to customize.

Expected Results:  
Customize Toolbar should be presented as a "sheet" that slides down from the
tool bar.
(Reporter)

Comment 1

16 years ago
Created attachment 123909 [details]
snapshot of firebird incorrectly displaying customize toolbar as a dialog
(Reporter)

Comment 2

16 years ago
Created attachment 123910 [details]
snapshot of camino correctly displaying customize toolbar as a sheet

Comment 3

16 years ago
Confirmed using Firebird/Mac/2003-05-16 (0.6).

Comment 4

16 years ago
Really marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

16 years ago
a similar thing happens with gtk2 builds on linux, which may or may not be related

Comment 6

15 years ago
Taking QA Contact, sorry for bugspam
QA Contact: asa → bugzilla

Comment 7

15 years ago
No, any Linux behavior is not related. This bug is about obeying the Apple UI
specifications--right or wrong.

Comment 8

15 years ago
Seeing bug 235033 today reminded me of this bug, and something occurred to me.

Crudely put, sheets should pretty much only be used when they pertain to a
single window, and you want the user to dismiss the sheet before continuing with
anything else in that window.

Isn't Customize Toolbar something that pertains to the entire application, not
just the foreground window? In that sense, I'd equate it with Preferences and
wonder if it should remain a modeless dialog (see bug 206651).
(Reporter)

Comment 9

15 years ago
It is window specific, because an application have multiple types windows with different toolbars. For 
instance, the download manager does not have the same toolbar as the browser window. Therefore, the 
customize toolbar sheet only applies to the the browser window and should be attached to it.

Comment 10

15 years ago
You can give the download manager a toolbar? :)

I see what you mean though. While configuring the toolbar really is a global
preference change (it sets the toolbar for all new browser windows, not just the
open one), it's implemented in such a way that it'll only interact with the
current window. So making it a sheet makes sense. At least in the way it's
currently implemented. Thanks, I wasn't sure :)

Comment 11

15 years ago
it's already a sheet on Windows, so it's weird (and backwards) that it's not on Mac.
Any progress?

By the way is hyatt still works for mozilla.org !? (while he works on Safari :) )

If the answer in "no", bug should be reassigned...

Updated

15 years ago
Assignee: hyatt → bugs
I thought this was a sheet on Mac at one point.

Well, it is a sheet on other platforms, and should be a sheet on Mac.  Plus,
when the dialog first comes up, it covers part of the toolbar, and the user
needs to move it out of the way.  Then, when making changes to the toolbar, the
dialog jumps around the screen a little.  It's all very unclean.
Flags: blocking1.0?
Moving blocking requsetto 1.0mac, since it's the right place for mac UI issues.
Flags: blocking1.0? → blocking1.0mac?

Updated

14 years ago
Flags: blocking1.0mac? → blocking1.0mac+
There are several problems with the customize toolbar dialog on Mac:
(1) The dialog should be a Mac OS X sheet:  This would be the best solution, but
I don't think it is possible.  Sheets are attached either to a window or to a
system toolbar.  We can't have this sheet attached to the window, since it would
come down from the titlebar, covering up the toolbar itself.  Also, we are not
using a system toolbar; rather, this toolbar is generated by Mozilla itself, so
we can't make a sheet appear underneath the toolbar as is seen in Camino (it
does use a Mac OS X toolbar).  So we are left with using this custom sheet
implementation which is used by all the Firefox platforms.

(2) The dialog has a titlebar:  This one seems rather easy to fix.  The dialog
is created with the options "chrome,all,dependent", but then the javascript
calls HideWindowChrome.  So it seems silly to create the window with "all"
enabled, but then try to hide it all.  If I change the options to just be
"chrome,dependent", then I get no titlebar on Mac.  This caused no noticeable
changes on Linux Firefox.
The other issue here is that HideWindowChrome is not implemented on Mac.  But it
wouldn't help us with this issue, since there is no way to remove the titlebar
from an existing window.  The only way to get no titlebar is to destroy the
window and recreate it with a window class that has no titlebar.  Yeah, that
kinda sucks.

(3) The dialog is not positioned correctly:  This one I haven't been able to
figure out.  The dialog opens up in the upper left corner every time.  I see the
call to move it underneath the toolbar, but it doesn't seem to have any effect.
 Anyone have any idea why this isn't working?
So I found out why the customize toolbar dialog would always display in the top
left corner.  It's this code "RepositionWindow" code here:
http://lxr.mozilla.org/mozilla/source/widget/src/mac/nsMacWindow.cpp#1523
It was originally checked in under bug 75653 (the initial mach-o changes bug),
but there isn't much documentation as to why it is necessary.  Anyone know why
this is necessary?

I removed that code from my build, and didn't notice anything amiss.  The
customize toolbar dialog looks much better, but now it appears a little too low.
 In fact, it looks like it is off by the height of the menubar.
i would venture that it was to work around a bug that is no longer in OSX. I
remember there were lots of problems on 10.0 and early betas where if a window
started sized 1x1 (gecko loves to do that), it would crash or do other crazy
stuff. not sure how relevant it still is.
Created attachment 157729 [details] [diff] [review]
browser.js patch

Remove "all" attribute from openDialog line.  It seems to me that there is no
reason to define all if we are then calling hideWindowChrome.  I've only tested
this in Mac and Linux.
Created attachment 157732 [details] [diff] [review]
OSX patch

This patch gets rid of the RepositionWindow code, so now the dialog doesn't
move to the upper left.  I also implemented HideWindowChrome, although as I've
mentioned before, on Mac there is no way to remove/hide the titlebar without
destroying and recreating the native window.

There's still the issue of the dialog showing up a little too low, but these
two patchs already make the situation much better.
Comment on attachment 157732 [details] [diff] [review]
OSX patch

Notice that we should check it in together with the patch for bug 237136, which
fixed the window movement issues.

BTW, we already have some windows without a title bar.
QA Contact: bugzilla → bugs.mano

Updated

14 years ago
Attachment #157732 - Flags: review?(peterv)
Comment on attachment 157732 [details] [diff] [review]
OSX patch

We ought to find a way to position the sheet under the toolbar.
Attachment #157732 - Flags: review?(peterv) → review+

Updated

14 years ago
Attachment #157729 - Flags: review?(mconnor)

Updated

14 years ago
Attachment #157732 - Flags: superreview?(sfraser)
Comment on attachment 157732 [details] [diff] [review]
OSX patch

Hmm, actually, do we want to set mSavedAttributes to a reasonable default
value? Maybe in StandardCreate? I'd rather not rely on HideWindowChrome being
called in the right order.
Created attachment 159765 [details] [diff] [review]
OSX patch v2

You're right.  I changed mSavedAttributes to be initially set to all bits on
(which seems to me that it would never happen).  Then I just check for it in
HideWindowChrome.
Attachment #157732 - Attachment is obsolete: true

Updated

14 years ago
Attachment #159765 - Flags: superreview?(sfraser)
Attachment #159765 - Flags: review?(peterv)

Updated

14 years ago
Attachment #157732 - Flags: superreview?(sfraser)
Attachment #157732 - Flags: review+
Comment on attachment 159765 [details] [diff] [review]
OSX patch v2

>Index: nsMacWindow.cpp
>===================================================================

>+  static const WindowAttributes kWindowChrome =
>+            kWindowStandardDocumentAttributes | kWindowToolbarButtonAttribute;
>+
>+  if (aShouldHide) {
>+    ::GetWindowAttributes(mWindowPtr, &mSavedAttributes);
>+    ::ChangeWindowAttributes(mWindowPtr, kWindowNoAttributes, kWindowChrome);
>+  } else {
>+    if (mSavedAttributes == 0xffffffffUL) {
>+      NS_WARNING("Calling HideWindowChrome(FALSE) without having first called HideWindowChrome(TRUE)");
>+    } else {
>+      ::ChangeWindowAttributes(mWindowPtr, mSavedAttributes, kWindowNoAttributes);

I would make this

      ::ChangeWindowAttributes(mWindowPtr, mSavedAttributes & kWindowChrome,
			       kWindowNoAttributes);
Attachment #159765 - Flags: review?(peterv) → review+
Don't know why but i see this fixed in Mozilla/5.0 (Macintosh; U; PPC Mac OS X
Mach-O; en-US; rv:1.7.3) Gecko/20040930 Firefox/0.10

Am i drunk!? :-)
Comment on attachment 157729 [details] [diff] [review]
browser.js patch

Don't really need this patch anymore, since the customize toolbar dialog no
longer slides out.  Now, it looks just fine with a titlebar.  Plus, in later
builds, I noticed that removing "all" from the attributes caused many of the
toolbar items to not display in the dialog for some reason.
Attachment #157729 - Attachment is obsolete: true
Attachment #157729 - Flags: review?(mconnor)
sfraser: If you've got the time, can you take a look at the latest patch?  I'd
like to close this bug out for 1.0.

Comment 28

14 years ago
My 2 cents what needs to be done before this can be considered fixed:

• The dialog needs to look like a sheet. Ie. no title bar (and ideally
semi-transparent background)
• The dialog needs to be fixed in x, y, and z to the parent window. Use a
WindowGroup for this.
• The window needs to slide out of the toolbar. Probably TransitionWindow...()
can be used for this. Either with the genie transition effect, or with the sheet
transition effect on a transparent dummy parent window that is placed at the
bottom of the toolbar.
Manfred: Those are interesting ideas, but it would require some non-trivial
changes to cross-platform code.  For one, we would need some way to know that
this dialog is attached to a toolbar, such as passing "modal-toolbar" as one of
the window attributes to |openDialog()|.  And then every platform but MacOSX
would ignore it.  Although I don't know how willing the Firefox devs would be to
such a change.

Comment 30

14 years ago
I guess that if it is too much of an effort to make a proper customize window,
we should at least remove the minimize button and all the other disabled buttons
from the title bar. Basically it means changing the class of the window to
something like movable modal or movable alert. However, I believe that in the
long run it would be better to make the customize window look like the
preferences (which by the way should not be a sheet) window. The only problem is
that it should be "attached" to the main window below the toolbar and not just
below the title bar. Moreover in whatever situation both the customize window
and the preferences window should not have the resize box in the left bottom
part. On top of that is the fact that you can just click the main window again,
leaving the customize window open, but then no menus work. If an inexperienced
user does something like this he will certainly be confused. So basically I
would opt for the customize window being like preferences window but attached
just below the toolbar.

Comment 31

14 years ago
moving blocking1.0mac bugs to Firefox1.1 Target Milestone.
Target Milestone: --- → Firefox1.1

Updated

14 years ago
Blocks: 73812

Comment 32

14 years ago
Is this patch still useful?
(In reply to comment #32)
> Is this patch still useful?

We will probably resotre the xul-sheet code for mac only, so the answer is
probably "yes".

Updated

14 years ago
Blocks: 277000
Assignee: bugs → nobody
Flags: blocking-aviary1.0mac+
QA Contact: bugs.mano → toolbars
Target Milestone: Firefox1.1 → ---

Comment 34

13 years ago
Comment on attachment 159765 [details] [diff] [review]
OSX patch v2

Looks OK, but why the removal of the RepositionWindow() part? Is it no longer
needed for 10.2?
|RepositionWindow()| was causing issues (don't remember them now), but it was
removed in a separate patch.  This patch is old and hasn't been updated to the
latest code.
(In reply to comment #33)
> We will probably resotre the xul-sheet code for mac only, so the answer is
> probably "yes".

Do you plan to do this for 1.5?  If so, is there a bug open for that?
Mano, is this still take-able for Fx2?
Keywords: polish
This is a very confusing behaviour, and I wish I'd noticed and reported it sooner.  If you have customize toolbar up and click on the window -- such as on the toolbar, without dragging! -- you bring the window to the fore.  At that point the window is _kinda_ functional, but the toolbar isn't, and has no visual indication that it's in this edit state.  You have to look for and find the customize-toolbar dialog off in the background (poor people without Expose!), and then put two and two together.

Nominating -- it's a pretty bad scene, and I think that the changes to integration the go and search buttons into their respective widgets will have a lot of existing users reaching into the customize dialog for the first time in a long time.
Flags: blocking-firefox2?
Not the current patch, no. But I'm going to look at this and see if there's anything we can do to improve the current UE.
Assignee: nobody → bugs.mano
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2

Comment 41

12 years ago
With cocoa widgets, toolbars and their customization sheets could use the Cocoa APIs and therefore not suck.

Comment 42

12 years ago
Comment on attachment 159765 [details] [diff] [review]
OSX patch v2

Mark should sr this.
Attachment #159765 - Flags: superreview?(sfraser_bugs) → superreview?(mark)
Comment on attachment 159765 [details] [diff] [review]
OSX patch v2

FWIW, I don't need this change for the Fx2 implementation (i.e. we're not going to make it slide out).

Comment 44

12 years ago
Comment on attachment 159765 [details] [diff] [review]
OSX patch v2

So you probably don't need this patch now according to Mano's comments, but I'll give my comments anyway just in case.

>Index: nsMacWindow.cpp

>+  , mSavedAttributes(0xffffffffUL)

I'd prefer -1 - what if the type becomes 64-bit?  But this seems kind of dirty, because you use this value to track unsaved attributes.  What if all of the attributes really are set?  OK, it's unlikely today, but don't be afraid to add another PRPackedBool.

>+NS_IMETHODIMP
>+nsMacWindow::HideWindowChrome(PRBool aShouldHide)
>+{
>+  // On MacOSX, we can disable most of the window chrome, except for the
>+  // titlebar.  The only way to get rid of the titlebar is to destroy the
>+  // window and recreate it as a window class that lacks a titlebar.

On Tiger, you can actually set kWindowNoTitleBarAttribute.  Don't know if it's worth diverging.

>+  static const WindowAttributes kWindowChrome =
>+            kWindowStandardDocumentAttributes | kWindowToolbarButtonAttribute;
>+
>+  if (aShouldHide) {
>+    ::GetWindowAttributes(mWindowPtr, &mSavedAttributes);

If you call HideWindowChrome(true) twice successively, you'll obliterate mSavedAttributes.  Since there's no example consumer of this API, I don't know if that would ever happen, but Gecko in general likes to do things like show windows already being shown, and resize windows without changing their bounds.  If you had mChromeHidden, you could compare it to aShouldHide and wrap the meat of this method in that comparison, like we do in Show and Resize.

>+    ::ChangeWindowAttributes(mWindowPtr, kWindowNoAttributes, kWindowChrome);
>+  } else {
>+    if (mSavedAttributes == 0xffffffffUL) {
>+      NS_WARNING("Calling HideWindowChrome(FALSE) without having first called HideWindowChrome(TRUE)");
>+    } else {
>+      ::ChangeWindowAttributes(mWindowPtr, mSavedAttributes, kWindowNoAttributes);

peterv's got a good suggestion in comment 24.

>+      mSavedAttributes = 0xffffffffUL;
>+    }
>+  }

>Index: nsMacWindow.h

> 	PRPackedBool                    mIsSheet;        // true if the window is a sheet (Mac OS X)
> 	PRPackedBool                    mIgnoreDeactivate;  // true if this window has a (Mac OS X) sheet opening
> 	PRPackedBool                    mAcceptsActivation;
> 	PRPackedBool                    mIsActive;
> 	PRPackedBool                    mZoomOnShow;
> 	PRPackedBool                    mZooming;
> 	PRPackedBool                    mResizeIsFromUs;    // we originated the resize, prevent infinite recursion
> 	Point                           mBoundsOffset;      // offset from window structure to content
> 	auto_ptr<nsMacEventHandler>     mMacEventHandler;
> 	nsIWidget                      *mOffsetParent;
>+	WindowAttributes                mSavedAttributes;

This class is already a mess, but the wider types like WindowAttributes should come before narrower ones like PRPackedBool.
Attachment #159765 - Flags: superreview?(mark) → superreview-
Should we split off the Fx2 fix into a new bug and not hijack this?
Actual work (for FF2.0) TBD on bug 350129.
Whiteboard: see bug 350129
Minusing for Fx2 per comment 46. Do we want to nom this for 1.9?
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: Firefox 2 → ---

Updated

11 years ago
Flags: blocking-firefox3?
Colin, can you evaluate this patch if you get a second?
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: see bug 350129 → [wanted-firefox3] see bug 350129
The only non-obsolete patch on here is against the old Carbon widgets code. Cocoa widgets have extensive support for native sheets already, so if you wanted to resolve this bug it'd be in JS somewhere. I don't know the JS API for opening windows and such, but I assume you pass some flag to indicate you want a child window to open as a sheet (Mano would know more?).

Hope that helps!

(In reply to comment #49)
> The only non-obsolete patch on here is against the old Carbon widgets code.
> Cocoa widgets have extensive support for native sheets already, so if you
> wanted to resolve this bug it'd be in JS somewhere. I don't know the JS API for
> opening windows and such, but I assume you pass some flag to indicate you want
> a child window to open as a sheet (Mano would know more?).
> 
> Hope that helps!
> 

Huh, not that easy. the toolbar customization sheet should be docked to the window's toolbar, no to its titlebar (also, the opener should still accept d&d events, etc). On trunk, we emulate this using a panel element. We don't emulate the animation though, and _you_ know why ;)
(In reply to comment #50) 
> Huh, not that easy. the toolbar customization sheet should be docked to the
> window's toolbar, no to its titlebar (also, the opener should still accept d&d
> events, etc).

Ah you're right, my bad :)

> On trunk, we emulate this using a panel element. We don't emulate
> the animation though, and _you_ know why ;)

:(

There are a bunch of simple changes that would make it be closer the ones in native apps. The text should read "Drag your favorite item into the toolbar…" "…or drag the default set", for example (I thought I filed a bug about this but I'm unable to find it).

Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3] see bug 350129 → see bug 350129

Comment 52

10 years ago
This bug no longer exists in Firefox 3 and should probably be closed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 53

10 years ago
Currently, the dialog is implemented as a popup, not a sheet. A sheet would slide in, which is what the original reporter requested, and maybe make keyboard-accessibility more straightforward.

The sheet could be positioned below the toolbars using the -window:willPositionSheet:usingRect: method in NSWindow. FrameIsInActiveWindow() in FrameIsInActiveWindow.mm would have to also special-case the Customize Toolbar dialog specifically.

Comment 54

10 years ago
While you're free to file a new bug for the specifics, I think the general intent of this bug has been provided. When this bug was filed (almost 4 years ago), the toolbar customization appeared in a new, separate window (see attached screenshot). Now, the window does appear to be a sheet, although it might not perfectly mimic an OS X native (Cocoa) sheet.

Regardless, if you'd like to file a bug about the implementation method, the lack of animation, that it should appear in a different location, or be implemented as a native OS X sheet, you're welcome to do so. Naturally, please make sure these have not already been filed before doing so. This bug is simply too general and not the right place for these items.
You need to log in before you can comment on or make changes to this bug.