269410 - Add an attribute to control the presence of the toolbar collapse pill button

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Daryle Walker]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The elongated button in the right side of the window title bar is supposed to
control the window's toolbar.  However, I've seen that button on window types
that will _never_ (AFAIK) have toolbars.  So the toolbar button should be
removed for those window types.  The types I seen are the view-source and
extensions windows.


Reproducible: Always
Steps to Reproduce:
1.Open an appropriate window using the "Page Source" or "Extensions" menu items
2.Observe that the resulting windows have a non-functional toolbar window
3.



Expected Results:  
Windows that don't ever have toolbars shouldn't have toolbar control buttons

Comment 1

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 2

[Daryle Walker]

The bug still exists in Mozilla 1.0.7 and 1.5b1.  In the latter version, the credits/about box is now a 
separate window, and it now has a useless toolbar control button.  I think you need to change your base 
window classes to add the toolbar control button if and only if a window has a toolbar associated with it 
(even if said toolbar is currently hidden) and omit the button if the window could _never_ have a toolbar.

BTW, step 2 for reproduction should have "toolbar window" replaced "toolbar control button".

Comment 3

[Daryle Walker]

The bug still exists in Mozilla 1.5b2.

Comment 4

[Minh Nguyễn]

XUL <wizard>s don't seem to have the toolbar control button, but all the other window types do.

Comment 6

[Markus Stange [:mstange]]

Attachment: add "showstoolbarbutton" attribute, defaulting to off

Comment 7

[Markus Stange [:mstange]]

Attachment: v2, add an NS_ENSURE_TRUE

Comment 8

[Markus Stange [:mstange]]

Attachment: restore the button for the browser and places window

The only window where giving the option to collapse the toolbar makes sense to me is the main browser window.
This patch also displays the button for the Places window because in my opinion that window's toolbar comes closest to a "real" toolbar (where by "real" I mean toolbars that can be customized and where icon size and text display can be changed).
Alex, do you think that's enough?

Comment 9

[Minh Nguyễn]

How about the Error Console? That's the only other window I know of where there's toolbars as opposed to (essentially) tabs.

Comment 10

[Markus Stange [:mstange]]

The buttons in the Error Console also feel like tabs to me (except for the Clear button).

Comment 11

[[e-mail]]

Does anyone know what the current rule is for showing this button? I'm guessing it depends on the window features somehow.

Comment 12

[Markus Stange [:mstange]]

It depends on whether nsWindowWatcher::CalculateChromeFlags includes nsIWebBrowserChrome::CHROME_OPENAS_DIALOG. If it's a dialog, it won't have a toolbar button, otherwise it will.

Comment 13

[Markus Stange [:mstange]]

(Because CHROME_OPENAS_DIALOG will translate to eWindowType_dialog and we only create windows with the ToolbarWindow class for the toplevel window type.)

Comment 14

[Markus Stange [:mstange]]

Hmm, I wonder whether we should ignore the showstoolbarbutton attribute on non-<window> window elements, since xul.css only supperts the chromehidden attribute for <window>s.

Comment 15

[Markus Stange [:mstange]]

OK, xul.css supports prefwindow, too.

Comment 16

[[e-mail]]

Comment on attachment 391794 [details] [diff] [review]
v2, add an NS_ENSURE_TRUE

I would say that other windows such as Page Info exist that have deliberately added chromeclass-toolbar to their elements so that the pill takes effect. I don't know of a good way to automatically detect this though.

Note: On Windows the dialog style is used to turn off the system menu without turning off the close button, and is only suitable for dependent (or modal) windows, so I see we can't ask people to add it just to turn off the pill.

>+  nsresult rv = windowElement->GetAttribute(NS_LITERAL_STRING("showstoolbarbutton"), attr);
That's a bit of a mouthful. How about "toggletoolbar"?

>+    mWindow->SetShowsToolbarButton(attr.LowerCaseEqualsLiteral("true"));
Interesting. I see where you copied this from, but normally XUL is sensitive!

Comment 17

[Markus Stange [:mstange]]

