Last Comment Bug 206544 - Include "Full Screen" button in toolbar customize menu
: Include "Full Screen" button in toolbar customize menu
Status: VERIFIED FIXED
: user-doc-complete, verified1.9.2
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: Trunk
: All All
: -- enhancement with 14 votes (vote)
: Firefox 3.6a1
Assigned To: Steffen Wilberg
:
: :Gijs Kruitbosch
Mentors:
: 318930 425670 (view as bug list)
Depends on:
Blocks: 425582 505699 507861
  Show dependency treegraph
 
Reported: 2003-05-20 22:22 PDT by Kevin Ar18
Modified: 2009-11-20 12:22 PST (History)
20 users (show)
mconnor: blocking‑aviary1.5-
mbeltzner: blocking‑firefox3-
reed: wanted‑firefox3+
benjamin: blocking1.9a1-
mconnor: blocking‑firefox2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Suggested patch (3.40 KB, patch)
2006-06-02 19:45 PDT, Georges-Etienne Legendre
no flags Details | Diff | Splinter Review
Suggested patch with "checkbox" style button (10.71 KB, patch)
2006-06-03 04:35 PDT, Georges-Etienne Legendre
no flags Details | Diff | Splinter Review
unbitrotted and fixed patch (10.53 KB, patch)
2009-07-26 09:10 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
correct patch (10.76 KB, patch)
2009-07-26 09:12 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
keeping "View:FullScreen" (9.78 KB, patch)
2009-07-29 06:35 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
keeping the command (9.14 KB, patch)
2009-07-29 13:31 PDT, Steffen Wilberg
dao+bmo: review-
Details | Diff | Splinter Review
fixed review comments (8.16 KB, patch)
2009-07-30 12:30 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
fixed further comments (6.87 KB, patch)
2009-07-30 13:36 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
with nicer tooltip (7.84 KB, patch)
2009-07-30 15:28 PDT, Steffen Wilberg
dao+bmo: review+
jboriss: ui‑review+
Details | Diff | Splinter Review
with comment 41 addressed (7.88 KB, patch)
2009-07-31 08:59 PDT, Steffen Wilberg
steffen.wilberg: review+
steffen.wilberg: ui‑review+
Details | Diff | Splinter Review

Description Kevin Ar18 2003-05-20 22:22:57 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

Would be nice to have the "Full Screen" option from the View menu also available
on the toolbar (through Customize...?)

Reproducible: Always

Steps to Reproduce:
.
Actual Results:  
.

Expected Results:  
.

.
Comment 1 Kevin Ar18 2003-05-20 22:23:33 PDT
OS -> All
Comment 2 David P James 2003-06-05 18:20:09 PDT
Have you considered using the following extension? It adds an item to the
context menu to go to/from Full Screen.

http://texturizer.net/firebird/extensions.html#Autohide%20Toolbox%20%28True%20Fullscreen%29
Comment 3 Kevin Ar18 2003-06-09 12:32:00 PDT
Thanks, I just tested the extension....
I already had the Full Screen option in the context menu, so that wasn't an
issue in my request.

What I am proposing is making some more icons available for placing in the
toolbar like the fullscreen option.
Comment 4 Mike Connor [:mconnor] 2003-06-10 10:05:48 PDT
This is something that would belong more in the linked extension, but the
request seems clear enough.  Confirming and recommending WONTFIX.
Comment 5 Kevin Ar18 2003-06-11 21:08:13 PDT
Should this be relegated to an extension?  Would it not be prudent to include a
full range of options (buttons) for the user to add to their toolbar?  I was
considering suggesting other icons, that could be available in the cutomize
section.  Then again, maybe even have them as a default.  It might be much more
helpful helpful that way.
Comment 6 Simon Paquet [:sipaq] 2003-07-28 05:20:09 PDT
Taking QA Contact
Comment 7 Stephan T. Lavavej 2004-04-18 05:44:51 PDT
I was just about to submit a duplicate of this, but wisely checked the 
existing bugs first.

