If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: Daryle Walker, Assigned: mstange)

Tracking

Trunk
mozilla1.9.2a1
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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
Component: Toolbars → XP Toolkit/Widgets
Product: Firefox → Browser
Whiteboard: DUPEME
Version: unspecified → Trunk
Assignee: bugs → jag
QA Contact: bugzilla → jrgmorrison
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/
Assignee: jag → joshmoz
Status: UNCONFIRMED → NEW
Component: XP Toolkit/Widgets → Widget: Mac
Ever confirmed: true
QA Contact: jrgmorrison → mac
(Reporter)

Comment 2

12 years ago
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".
(Reporter)

Comment 3

12 years ago
The bug still exists in Mozilla 1.5b2.

Updated

11 years ago
Blocks: 274273

Updated

11 years ago
Blocks: 328165

Updated

10 years ago
Assignee: joshmoz → nobody
Assignee: nobody → joshmoz
Component: Widget: Mac → Widget: Cocoa
QA Contact: mac → cocoa
Whiteboard: DUPEME

Comment 4

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

Updated

9 years ago
Blocks: 378951
(Assignee)

Updated

9 years ago
Duplicate of this bug: 489690

Updated

9 years ago
Assignee: joshmoz → nobody
(Assignee)

Updated

8 years ago
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Hardware: PowerPC → All
Summary: window types without toolbars still have control buttons → Add an attribute to control the presence of the toolbar collapse pill button
(Assignee)

Comment 6

8 years ago
Created attachment 391792 [details] [diff] [review]
add "showstoolbarbutton" attribute, defaulting to off
Attachment #391792 - Flags: superreview?(roc)
Attachment #391792 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Attachment #391792 - Flags: review?(joshmoz)
(Assignee)

Comment 7

8 years ago
Created attachment 391794 [details] [diff] [review]
v2, add an NS_ENSURE_TRUE
Attachment #391792 - Attachment is obsolete: true
Attachment #391794 - Flags: superreview?(roc)
Attachment #391794 - Flags: review?(neil)
Attachment #391792 - Flags: superreview?(roc)
Attachment #391792 - Flags: review?(neil)
Attachment #391792 - Flags: review?(joshmoz)
(Assignee)

Updated

8 years ago
Attachment #391794 - Flags: review?(joshmoz)
(Assignee)

Comment 8

8 years ago
Created attachment 391799 [details] [diff] [review]
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?
Attachment #391799 - Flags: ui-review?(faaborg)
Attachment #391799 - Flags: review?(enndeakin)

Comment 9

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

Comment 10

8 years ago
The buttons in the Error Console also feel like tabs to me (except for the Clear button).
Does anyone know what the current rule is for showing this button? I'm guessing it depends on the window features somehow.
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

8 years ago
OK, xul.css supports prefwindow, too.

Updated

8 years ago
Attachment #391794 - Flags: review?(neil) → review+
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!
(Assignee)

Comment 17

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

Comment 18

8 years ago
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.
Attachment #391799 - Flags: review?(enndeakin)

Comment 19

8 years ago
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.
Attachment #391794 - Flags: review?(joshmoz)
(Assignee)

Comment 20

8 years ago
Created attachment 392390 [details] [diff] [review]
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.
Attachment #391794 - Attachment is obsolete: true
Attachment #392390 - Flags: superreview?(roc)
Attachment #392390 - Flags: review?(joshmoz)
Attachment #391794 - Flags: superreview?(roc)
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.
Attachment #392390 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

8 years ago
Blocks: 508180
(Assignee)

Comment 22

8 years ago
Follow-up consolidation patch is in bug 508180.

Updated

8 years ago
Attachment #392390 - Flags: review?(joshmoz) → review+
Attachment #391799 - Flags: ui-review?(faaborg) → ui-review+
(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?
(Assignee)

Comment 24

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

Comment 25

8 years ago
Created attachment 392666 [details] [diff] [review]
part 1, v3, updated to trunk
Attachment #392390 - Attachment is obsolete: true
(Assignee)

Comment 26

8 years ago
Created attachment 392668 [details] [diff] [review]
part 2, with "toggletoolbar"
Attachment #391799 - Attachment is obsolete: true
Attachment #392668 - Flags: review?(neil)

Updated

8 years ago
Attachment #392668 - Flags: review?(neil)
Comment on attachment 392668 [details] [diff] [review]
part 2, with "toggletoolbar"

I'm not a browser peer.
(Assignee)

Updated

8 years ago
Attachment #392668 - Flags: review?(enndeakin)

Comment 28

8 years ago
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?
Attachment #392668 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 29

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c7a96c0ced36
http://hg.mozilla.org/mozilla-central/rev/33a666c42ee3
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.