729011 - In fullscreen mode content.innerHeight is 1 pixel smaller than the screen height

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alice0775 White]

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.,

Comment 1

[Diogo Golovanevsky Monteiro [:diogogmt]]

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

Comment 2

[Jurrie Overgoor]

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...

Comment 3

[:Gijs (he/him)]

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).

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

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

Comment 7

[Dão Gottwald [:dao]]

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?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 9

[Dão Gottwald [:dao]]

(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 :/

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

This should work then.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

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

Comment 12

[Dão Gottwald [:dao]]

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?

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

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 14

[Dão Gottwald [:dao]]

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 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 16

[Dão Gottwald [:dao]]

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.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Then it turns out we cannot use CSS.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b2b0a09c6e1a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2b0a09c6e1a

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 22

[Marat Tanalin]

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.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

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?

Comment 24

[Marat Tanalin]

(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.

Comment 25

[Marat Tanalin]

(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.

Comment 28

[:Gijs (he/him)]

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

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

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?

Comment 30

[:Gijs (he/him)]

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

No.

Comment 31

[Masatoshi Kimura [:emk]]

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

Comment 33

[Masatoshi Kimura [:emk]]

NPAPI is gone. Please consider re-landing this.

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 729011 - Make fullscr-toggler not affect viewport size. r?dao

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

Submitted a new patch for this, basically the same as the previous approved one.

Comment 36

[Pulsebot]

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

Comment 37

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/73aa4b7102da