As a user coming from IE this enhancement request is very important to me. If 
Firefox supports Full Screen, it should have a Full Screen button.
Comment 8 u88484 2004-08-25 12:39:03 PDT
ya firefox definently needs a full screen icon button
Comment 9 Tom Londer 2004-11-13 07:11:58 PST
Hi Firefox people,
I want to add an icon to the toolbar that allows Full Screen.  I know about the
View Menu and F11. I've searched through tons of FF and Mozilla pages looking
for this.  Please give me a heads up.  I may not be able to figure out how to
see this question as bug report (and it really isn't a bug) - it's just a handy
feature.
PLEASE, if someone has a solution or can "point me" to the right place to learn
how to do this PLEASE email me direct at tlonder@comcast.net.  Will be eternally
grateful (and maybe I can do something FOR YOU!
Thanks,
Tom Londer
Comment 10 Tom Londer 2004-11-14 07:23:35 PST
Bugzilla Bug 206544

full screen toolbar item

Here’s the solution for the above along with a  lot of other useful tools!:

<"http://www.w3.org/TR/html4/strict.dtd">

It’s called toolbar enhancements!  
Comment 12 Dan Fischbach 2004-11-21 18:52:50 PST
I agree, I only get the Toolbar Enhancements extension just for that button,
which sems silly.  Voted.
Comment 13 Dan Fischbach 2005-05-24 13:30:43 PDT
Can this be nominated for 1.1?  IE has this...why doesn't Firefox have this by
default?  It seems silly to get the extension and bloat Firefox just for this
one button.
Comment 14 Mike Connor [:mconnor] 2005-07-03 17:12:06 PDT
Not a blocker, will consider a reasonable patch.
Comment 15 Dave Townsend [:mossop] 2005-12-03 09:59:39 PST
*** Bug 318930 has been marked as a duplicate of this bug. ***
Comment 16 Georges-Etienne Legendre 2006-06-02 19:45:00 PDT
Created attachment 224285 [details] [diff] [review]
Suggested patch

Here is a suggested patch. Of course, a new icon will be required. This patch currently uses the "new-tab" icon.
Comment 17 Georges-Etienne Legendre 2006-06-03 04:35:59 PDT
Created attachment 224301 [details] [diff] [review]
Suggested patch with "checkbox" style button
Comment 18 Mike Connor [:mconnor] 2006-06-04 14:17:08 PDT
not a blocker, will consider patch separately.
Comment 19 Georges-Etienne Legendre 2006-06-06 18:07:02 PDT
Who is able to review this patch? What's the next step?
Comment 20 石庭豐 (Seak, Teng-Fong) 2007-01-28 07:14:21 PST
Personally, I use this extension (https://addons.mozilla.org/firefox/810/) to have the icon available for customization!

Not only is it quite stupid to install an extension/addon just to have an icon, it's also very tedious for administrators.

I'm in charge of installing O/S to computers for my company (amongst misc many other tasks).  Every time I install a new machine, I'll install Firefox.  Then I have to install the extensions: there're about 7 or 8 of them.  It would be nice if I could install one less.  Well, one extension less is one extension less.  If you have to install, say, one computer everyday, you would be delighted that there's a little bit of work to do!

And there were all those upgrades: from 1.0 to 1.5, then from 1.5 to 2.0.  And every upgrade just breaks this extension and I have to reinstall the extension over again for ALL the machines!  Can you imagine the workload!?

So, I also vote to include the icon in Firefox instead of having us install any extension.

I've changed Version value to 2.0 branch and hardware to ALL.  I know FF in some O/S doesn't work well with full screen mode, but it doesn't hurt to include the icon and hide it in O/S where full screen mode isn't available.
Comment 21 u88484 2007-09-01 11:08:43 PDT
Can we get this for Firefox 3? I know people are getting ready to work on the theme so this is idea time to add a new button and get it all in one shot!
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2007-09-13 07:47:10 PDT
Alex, can you add this to the icon inventory requirements?
Comment 23 u88484 2008-03-28 09:19:52 PDT
*** Bug 425670 has been marked as a duplicate of this bug. ***
Comment 24 Alex Faaborg [:faaborg] (Firefox UX) 2008-03-30 17:17:05 PDT
Note that we now have the icon in toolbar.png
Comment 25 u286998 2008-05-10 11:59:39 PDT
@Georges-Etienne Legendre: You should probably request a review for the patch, i guess Mike Conner is the person to ask here.
Comment 26 Steffen Wilberg 2009-07-26 09:10:03 PDT
Created attachment 390727 [details] [diff] [review]
unbitrotted and fixed patch

The previous patch was bitrotted in every single hunk.
This patch applies to mozilla-central. It uses the winstripe icons we have since Firefox 3.0, and the gtk stock icon for gnomestripe. It fixes a bug in the previous patch which showed a checked state of the button in normal mode instead of in fullscreen mode.

Tested on Windows, needs testing on Linux.
Comment 27 Steffen Wilberg 2009-07-26 09:12:07 PDT
Created attachment 390728 [details] [diff] [review]
correct patch

Sorry, this is the one.
Comment 28 Steffen Wilberg 2009-07-26 12:00:04 PDT
Tested on Linux as well.
Comment 29 Dão Gottwald [:dao] 2009-07-29 05:49:24 PDT
Can you do that without removing View:FullScreen?
Comment 30 Steffen Wilberg 2009-07-29 06:35:08 PDT
Created attachment 391323 [details] [diff] [review]
keeping "View:FullScreen"

Here you are.
Comment 31 Dão Gottwald [:dao] 2009-07-29 06:39:39 PDT
Sorry, I wasn't clear. I actually meant keeping the command, rather than calling the broadcaster View:FullScreen.
Comment 32 Steffen Wilberg 2009-07-29 13:31:20 PDT
Created attachment 391428 [details] [diff] [review]
keeping the command

I just learned that I can use a command like a broadcaster:
"Broadcasters aren't used frequently as commands can generally handle most uses. One thing to point out is that there really is no difference between the command  element and the broadcaster element. They both do the same thing. The difference is more semantic. Use commands for actions and use broadcasters for state. In fact, any element can act as broadcaster, as long as you observe it using the observes attribute."
https://developer.mozilla.org/en/XUL_Tutorial/Broadcasters_and_Observers

So I can keep the command, but use it like a broadcaster, which I need for the pressed state of the toolbar button (the checkbox of the menu item isn't visible in full screen mode since you can't access the menu bar).

Are you worrying about extension compat?
Comment 33 Dão Gottwald [:dao] 2009-07-29 14:05:40 PDT
Comment on attachment 391428 [details] [diff] [review]
keeping the command

>+    <command id="View:FullScreen"
>+             type="checkbox" autoCheck="false"
>+             label="&fullScreenCmd.label;" 
>+             oncommand="BrowserFullScreen();"/>

type, autoCheck and label should be set on the appropriate elements directly rather than on the command element.

>-    <key id="key_fullScreen" keycode="VK_F11" command="View:FullScreen"/>
>+    <key id="key_fullScreen" keycode="VK_F11" observes="View:FullScreen"/>

That change doesn't make sense.

> function BrowserFullScreen()
> {
>   window.fullScreen = !window.fullScreen;
>+  let fullscreenBroadcaster = document.getElementById("View:FullScreen");
>+  fullscreenBroadcaster.setAttribute("checked", window.fullScreen);
> }

This needs to be added in the onFullScreen function.

>+#fullscreen-button {
>+  -moz-image-region: rect(0px 360px 24px 336px);
>+}
>+#fullscreen-button:not([disabled="true"]):hover {
>+  -moz-image-region: rect(24px 360px 48px 336px);
>+}
>+#fullscreen-button[disabled="true"] {
>+  -moz-image-region: rect(48px 360px 72px 336px);
>+}
>+#fullscreen-button:not([disabled="true"]):hover:active {
>+  -moz-image-region: rect(96px 360px 120px 336px);
>+}

