User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9 When Firefox is in full screen, none of the buttons on the toolbar are clickable at the very top of the screen. Fitts' Law states that the edge of screens are easier to click. Reproducible: Always Steps to Reproduce: 1. Set Firefox to Full Screen 2. Put mouse cursor at very top of screen 3. try to click any button Actual Results: Desired button was not clicked. Expected Results: Button should be clicked.

Setting polish keyword.

isn't this a dupe of something?

Certainly none that I can see, is it actually a theme bug though?

(In reply to comment #2) > isn't this a dupe of something? I searched for a dupe using the keywords "full screen" and "fullscreen" but I didn't find anything that looked the same. I'm open to being corrected, so feel free to dupe. :)

If someone would submit a patch for this, that'd be great.

As far as I can tell, there is no way for a theme developer to be able to tell if the window is in fullscreen mode or not, at the moment the fullscreen toggle function is finding out through a javascript variable, maybe there should be a DOM attribute. Thoughts?

toolbar { border-top-width: 0px !important } in userChrome.css does the trick, I'd rather not go with a hack, the borders are "right" in Pinstripe, but I'm not sure if they're overriding the top value as well.

This is an actual usability problem in full screen mode, it shouldn't be fixed with a userChrome hack. Furthermore, that doesn't fix the problem completely anyway because it doesn't fix the window control Buttons (Minimize,Restore and Close)

Works for me. - Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1 - Microsoft Windows 2000 Pro 5.00.2195 SP4

(In reply to comment #9) > Works for me. > > - Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 > Firefox/0.9.1 > - Microsoft Windows 2000 Pro 5.00.2195 SP4 It is not fixed for me using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040629 Firefox/0.9.1 (MOOX-AV) on Windows XP at the very top (I think is just 1 or 2 pixel) it is not clickable.

This patch makes two changes: 1) The Navigation bar's top border is removed when entering fullscreen mode, and placed again when you return. It is now possible to hit the buttons when the mouse is on the top edge of the screen. 2) The box style and padding is changed for the window controls (minimize, restore, close) in the top right. They are now in the top right corner of the screen, so they also can be hit when the mouse is on the top edge of the screen. Some testing help would be appreciated. The top right control box positioning is theme dependant, so 2) is only fixed for winstripe. A pinstripe fix can be made easily, but was not included, because I couldn't do the edit easily since I'm on windows.

not a PR blocker

I've just realized it's not feasible to hard code white as the border color for the navbar. Custom themes could change this. My patch should therefore probably just change the border size and not the border color. I'll be attaching a new patch shortly.

This is an updated patch that shouldn't break third party themes. What it does: 1) Removes the navbar's top border when going into fullscreen mode, and puts it back when coming out of fullscreen, so that the mouse can catch the Home, Reload, and Stop buttons on the top edge of the screen. 2) Moves the window control boxes to the top edge of the screen. Known Issues: 1) The window management buttons still need to be moved 2 pixels to the right, and I can't seem to get rid of that space.

If anyone has any ideas as to how to remove those spaces to the right of the control buttons I'd be greatful. I tried increasing the padding on the buttons but that just makes more space on the left. The appropriate code is here: http://lxr.mozilla.org/aviarybranch/source/browser/themes/winstripe/browser/browser.css#633 Will wait until I sort out the last issue before asking for review.

(In reply to comment #14) > Created an attachment (id=162557) > Patch v2 Note also that that patch only makes the change for Winstripe. I assume that the same change is necessary for Pinstripe.

Taking

Third try. Works for Winstripe, but setting a negative margin is kind of an ugly workaround. Haven't tested it in pinstripe.

This is a cleaner solution I think. I'm kind of wary of messing with the theme stuff though, so maybe Kevin Gerich should weigh in on this to double check that I'm not breaking anything. At the moment this is winstripe only.

Kevin, mind taking a look at this?

Comment on attachment 202860 [details] [diff] [review] patch v6 Passing request to beltzner, I will review this once we're done UE-wise ;)

Comment on attachment 202860 [details] [diff] [review] patch v6 This patch doesn't seem to do what it's supposed to on w32/Gecko/20051116 Firefox/1.6a1 ... when I put my mouse at the very tippy-top of the screen, buttons don't respond to clicks.

remove special styling in fullscreen

patch v7 doesn't seem to have the desire effect: - Applied patch v7 to HEAD, build as: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051119 Firefox/1.6a1 - Compare to 1.5RC3 (same config build from source): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051118 Firefox/1.5 In full screen mode, both result the same. Build config as: ac_add_options --enable-optimize ac_add_options --disable-debug ac_add_options --enable-default-toolkit=gtk2 ac_add_options --enable-xft ac_add_options --disable-static ac_add_options --enable-shared ac_add_options --enable-extensions=default ac_add_options --disable-tests ac_add_options --enable-cryptography Let me know if anything I could help further.

I've tested it on Windows, Classic and Luna, and it seems to work for me. If it doesn't work for linux, I can address that in a followup patch once I find a way to test, but since it doesn't regress anything on Linux (and Mac doesn't have fullscreen at this point), I think having it work on Windows is better than having it not work at all.

Comment on attachment 207274 [details] [diff] [review] updated v7 let me know if this doesn't apply :)

Gavin, re: updated v7 I think I am a bit late... it might no longer apply to the current cvs HEAD. Possible to have another update for me to try out? patching file browser/base/content/browser.js Hunk #1 succeeded at 3146 (offset 30 lines). Hunk #3 succeeded at 3194 (offset 30 lines). patching file browser/themes/winstripe/browser/browser.css Hunk #2 succeeded at 438 (offset -13 lines). Hunk #3 FAILED at 658. 1 out of 3 hunks FAILED -- saving rejects to file browser/themes/winstripe/browser/browser.css.rej

