Closed Bug 515196 Opened 10 years ago Closed 5 years ago

Full Screen Toolbar Autohide Sensitivity

Categories

(Firefox :: Toolbars and Customization, defect, minor)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
Firefox 36

People

(Reporter: mikeclackler, Assigned: akshendra521994, Mentored)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 6 obsolete files)

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.
That's an issue on all platforms and does not only happen with touch pads.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
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
Assignee: nobody → allamsetty.anup
Can you please provide me more details regarding the bug i.e. like where to change, description.
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).
Whiteboard: [good first bug][lang=js][mentor=dao]
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
Assignee: allamsetty.anup → cpt.at.work
Assignee: cpt.at.work → vyz1194
Hi, is Anup working on this bug? can i take it up?
Doesn't look like anybody is working on this at the moment.
Assignee: vyz1194 → nikhiljohny
Attached patch first try (obsolete) — Splinter Review
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
Attachment #8382864 - Flags: feedback?(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()...
Attachment #8382864 - Flags: feedback?(dao)
Hi, Nikhil - Are you still working on this bug?
Flags: needinfo?(nikhiljohny)
Mentor: dao
Whiteboard: [good first bug][lang=js][mentor=dao] → [good first bug][lang=js]
Assignee: nikhiljohny → nobody
Flags: needinfo?(nikhiljohny)
Is anyone working on the bug right now? If not, I would like to work on it.
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?
Thank you Mike for your comment! Waiting for the green light from 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.
Assignee: nobody → migusiel
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.
(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.
Assignee: migusiel → nobody
hey ,

i would like to work on this.i am a newbie. can you assign this to me ? please .
(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)...
Flags: needinfo?(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.
Flags: needinfo?(bakolden5)
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?
(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>?
Would like to pick up this bug. Please assign it to me.
Assignee: nobody → tejdeepg
Its working.
I have used 100px as the padding value, as I was not sure.
Attachment #8514549 - Flags: feedback?(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.
Attachment #8514549 - Flags: feedback?(dao)
Assignee: tejdeepg → nobody
Attachment #8382864 - Attachment is obsolete: true
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?
Flags: needinfo?(dao)
I'd suggest filing a separate bug for that.
Flags: needinfo?(dao)
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
Attachment #8514549 - Attachment is obsolete: true
Attachment #8515446 - Flags: feedback?(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.
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 on attachment 8514549 [details] [diff] [review]
515196_fullscreen_toolbar_autohide.patch

Ok, this looks good then, except for the handleEvent calls. Please remove those.
Attachment #8514549 - Attachment is obsolete: false
Attachment #8514549 - Flags: feedback+
Attachment #8515446 - Attachment is obsolete: true
Attachment #8515446 - Flags: feedback?(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?
Please assign that to me, thanks!!
Attachment #8514549 - Attachment is obsolete: true
Attachment #8515449 - Flags: review?(dao)
Assignee: nobody → akshendra521994
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.
Attachment #8515449 - Flags: review?(dao) → review-
Attachment #8515449 - Attachment is obsolete: true
Attachment #8515493 - Flags: review?(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.
The code containing addListener and removeListener ?
(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.
One more thing I forgot: You need to remove the gPrefService.addObserver and gPrefService.removeObserver calls since you removed the 'observe' method.
Attachment #8515493 - Attachment is obsolete: true
Attachment #8515493 - Flags: review?(dao)
Attachment #8515517 - Flags: review?(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!
Attachment #8515517 - Flags: review?(dao) → review+
Sorry for my impatience with the last patch ?-)
Attachment #8515518 - Flags: review?(dao)
Attachment #8515518 - Flags: review?(dao) → review+
Attachment #8515517 - Attachment is obsolete: true
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
Thanks :)
https://hg.mozilla.org/mozilla-central/rev/02566635767f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
[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.
Status: RESOLVED → VERIFIED
Depends on: 1195927
Depends on: 1192683
Depends on: 1327948
Depends on: 1358734
You need to log in before you can comment on or make changes to this bug.