206649 - customize toolbar dialog should be a "sheet"

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Paul Baker]

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.

Comment 1

[Paul Baker]

Attachment: snapshot of firebird incorrectly displaying customize toolbar as a dialog

Comment 2

[Paul Baker]

Attachment: snapshot of camino correctly displaying customize toolbar as a sheet

Comment 3

[Greg K.]

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

Comment 4

[Greg K.]

Really marking NEW.

Comment 5

[Chris Neale]

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

Comment 6

[Simon Paquet [:sipaq]]

Taking QA Contact, sorry for bugspam

Comment 7

[Sam Katz]

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

Comment 8

[Wayne Woods]

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).

Comment 9

[Paul Baker]

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

[Wayne Woods]

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

[scratch]

it's already a sheet on Windows, so it's weird (and backwards) that it's not on Mac.

Comment 12

[Asaf Romano (gone)]

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...

Comment 13

[jhp (no longer active)]

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.

Comment 14

[Asaf Romano (gone)]

Moving blocking requsetto 1.0mac, since it's the right place for mac UI issues.

Comment 15

[jhp (no longer active)]

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?

Comment 16

[jhp (no longer active)]

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.

Comment 17

[Mike Pinkerton (not reading bugmail)]

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.

Comment 18

[jhp (no longer active)]

Attachment: 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.

Comment 19

[jhp (no longer active)]

Attachment: 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 20

[Asaf Romano (gone)]

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.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 157732 [details] [diff] [review]
OSX patch

We ought to find a way to position the sheet under the toolbar.

Comment 22

[Peter Van der Beken [:peterv]]

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.

Comment 23

[jhp (no longer active)]

Attachment: 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.

Comment 24

[Peter Van der Beken [:peterv]]

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);

Comment 25

[Asaf Romano (gone)]

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 26

[jhp (no longer active)]

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.

Comment 27

[jhp (no longer active)]

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

[Manfred Schubert]

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.

Comment 29

[jhp (no longer active)]

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

[Ignacy Niemczycki]

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

[Asa Dotzler [:asa]]

moving blocking1.0mac bugs to Firefox1.1 Target Milestone.

Comment 32

[Simon Fraser [no longer active]]

Is this patch still useful?

Comment 33

[Asaf Romano (gone)]

(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".

Comment 34

[Simon Fraser [no longer active]]

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?

Comment 35

[jhp (no longer active)]

|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.

Comment 36

[jhp (no longer active)]

(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?

Comment 37

[Asaf Romano (gone)]

Not for 1.5.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Mano, is this still take-able for Fx2?

Comment 39

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 40

[Asaf Romano (gone)]

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.

Comment 41

[Simon Fraser [no longer active]]

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

Comment 42

[Simon Fraser [no longer active]]

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

Mark should sr this.

Comment 43

[Asaf Romano (gone)]

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

[Mark Mentovai]

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.

Comment 45

[Mike Connor [:mconnor]]

Should we split off the Fx2 fix into a new bug and not hijack this?

Comment 46

[Asaf Romano (gone)]

Actual work (for FF2.0) TBD on bug 350129.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Minusing for Fx2 per comment 46. Do we want to nom this for 1.9?

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Colin, can you evaluate this patch if you get a second?

Comment 49

[Colin Barrett [:cbarrett]]

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!

Comment 50

[Asaf Romano (gone)]

(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 ;)

Comment 51

[Colin Barrett [:cbarrett]]

(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).

Comment 52

[Jonathan Dance]

This bug no longer exists in Firefox 3 and should probably be closed.

Comment 53

[Minh Nguyễn]

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

[Jonathan Dance]

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.