Sure, here's a patch updated to current trunk (and tweaked slightly). Would appreciate some linux testing.

(In reply to comment #32) > patch v8 Tested this with Classic and Luna on Windows XP.

re: patch v8 Sorry to report that still have no luck on Linux, i.e. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060219 Firefox/1.6a1. Applied patch as below: $ patch -b -i ../patch/bug248330-p8 -p0 patching file browser/base/content/browser.js patching file browser/themes/winstripe/browser/browser.css And have also tried a distclean before build again. F11, and the toolbar behavior is as the same as before patch, and is also as the same as the latest/official 1.5.0.1 release installed on my XP.

(In reply to comment #34) > Sorry to report that still have no luck on Linux Hmm, no change at all? That is kind of surprising... Do you think you could zip up the build you are testing and make it available online? Or email it to me? Maybe I can manage to get a linux machine to test on, but having the build handy would make things easier. > And have also tried a distclean before build again. You only need to rebuild in browser/base and browser/themes for this to take effect, no need to distclean.

Comment on attachment 212420 [details] [diff] [review] patch v8 When I tested this on XP it was 99% there, but the "Go" button wasn't responding like the others. I didn't try adding any other buttons to the toolbar, but when testing, you should make sure that it works for all of them, too. (From my cursory reading of the patch, and as someone who sort of understands how this works, I'm not sure _why_ the Go button wasn't working ...)

Gavin told me in IRC that the Go button thing depends on 235277, which he'll fix and that'll fix what I mentioned and then ui-r=me and everyone can be happy and content and peaceful like my napping kitty: http://www.beltzner.ca/webdav/puddin.jpg

Comment on attachment 212420 [details] [diff] [review] patch v8 Should work now that bug 235277 is fixed.

(In reply to comment #35) > Hmm, no change at all? That is kind of surprising... Do you think you could zip > up the build you are testing and make it available online? Or email it to me? > Maybe I can manage to get a linux machine to test on, but having the build > handy would make things easier. Sure thing. Doing tar & bz2 as we speak. If I didn't catch you on #firefox tonight, I will try to email you (not sure the size at this moment). > You only need to rebuild in browser/base and browser/themes for this to take > effect, no need to distclean. Could you show me the command how? When I tried that, it says nothing to do. BTW, the reason I tried distclean was, after patch applied, I did this: $ gmake -f client.mk build And see no change of the result build, I was surprised too, hence try again with this: $ gmake -f client.mk distclean && gmake -f client.mk build NB. Patch was applied in both cases, I believe.

Comment on attachment 212420 [details] [diff] [review] patch v8 >Index: browser/base/content/browser.js >=================================================================== > showXULChrome: function(aTag, aShow) > { > var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > var els = document.getElementsByTagNameNS(XULNS, aTag); >+ var toolbox = document.getElementById("navigator-toolbox"); > > var i, savedMode, savedIconSize; > for (i = 0; i < els.length; ++i) { ... > els[i].removeAttribute("context"); >+ >+ // Set the inFullscreen attribute to allow specific styling >+ // in fullscreen mode >+ els[i].setAttribute("inFullscreen", true); >+ toolbox.setAttribute("inFullscreen", true); ... > // XXX see above. > if (els[i].hasAttribute("saved-context")) { > var savedContext = els[i].getAttribute("saved-context"); > els[i].setAttribute("context", savedContext); > els[i].removeAttribute("saved-context"); > } >+ >+ els[i].removeAttribute("inFullscreen"); >+ toolbox.removeAttribute("inFullscreen"); > } You shouldn't set toolbox attribute on each loop iteration. >Index: browser/themes/winstripe/browser/browser.css >=================================================================== >+#navigator-toolbox[inFullscreen="true"], toolbar[inFullscreen="true"] { >+ border-top: 0px; >+ -moz-appearance: none; >+} >+ Do we really need the attribute on both? How about navigator-toolbox[inFullscreen="true"], navigator-toolbox[inFullscreen="true"] toolbar { ... } Also, should we use the new attribute for hiding the toolbars (in the stylesheet) instead of using moz-collapsed?

Notes to self: need to ensure that the margin changes don't negatively impact the non-fullscreen case, and /toolkit/content/fullScreen.js can be cvs removed.

I refactored the code a little bit. I'm a little iffy on whether factoring saveAttr() helps or hurts. Cuts down on lines, but we now need to get "mode" twice. I can go either way, let me know what you think. I've tested this on windows with Luna and Classic. I also removed the toolkit fullScreen.js, which isn't used, and removed the code from bug 174174 which as far as I can tell isn't needed per vlad's comment (I can't reproduce bug 174174 with the code removed).

Comment on attachment 242829 [details] [diff] [review] patch My personal preference is to remove saveAttr but keep restoreAttr, either way is fine though. >Index: browser/themes/winstripe/browser/browser.css >=================================================================== >+#navigator-toolbox[inFullscreen="true"], >+#navigator-toolbox[inFullscreen="true"] > #nav-bar { >+ border-top: 0px; >+} >+ nit: border-top: none; r=mano.

mozilla/browser/base/content/browser.js 1.725 mozilla/browser/themes/winstripe/browser/browser.css 1.47 mozilla/toolkit/content/fullScreen.js (removed) Confirmation that this is fixed on Linux would be appreciated.