(In reply to comment #16)
> (From update of attachment 391794 [details] [diff] [review])
> I would say that other windows such as Page Info exist that have deliberately
> added chromeclass-toolbar to their elements so that the pill takes effect.

Ah. Do you think I should remove chromeclass-toolbar from the toolbars in those windows where I kill the button?

> >+  nsresult rv = windowElement->GetAttribute(NS_LITERAL_STRING("showstoolbarbutton"), attr);
> That's a bit of a mouthful. How about "toggletoolbar"?

"toggletoolbar" is great. I'll use that.

> >+    mWindow->SetShowsToolbarButton(attr.LowerCaseEqualsLiteral("true"));
> Interesting. I see where you copied this from, but normally XUL is sensitive!

Heh. The other place I looked at was the handling of the "noautohide" attribute in nsMenuPopupFrame.cpp, which is also insensitive.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 391799 [details] [diff] [review]
restore the button for the browser and places window

I'll update this patch with the new attribute name when the rest has been reviewed.

Comment 19

[Josh Aas]

Comment on attachment 391794 [details] [diff] [review]
v2, add an NS_ENSURE_TRUE

>+++ b/widget/public/nsIWidget.h

Bump iid since you're changing the interface.

>+NS_IMETHODIMP nsXULWindow::SyncAttributesToWidget()
>+{
>+  nsCOMPtr<nsIDOMElement> windowElement;
>+  GetWindowDOMElement(getter_AddRefs(windowElement));
>+  NS_ENSURE_TRUE(windowElement, NS_ERROR_FAILURE);
>+  nsAutoString attr;
>+  nsresult rv = windowElement->GetAttribute(NS_LITERAL_STRING("showstoolbarbutton"), attr);
>+  if (NS_SUCCEEDED(rv)) {
>+    mWindow->SetShowsToolbarButton(attr.LowerCaseEqualsLiteral("true"));
>+  }
>+}
>+

This method name for updating the widget after XUL attributes load is generic. This is probably not the only bit of updating we do after loading XUL attributes (size? icons?), we should consolidate the updates that need to happen in one method. That means moving the other updates to this generic method, or for now at least making this method name specific to the toolbar button. If you only rename this method, please file a bug on consolidating updates.

I'll r+ after another patch with all of the suggested changes so far.

Comment 20

[Markus Stange [:mstange]]

Attachment: part 1, v3

I've bumped the IID and renamed the method to LoadToolbarButtonPresenceFromXUL. I'll consolidate the methods in a follow-up bug real soon.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 392390 [details] [diff] [review]
part 1, v3

I agree that XUL attribute checks should be case sensitive. r+ with a case sensitive check.

Comment 22

[Markus Stange [:mstange]]

Follow-up consolidation patch is in bug 508180.

Comment 23

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 391794 [details] [diff] [review])
> > I would say that other windows such as Page Info exist that have deliberately
> > added chromeclass-toolbar to their elements so that the pill takes effect.
> Ah. Do you think I should remove chromeclass-toolbar from the toolbars in those
> windows where I kill the button?
I think you need to research why they added the chromeclass-toolbar - maybe it was intentional?

Comment 24

[Markus Stange [:mstange]]

I did some research, it was very enlightening.
Up to now I thought that these attributes were added in order not to have a useless button sitting in the titlebar. However, then I had a close look at what else can add chromehidden="toolbar" to the window, and found out about the "toolbar" window flag, which is specified when opening the window.

http://mxr.mozilla.org/mozilla-central/search?string=%2Ctoolbar
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#2106

So whenever you open a window that uses a toolbar with class "chromeclass-toolbar", you need to set the toolbar flag, otherwise the toolbar will be hidden.

So I guess I shouldn't be removing chromeclass-toolbar, since then you can no longer open those windows with the toolbar hidden by default. I don't know who'd do that, though...

Comment 25

[Markus Stange [:mstange]]

Attachment: part 1, v3, updated to trunk

Comment 26

[Markus Stange [:mstange]]

Attachment: part 2, with "toggletoolbar"

Comment 27

[[e-mail]]

Comment on attachment 392668 [details] [diff] [review]
part 2, with "toggletoolbar"

I'm not a browser peer.

Comment 28

[Neil Deakin]

Comment on attachment 392668 [details] [diff] [review]
part 2, with "toggletoolbar"

Looks ok but shouldn't the default value be determined automatically based on whether a toolbar is present or not?

Comment 29

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/c7a96c0ced36
http://hg.mozilla.org/mozilla-central/rev/33a666c42ee3

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20090930 Namoroka/3.6b1pre ID:20090930033826