In full screen buttons at top are not on edge of screen

RESOLVED FIXED in Firefox 3 alpha1

Status

()

P5
minor
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: michael, Assigned: Gavin)

Tracking

({polish})

Trunk
Firefox 3 alpha1
polish
Points:
---
Bug Flags:
blocking-aviary1.0PR -
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

43.17 KB, image/png
Details
10.63 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

15 years ago
Setting polish keyword.
Keywords: polish

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.0?
OS: Windows XP → All
Hardware: PC → All
isn't this a dupe of something?
(Reporter)

Comment 3

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

Comment 4

15 years ago
(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. :)

Comment 5

15 years ago
If someone would submit a patch for this, that'd be great.
Flags: blocking1.0? → blocking1.0-
(Reporter)

Comment 6

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

Comment 8

15 years ago
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)

Comment 9

14 years ago
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

Comment 10

14 years ago
(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.
Created attachment 153619 [details] [diff] [review]
Patch v.1 (for winstripe)

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.
Attachment #153619 - Flags: review?(firefox)
Flags: blocking-aviary1.0PR?
Whiteboard: [have patch]
not a PR blocker
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR-
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.
Created attachment 162557 [details] [diff] [review]
Patch v2

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.
Attachment #153619 - Attachment is obsolete: true
Attachment #153619 - Flags: review?(firefox)
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.
Version: unspecified → 1.0 Branch
(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.
Component: General → Toolbars
Keywords: helpwanted
Whiteboard: [have patch]
Taking
Assignee: firefox → gavin_sharp+bugzilla
Priority: -- → P4
Target Milestone: --- → Firefox1.1
Whiteboard: [needswork]
Created attachment 174117 [details] [diff] [review]
Patch v3

Third try. Works for Winstripe, but setting a negative margin is kind of an
ugly workaround. Haven't tested it in pinstripe.
Attachment #162557 - Attachment is obsolete: true
Severity: normal → minor
Status: NEW → ASSIGNED
Priority: P4 → P5
Version: 1.0 Branch → Trunk
Created attachment 176212 [details] [diff] [review]
Patch v4

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.
Attachment #174117 - Attachment is obsolete: true
Attachment #176212 - Flags: review?(mconnor)
Kevin, mind taking a look at this?
Whiteboard: [needswork] → [patch-r?]
Keywords: helpwanted
Created attachment 187496 [details] [diff] [review]
Updated
Attachment #176212 - Attachment is obsolete: true
Attachment #187496 - Flags: review?(mconnor)
Attachment #176212 - Attachment is obsolete: false
Attachment #176212 - Flags: review?(mconnor)
Attachment #176212 - Attachment is obsolete: true
Target Milestone: Firefox1.5 → ---
Created attachment 202860 [details] [diff] [review]
patch v6
Attachment #187496 - Attachment is obsolete: true
Attachment #202860 - Flags: review?(bugs.mano)
Attachment #187496 - Flags: review?(mconnor)
Comment on attachment 202860 [details] [diff] [review]
patch v6

Passing request to beltzner, I will review this once we're done UE-wise ;)
Attachment #202860 - Flags: review?(bugs.mano) → review?(beltzner)
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.
Attachment #202860 - Flags: review?(beltzner) → review-
Created attachment 203596 [details]
full screen shot using 1.6a in xp with luna theme (no workee)
Created attachment 203602 [details] [diff] [review]
patch v7

remove special styling in fullscreen
Attachment #202860 - Attachment is obsolete: true
Attachment #203602 - Flags: review?(beltzner)

Comment 27

13 years ago
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.
Created attachment 207274 [details] [diff] [review]
updated v7
Attachment #203602 - Attachment is obsolete: true
Attachment #207274 - Flags: review?(beltzner)
Attachment #203602 - Flags: review?(beltzner)
Comment on attachment 207274 [details] [diff] [review]
updated v7

let me know if this doesn't apply :)
Attachment #207274 - Flags: review?(beltzner) → ui-review?(beltzner)

Comment 31

13 years ago
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
Created attachment 212420 [details] [diff] [review]
patch v8

Sure, here's a patch updated to current trunk (and tweaked slightly). Would appreciate some linux testing.
Attachment #207274 - Attachment is obsolete: true
Attachment #212420 - Flags: ui-review?(beltzner)
Attachment #207274 - Flags: ui-review?(beltzner)
(In reply to comment #32)
> patch v8

Tested this with Classic and Luna on Windows XP.

Comment 34

13 years ago
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 ...)
Attachment #212420 - Flags: ui-review?(beltzner) → ui-review-
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
Depends on: 235277
Comment on attachment 212420 [details] [diff] [review]
patch v8

Should work now that bug 235277 is fixed.
Attachment #212420 - Flags: ui-review- → ui-review?(beltzner)

Comment 39

13 years ago
(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.
Priority: P5 → P3
Target Milestone: --- → Firefox 2 beta1
QA Contact: general → toolbars
Attachment #212420 - Flags: ui-review?(beltzner) → ui-review+
Attachment #212420 - Flags: review?(bugs.mano)
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?
Attachment #212420 - Flags: review?(bugs.mano) → review-
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.
Priority: P3 → P5
Whiteboard: [patch-r?]
Target Milestone: Firefox 2 beta1 → ---
Created attachment 242829 [details] [diff] [review]
patch

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).
Attachment #212420 - Attachment is obsolete: true
Attachment #242829 - Flags: review?
Attachment #242829 - Flags: review? → review?(mano)
Whiteboard: [patch-r?]
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.
Attachment #242829 - Flags: review?(mano) → review+
Created attachment 242880 [details] [diff] [review]
comments addressed
Attachment #242829 - Attachment is obsolete: true
Whiteboard: [patch-r?] → [checkin needed]
Target Milestone: --- → Firefox 3 alpha1
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.