515196 - Full Screen Toolbar Autohide Sensitivity

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Michael Clackler]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090907 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090907 Minefield/3.7a1pre

The target area for toolbar activity in full screen mode before the toolbar hides is a little unfriendly especially when using touchpads.  When trying to select a tab the toolbar hides when the pointer leaves the toolbar.  There needs to be a slightly expanded target area before the toolbar hides.

Reproducible: Always

Steps to Reproduce:
1. Enter full screen mode
2. Go to the top of the page to open the toolbar
3. Slowly mouse down from the top of the page
Actual Results:  
The toolbar hides as soon as the pointer leaves the toolbar.

Expected Results:  
The toolbar doesn't hide until a few pixels below the toolbar.  Possibly add the  pixel height used to indicate the toolbar is hidden to the target used to hide.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's an issue on all platforms and does not only happen with touch pads.

Comment 2

[Anup Kumar [:reznord]]

Hi,

I am willing to fix this bug. So, please I request you to assign this bug to me.

Thanks in Advance,

Regards,
A. Anup

Comment 3

[Anup Kumar [:reznord]]

Can you please provide me more details regarding the bug i.e. like where to change, description.

Comment 4

[Dão Gottwald [:dao]]

The code in question is here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js

Look for _collapseCallback.

Instead of listening to the mousemove event directly, we should probably use the MousePosTracker helper (implemented here: <http://hg.mozilla.org/mozilla-central/annotate/3c61cc01a3b1/browser/base/content/browser.js#l7288>; see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml for code using it).

Comment 5

[Anup Kumar [:reznord]]

I am unable to get the problem correctly. So I request you to help me regarding this bug on IRC. I will be availale on IRC by the name "Anupkumar". 

Regards,
A.Anup

Comment 6

[Nikhil Johny]

Hi, is Anup working on this bug? can i take it up?

Comment 7

[Dão Gottwald [:dao]]

Doesn't look like anybody is working on this at the moment.

Comment 8

[Nikhil Johny]

Attachment: first try

I am sorry if there are plenty of mistakes in my code. I am new to javascript and this seems to be a good bug to get my hands dirty, that's the reason i took it up. Please correct me if I am wrong. After looking into the files you mentioned. This is what I understood:

1. The MousePosTracker.addListener() will take the listener as parameter and call the _callListener().
2. In the _callListener(), the listener.getMouseTargetRect() is called to get the top, left, bottom and right of the listener.  The mouse position is compared with the rect properties. If they match, hover is set to true and onMouseEnter() is called else onMouseLeave() is called.

i added the getMouseTargetRect(), onMouseEnter() and onMouseLeave().

After that i replaced all the mousemove events with MousePosTracker.addListener(this) and MousePosTracker.removeListener(this) where ever necessary. (this is not shown in the patch as it didn't work). 

Thanks

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8382864 [details] [diff] [review]
first try

>@@ -193,16 +192,35 @@ var FullScreen = {
>   _expandCallback: function()
>   {
>     FullScreen.mouseoverToggle(true);
>   },
>   _collapseCallback: function()
>   {
>     FullScreen.mouseoverToggle(false);
>   },
>+  getMouseTargetRect: function()
>+  {
>+    let rect = this.getBoundingClientRect();

getBoundingClientRect isn't defined on this object. You could try gBrowser.getBoundingClientRect()...

Comment 10

[Mike Hoye [:mhoye]]

Hi, Nikhil - Are you still working on this bug?

Comment 11

[migusiel]

Is anyone working on the bug right now? If not, I would like to work on it.

Comment 12

[Mike Hoye [:mhoye]]

It looks like Nikhil's let it go, so you're welcome to it. Dao, you're still willing to mentor this bug, right? Can Migusiel pick it up where it was left off?

Comment 13

[migusiel]

Thank you Mike for your comment! Waiting for the green light from Dao ;)

Comment 14

[Dão Gottwald [:dao]]

(In reply to migusiel from comment #11)
> Is anyone working on the bug right now? If not, I would like to work on it.

Go ahead.

Comment 15

[bakolden5]

Hi, I'm new to this community. I'm not very familiar with the code to be much help in the short term, but I was thinking a potential change would be to activate a timer when the mouse is removed from the toolbar. When the timer is up, a recheck of whether the mouse is on the bar or not. If it is back on the bar, it stays up. If it is off the bar, it can disappear.

Comment 16

[Dão Gottwald [:dao]]

(In reply to bakolden5 from comment #15)
> Hi, I'm new to this community. I'm not very familiar with the code to be
> much help in the short term, but I was thinking a potential change would be
> to activate a timer when the mouse is removed from the toolbar. When the
> timer is up, a recheck of whether the mouse is on the bar or not. If it is
> back on the bar, it stays up. If it is off the bar, it can disappear.

That's an interesting idea. Feel free to give it a shot. Let me know when you have questions.

Comment 17

[nithin[:lazybug]]

hey ,

i would like to work on this.i am a newbie. can you assign this to me ? please .

Comment 18

[Dão Gottwald [:dao]]

(In reply to nithin from comment #17)
> hey ,
> 
> i would like to work on this.i am a newbie. can you assign this to me ?
> please .

Somebody already wanted to start working on this just yesterday (see comment 15)...

Comment 19

[bakolden5]

I don't feel I have the knowledge to write in JavaScript, I'm more of a c/c++ person. I was just giving a possible solution. Feel free to give the assignment to anyone else.

Comment 20

[Krishna Tulsyan]

Hey,
I am a newbie here..
I would like to work on this bug and like to implement bakolden5's idea of having a timer when the mouse is removed from the toolbar.

can someone help with the description and the process involved to implement this?

Comment 21

[Dão Gottwald [:dao]]

(In reply to Krishna Tulsyan from comment #20)
> Hey,
> I am a newbie here..
> I would like to work on this bug and like to implement bakolden5's idea of
> having a timer when the mouse is removed from the toolbar.
> 
> can someone help with the description and the process involved to implement
> this?

Hi! What kind of description are you looking for? As for the general process, have you seen <https://developer.mozilla.org/en-US/docs/Introduction>?

Comment 22

[tejdeepg]

Would like to pick up this bug. Please assign it to me.

Comment 23

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

Its working.
I have used 100px as the padding value, as I was not sure.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8514549 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

Thanks! This looks pretty good code wise (except for the calls to MousePosTracker.handleEvent, those don't make much sense). Unfortunately, it looks like MousePosTracker won't get mouse events from the content area when hovering over a windowed plugin. :(

For example: https://www.kk-software.de/flash/05_hofer-fabrik.html

So I think we have to resort to the approach from comment 15.

Comment 25

[Akshendra Pratap Singh]

Since we are going to use the timeout mechanism, animating the collapse all the time will look good. I know its not important but I just love eye candies. What do you think?

Comment 26

[Dão Gottwald [:dao]]

I'd suggest filing a separate bug for that.

Comment 27

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

This kind of a draft right now, but I will like your feedback on this. Cause its working but has problem. 
First, is that its still no working on the link(kk-software or something)
Second, the timer seems to start again when the mouse has settled down

Comment 28

[Dão Gottwald [:dao]]

(In reply to Akshendra Pratap Singh from comment #27)
> Created attachment 8515446 [details] [diff] [review]
> 515196_fullscreen_toolbar_autohide.patch
> 
> This kind of a draft right now, but I will like your feedback on this. Cause
> its working but has problem. 
> First, is that its still no working on the link(kk-software or something)

Oh, turns out this is already a problem with an unpatched Firefox. I guess we could after all take the MousePosTracker approach then, since it doesn't make this problem much worse? I'm actually not sure which is more user-friendly, the extra margin or the timer.

Comment 29

[Akshendra Pratap Singh]

I think the timer approach is used at a lot of places but the margin is a better user-friendly effort. I do not want to wait for the timer to run off. And also this was the original implication in this bug.

> I guess we could after all take the MousePosTracker approach then
So should I submit the patch again or you can provide a feedback on the patch I submitted earliar?

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8514549 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

Ok, this looks good then, except for the handleEvent calls. Please remove those.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8514549 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

>+      top: rect.top + 100,

I also think 100 is a bit too much. How about 50?

Comment 32

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

Please assign that to me, thanks!!

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8515449 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

>--- a/browser/base/content/browser-fullScreen.js
>+++ b/browser/base/content/browser-fullScreen.js

>     if (enterFS) {
>-      if (gPrefService.getBoolPref("browser.fullscreen.autohide"))
>-        gBrowser.mPanelContainer.addEventListener("mousemove",
>-                                                  this._collapseCallback, false);
>+      if (gPrefService.getBoolPref("browser.fullscreen.autohide")) {
>+        MousePosTracker.addListener(this);
>+      }

Hmm, this call seems redundant. Can you remove it?

>   observe: function(aSubject, aTopic, aData)
>   {
>     if (aData == "browser.fullscreen.autohide") {
>       if (gPrefService.getBoolPref("browser.fullscreen.autohide")) {
>-        gBrowser.mPanelContainer.addEventListener("mousemove",
>-                                                  this._collapseCallback, false);
>+        MousePosTracker.removeListener(this);
>       }
>       else {
>-        gBrowser.mPanelContainer.removeEventListener("mousemove",
>-                                                     this._collapseCallback, false);
>+        MousePosTracker.removeListener(this);
>       }
>     }
>   },

The first removeListener should be addListener instead. Having said that... this code seems obsolete too.

>+  getMouseTargetRect: function()
>+  {
>+  	let rect = gBrowser.mPanelContainer.getBoundingClientRect();
>+  	return {
>+      top: rect.top + 50,
>+      bottom: rect.bottom,
>+      left: rect.left,
>+      right: rect.right
>+    };
>+  },

nit: there are two tab stops in there that should be spaces instead.

Since this method is called pretty often, I wonder if it would be worthwhile to calculate the rect just once before calling addListener, store it in this._mouseTargetRect and return that here.

Comment 34

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 8515493 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

> var FullScreen = {
>   _XULNS: "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> 
>   init: function() {
>     // called when we go into full screen, even if initiated by a web page script
>     window.addEventListener("fullscreen", this, true);
>     window.messageManager.addMessageListener("MozEnteredDomFullscreen", this);
> 
>+    let rect = gBrowser.mPanelContainer.getBoundingClientRect();
>+    this._targetRect = {
>+      top: rect.top + 50,
>+      bottom: rect.bottom,
>+      left: rect.left,
>+      right: rect.right
>+    };

nit: _mouseTargetRect would be a better, more specific name

I don't think we can reuse this rect throughout the lifetime of the window, as the window could be moved to a different screen with different dimensions. So I think you need to move this right before the addListener call. And I think that whole block needs to be moved down a few lines after the code hiding/showing the toolbox and the toggler.

Comment 36

[Akshendra Pratap Singh]

The code containing addListener and removeListener ?

Comment 37

[Dão Gottwald [:dao]]

(In reply to Akshendra Pratap Singh from comment #36)
> The code containing addListener and removeListener ?

Yep, otherwise getBoundingClientRect() will measure wrong dimensions, if I read the code correctly.

Comment 38

[Dão Gottwald [:dao]]

One more thing I forgot: You need to remove the gPrefService.addObserver and gPrefService.removeObserver calls since you removed the 'observe' method.

Comment 39

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 8515517 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

>+      MousePosTracker.addListener(this);
>+    }
>+    else {

nit: while you're at it, use this style:

    } else {

r+ with comment 38 addressed. ;) Thanks!

Comment 41

[Akshendra Pratap Singh]

Attachment: 515196_fullscreen_toolbar_autohide.patch

Sorry for my impatience with the last patch ?-)

Comment 42

[Dão Gottwald [:dao]]

I removed the obsolete and misleading "chrome is collapsed so don't spam needless mousemove events" comment before landing this...

https://hg.mozilla.org/integration/mozilla-inbound/rev/02566635767f

Comment 43

[Akshendra Pratap Singh]

Thanks :)

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02566635767f

Comment 45

[Mayur Patil]

[bugday-20150121]
Name: Firefox
Version: 36.0
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Multiprocess Windows: 0/1
Channel: beta release

Status: Fixed -> Verified

Works for Hotkeys and Menu.