Build Identifier: http://hg.mozilla.org/mozilla-central/rev/0a7410527788 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/13.0 Firefox/13.0a1 ID:20120220074932 Fullscreen is not real full screen. content.innerHeight has a smaller 1px than monitor size. Reproducible: Always Steps to Reproduce: 1. Start Firefox with new profile 2. Go any page 3. F11 Actual Results: content.innerHeight has a smaller 1px than monitor size Expected Results: content.innerHeight should be equal to monitor size. Google chrome and Opera does.,

I'm also getting the 1px difference on linux.

I also suffer from this issue. Running 15.0.1 on Ubuntu 12.04. This issue also seems to affect CSS Media Queries. @media (width: 1920px and height: 1080px) { ... } does not work, @media (width: 1920px and height: 1079px) { ... } does. Ieuw...

This is because of: http://hg.mozilla.org/mozilla-central/annotate/c2b375f3a909/browser/base/content/browser-fullScreen.js#l53 In order to fix this, we would probably want to have another method of detecting these events, or ensure that the element overlaps the browser rather than pushing it down (perhaps by using margins + z-index).

Not sure whether this could have performance impact, but it shouldn't be large, if any, I suppose.

Comment on attachment 8723414 [details] [diff] [review] patch There's the MousePosTracker helper that might slightly reduce the perf overhead (it's probably a wash though). It's also already used in browser-fullScreen.js, so there could be some room for consolidation. However, AFAIK we don't get mouse events while hovering over a plug-in. So when a plug-in was at the top of the content window that would prevent us from showing toolbars, wouldn't it?