Why should that toolbarbutton support the disabled state?
Comment 34 Steffen Wilberg 2009-07-30 12:30:47 PDT
Created attachment 391673 [details] [diff] [review]
fixed review comments

(In reply to comment #33)
> (From update of attachment 391428 [details] [diff] [review])
> >+    <command id="View:FullScreen"
> >+             type="checkbox" autoCheck="false"
> >+             label="&fullScreenCmd.label;" 
> >+             oncommand="BrowserFullScreen();"/>
> 
> type, autoCheck and label should be set on the appropriate elements directly
> rather than on the command element.
Fixed. Note that this duplicates type, autoCheck and label on the menuitem and toolbarbutton.

> >-    <key id="key_fullScreen" keycode="VK_F11" command="View:FullScreen"/>
> >+    <key id="key_fullScreen" keycode="VK_F11" observes="View:FullScreen"/>
> 
> That change doesn't make sense.
I dropped that.

> > function BrowserFullScreen()
> > {
> >   window.fullScreen = !window.fullScreen;
> >+  let fullscreenBroadcaster = document.getElementById("View:FullScreen");
> >+  fullscreenBroadcaster.setAttribute("checked", window.fullScreen);
> > }
> 
> This needs to be added in the onFullScreen function.
Yeah, but FullScreen.toggle(), which is called from onFullScreen(), is even better, because it already set the checked attribute for the menuitem.
Now it sets the attribute on the command, which is observed from both menuitem and toolbarbutton.

> >+#fullscreen-button {
> >+  -moz-image-region: rect(0px 360px 24px 336px);
> >+}
> >+#fullscreen-button:not([disabled="true"]):hover {
> >+  -moz-image-region: rect(24px 360px 48px 336px);
> >+}
> >+#fullscreen-button[disabled="true"] {
> >+  -moz-image-region: rect(48px 360px 72px 336px);
> >+}
> >+#fullscreen-button:not([disabled="true"]):hover:active {
> >+  -moz-image-region: rect(96px 360px 120px 336px);
> >+}
> 
> Why should that toolbarbutton support the disabled state?
Not sure. Why do the home, bookmarks, history, downloads, print, new tab, new window buttons support the disabled state?
Why do those buttons have disabled states in Toolbar.png?
Comment 35 Dão Gottwald [:dao] 2009-07-30 12:37:51 PDT
Comment on attachment 391673 [details] [diff] [review]
fixed review comments