(In reply to Dão Gottwald [:dao] from comment #7) > Comment on attachment 8723414 [details] [diff] [review] > patch > > There's the MousePosTracker helper that might slightly reduce the perf > overhead (it's probably a wash though). It's also already used in > browser-fullScreen.js, so there could be some room for consolidation. Well, I don't think MousePosTracker help performance here, because in this case, we only need one condition that y == 0, however MousePosTracker always requires you to provide a rect, and check each position against the rect, which is 4 conditions. Providing a rect also make the code here even more complicated, because we would need to get the width and build the rect in addition to add this to the tracker. > However, AFAIK we don't get mouse events while hovering over a plug-in. So > when a plug-in was at the top of the content window that would prevent us > from showing toolbars, wouldn't it? Windowed plugin... probably yes. I don't think that's a common case, but it could potentially have security concerns.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #8) > (In reply to Dão Gottwald [:dao] from comment #7) > > Comment on attachment 8723414 [details] [diff] [review] > > patch > > > > There's the MousePosTracker helper that might slightly reduce the perf > > overhead (it's probably a wash though). It's also already used in > > browser-fullScreen.js, so there could be some room for consolidation. > > Well, I don't think MousePosTracker help performance here, because in this > case, we only need one condition that y == 0, however MousePosTracker always > requires you to provide a rect, and check each position against the rect, > which is 4 conditions. That's simple math. Might be less overhead than another event listener. Anyway, I think code consolidation is a stronger argument here. > Providing a rect also make the code here even more > complicated, because we would need to get the width and build the rect in > addition to add this to the tracker. You can just use Infinity as the width, no? > > However, AFAIK we don't get mouse events while hovering over a plug-in. So > > when a plug-in was at the top of the content window that would prevent us > > from showing toolbars, wouldn't it? > > Windowed plugin... probably yes. I don't think that's a common case, but it > could potentially have security concerns. Even if it's not common, when it happens the usability implications would be pretty severe. And yes, someone might exploit this intentionally :/

This should work then.

Add z-index to make it always available even if the toolbar is collapsing.

So with this patch innerHeight will cover the whole screen, and web content will be able to render stuff at the top pixel row, but it won't be able to respond to mouse events there. Are you sure that's better than the status quo?

Well, you are able to: when the toolbar slides down, you can do whatever you want on the top pixels. The toolbar wouldn't slide up again until you move the pointer further down probably 20px. I don't think it makes anything worse than our current status. If you move the pointer upwards, is it easy to stop the pointer on the second pixel row without touching the top? I don't really believe. This change also buy us some other improvements: 1. no longer have flick due to pushing content down 2. no longer need to handle carefully when the black line could appear 3. the top line can respond to mouse event even during the toolbar's sliding animation Actually, this bug attracts me again because I tried to fix bug 1244546 and found some hack in our event handling code to workaround this issue [1]. I believe fixing this could help us safely getting rid of this strange hack, and avoid potential conflict with the fix in my mind. [1] https://dxr.mozilla.org/mozilla-central/rev/4ea7408b3eef059aa248f4b00328f8fdb4475112/dom/events/EventStateManager.cpp#4172-4177

Comment on attachment 8726031 [details] [diff] [review] patch > #fullscr-toggler { >+ top: 0; left: 0; nit: new line after ; >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >@@ -1005,18 +1005,16 @@ > <toolbarbutton id="fullscreen-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > observes="View:FullScreen" > type="checkbox" > label="&fullScreenCmd.label;" > tooltip="dynamic-shortcut-tooltip"/> > </toolbarpalette> > </toolbox> > >- <hbox id="fullscr-toggler" hidden="true"/> >- > <deck id="content-deck" flex="1"> > <hbox flex="1" id="browser"> > <vbox id="browser-border-start" hidden="true" layer="true"/> > <vbox id="sidebar-box" hidden="true" class="chromeclass-extrachrome"> > <sidebarheader id="sidebar-header" align="center"> > <label id="sidebar-title" persist="value" flex="1" crop="end" control="sidebar"/> > <image id="sidebar-throbber"/> > <toolbarbutton class="close-icon tabbable" tooltiptext="&sidebarCloseButton.tooltip;" oncommand="SidebarUI.hide();"/> >@@ -1130,9 +1128,11 @@ > </svg:svg> > > </vbox> > # <iframe id="tab-view"> is dynamically appended as the 2nd child of #tab-view-deck. > # Introducing the iframe dynamically, as needed, was found to be better than > # starting with an empty iframe here in browser.xul from a Ts standpoint. > </deck> > >+ <hbox id="fullscr-toggler" hidden="true"/> Why are you moving this node? Would it make sense to hide fullscr-toggler with CSS rather than JS at this point?

Comment on attachment 8728362 [details] [diff] [review] patch Turns out this breaks rushing the mouse pointer to the top of the screen in order to switch tabs; you need to move it back down by 1px before you can select tabs.

Then it turns out we cannot use CSS.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d95f2926e6ca7011ea1a32288f04e22337b0dd Backed out changeset b2b0a09c6e1a (bug 729011) for bug 1257686

The patch is backed out for bug 1257686. The original hack for bug 799523 should have been fixed by bug 1244546 in another way, thus the backout should be safe. Reopen.

Could we apply the fix at least for cases when an IMAGE is viewed directly? This is actually the most critical case that would probably solve the issue for 99%. For example, if we are viewing a 1920x1080 image on a 1920x1080 display, and we have switched to full screen mode EXACTLY to prevent image from being resized with introducing blur, Firefox is currently forced to resize the image ANYWAY by just 1-2px. And when we click the image to view it at full size, scrollbars appear and they overlap the image much more than by 1px of height. With a 4K image on a 4K monitor at OS-level zoom of 200%, the situation is even WORSE: thanks to automatic resize-to-fit, we should be able to view a 4K image ENTIRELY and PIXEL-PERFECTLY (exactly 1 physical pixel per 1 image pixel) in full-screen mode, but we currently can't, so we are forced not just to click the image, but, because of 200% OS-level zoom, we are then getting the image which is 4-times larger then needed (2x2 blurry physical pixels per image pixel), so we are then forced to also press `[Ctrl] + [-]` 4 times (!) to finally view the image at its real size. That's exciting, to say the least. So could we apply the fix to viewing an image as it's apparently a MUCH more common case than some full-screen plugins today? Some relative complexity (though as a web developer, I suspect the complexity wouldn't be that high) of the corresponding conditional logic is probably not a reason to do nothing and instead just wait for years when (never) support for plugins at all is dropped. Please see also the bug 638554 comment 9. Thanks.

For this case, I guess the right way might be to add a context menu item for image named "View in full screen", which uses the DOM Fullscreen API rather than the browser fullscreen mode. This is what we have for <video>s, and I think it makes sense for images as well. And for pure image document, we could probably add some controls onto it to have this available in one-click?

(In reply to comment #23) Maybe this would make sense, but this is in no way mutually-exclusive with fixing the current redundant-one-pixel bug. Limiting the fix for the current bug to the case of viewing images is just a potential way to simplify and speed-up the fixing process for the most of usecases where this 1px is critical.

(In reply to comment #23) Fwiw, also take into account how multiple images are often viewed: multiple images are opened via links on a web page, each image in its own tab (via e.g. Ctrl+Click or Middle-Click), then the user switches to full-screen mode by pressing F11, and uses Ctrl+Tab to switch from one image to another sequentially one by one. It's unlikely the DOM-like full-screen mode of viewing a single image you've proposed would provide this ability. Unlike videos, images ARE typically viewed sequentially.

Can we do more here now non-flash NPAPI is dead on 52?

It doesn't depend on whether non-Flash NPAPI is dead. It depends on whether windowed plugin is dead. Actually bug 1257686, for which we backout the patch here, was for Flash. So have we disabled windowed flash everywhere?

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #29) > So have we disabled windowed flash everywhere? No.

Why the dependency has been removed? Is it possible to reland the reverted patch now?

Pushed by mozilla@upsuper.org: https://hg.mozilla.org/integration/autoland/rev/73aa4b7102da Make fullscr-toggler not affect viewport size. r=dao