>+#ifndef XP_MACOSX
>     <command id="View:FullScreen" oncommand="BrowserFullScreen();"/>
>+#endif

What's that change about?

(In reply to comment #34)
> Not sure. Why do the home, bookmarks, history, downloads, print, new tab, new
> window buttons support the disabled state?
> Why do those buttons have disabled states in Toolbar.png?

Because somebody thought they might be needed, even though they aren't.
Comment 36 Steffen Wilberg 2009-07-30 12:46:22 PDT
(In reply to comment #35)
> (From update of attachment 391673 [details] [diff] [review])
> >+#ifndef XP_MACOSX
> >     <command id="View:FullScreen" oncommand="BrowserFullScreen();"/>
> >+#endif
> 
> What's that change about?
The menuitem, key and the new toolbarbutton are ifndef XP_MACOSX, so why not the command? Nobody's using the command on Mac.

> (In reply to comment #34)
> > Not sure. Why do the home, bookmarks, history, downloads, print, new tab, new
> > window buttons support the disabled state?
> > Why do those buttons have disabled states in Toolbar.png?
> 
> Because somebody thought they might be needed, even though they aren't.
OK, I'll drop it.
Comment 37 Dão Gottwald [:dao] 2009-07-30 12:51:49 PDT
(In reply to comment #36)
> The menuitem, key and the new toolbarbutton are ifndef XP_MACOSX, so why not
> the command? Nobody's using the command on Mac.

Extensions could.
Comment 38 Steffen Wilberg 2009-07-30 13:36:20 PDT
Created attachment 391691 [details] [diff] [review]
fixed further comments

OK, I left the command alone and dropped the disabled state of the toolbarbutton.
Comment 39 Dão Gottwald [:dao] 2009-07-30 14:00:51 PDT
Comment on attachment 391691 [details] [diff] [review]
fixed further comments

>                 <menuitem id="fullScreenItem"
>                           accesskey="&fullScreenCmd.accesskey;"
>                           label="&fullScreenCmd.label;"
>                           key="key_fullScreen"
>                           type="checkbox"
>-                          command="View:FullScreen"/>
>+                          autoCheck="false"
>+                          observes="View:FullScreen"/>

autoCheck="false" isn't really needed, is it?

>+#ifndef XP_MACOSX
>+        <toolbarbutton id="fullscreen-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                       observes="View:FullScreen"
>+                       label="&fullScreenCmd.label;"
>+                       tooltiptext="&fullScreenCmd.label;"/>
>+#ifndef XP_MACOSX

Looking at the other toolbarbuttons, it looks like you shouldn't use &fullScreenCmd.label; as the tooltiptext but add a new entity. This is the only significant remaining issue, sorry for not noticing earlier.

>+toolbar[iconsize="small"] #fullscreen-button > .toolbarbutton-icon {
>+  padding-left: 1px;
>+}

We seem to do this for all small toolbarbuttons, but I don't understand why. :(
Do you happen to know?
Comment 40 Steffen Wilberg 2009-07-30 15:28:37 PDT
Created attachment 391721 [details] [diff] [review]
with nicer tooltip

(In reply to comment #39)
> (From update of attachment 391691 [details] [diff] [review])
> >                 <menuitem id="fullScreenItem"
> >+                          autoCheck="false"
> 
> autoCheck="false" isn't really needed, is it?
I don't think so, no. At least I can't check whether the checkbox is set on the menuitem in full screen mode since I can't access the menu bar there.

> Looking at the other toolbarbuttons, it looks like you shouldn't use
> &fullScreenCmd.label; as the tooltiptext but add a new entity. This is the only
> significant remaining issue, sorry for not noticing earlier.
Fixed.

> >+toolbar[iconsize="small"] #fullscreen-button > .toolbarbutton-icon {
> >+  padding-left: 1px;
> >+}
> 
> We seem to do this for all small toolbarbuttons, but I don't understand why. :(
> Do you happen to know?
Nope. Mano added that in bug 353673 without explanation. Maybe the buttons are too squeezed together without it, or it interferes with the hover box. I didn't try to remove it though.
Comment 41 Dão Gottwald [:dao] 2009-07-30 15:36:45 PDT
Comment on attachment 391721 [details] [diff] [review]
with nicer tooltip

>+        <toolbarbutton id="fullscreen-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                       observes="View:FullScreen"
>+                       label="&fullScreenCmd.label;"
>+                       tooltiptext="&fullScreenButton.tooltip;"/>

Seems like you missed type="checkbox".

Looks good otherwise. The new string needs ui-review.
Comment 42 Dão Gottwald [:dao] 2009-07-30 15:38:04 PDT
(In reply to comment #40)
> Mano added that in bug 353673 without explanation. Maybe the buttons are
> too squeezed together without it, or it interferes with the hover box.

The icons look misaligned over here because of that padding. We need to remove it, I think, but that can happen in a separate bug.
Comment 43 Steffen Wilberg 2009-07-31 00:14:14 PDT
Comment on attachment 391721 [details] [diff] [review]
with nicer tooltip

Alex, this patch adds an optional "full screen" toolbar button. We already have the icon in Toolbar.png, as you noted 16 months ago in comment 24.

Requesting ui-review on its tooltip "Display the window in full screen". Thanks.
Comment 44 Jennifer Morrow [:Boriss] (UX) 2009-07-31 06:28:41 PDT
Comment on attachment 391721 [details] [diff] [review]
with nicer tooltip

ui+
Comment 45 Steffen Wilberg 2009-07-31 08:59:39 PDT
Created attachment 391889 [details] [diff] [review]
with comment 41 addressed
Comment 46 Dão Gottwald [:dao] 2009-07-31 11:01:48 PDT
http://hg.mozilla.org/mozilla-central/rev/702e686f3bcd
Comment 47 Dão Gottwald [:dao] 2009-07-31 11:44:46 PDT
and http://hg.mozilla.org/mozilla-central/rev/94cd140316fb
Comment 48 Steffen Wilberg 2009-07-31 15:40:57 PDT
Thanks for the quick reviews and the pushes, and sorry for the bustage.
Comment 49 Steffen Wilberg 2009-08-01 02:07:35 PDT
Might want to announce this in the release notes since it's hidden in the customize dialog by default.
Comment 50 Henrik Skupin (:whimboo) 2009-09-22 14:25:14 PDT
Verified fixed on 1.9.2 for builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20090922 Namoroka/3.6b1pre ID:20090922041132
Comment 51 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-30 13:06:55 PDT
Removing relnote; called out as a new feature.
Comment 52 Mike Kaply [:mkaply] 2009-11-01 08:01:01 PST
did we have Mac fullscreen before this?

Should we also be calling out that we have mac fullscreen support now?
Comment 53 Henrik Skupin (:whimboo) 2009-11-01 08:17:10 PST
(In reply to comment #52)
> did we have Mac fullscreen before this?

We had until 1.0 or 1.5.

> Should we also be calling out that we have mac fullscreen support now?

We haven't done that in the beta release notes? See bug 505699 where we put a relnote on it and which has been removed by Beltzner. Please follow up there or with a message in the newsgroup. It's nothing for this bug.
Comment 54 Steffen Wilberg 2009-11-01 12:01:11 PST
No, neither this bug (add an optional Full Screen toolbar button) nor bug 505699 (make full screen work on OS X at all) are mentioned in the release notes for 3.6b1. Full Screen *video* is mentioned, but that's not the same.
Comment 55 Chris Ilias [:cilias] 2009-11-20 12:22:17 PST
user-doc-complete
https://support.mozilla.com/en-US/kb/Navigation+Toolbar+items

Note You need to log in before you can comment on or make changes to this bug.