-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Few ideas for fluentRevealNavbar.uc.js #77
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty neat, thanks. I have some feedback, mostly nitpicks.
if (el.disabled || areaStyle.pointerEvents == "none") { | ||
// previous pointerEvents check may break the effect in FF 115 | ||
//if (el.disabled || areaStyle.pointerEvents == "none") { | ||
if (el.disabled) { | ||
return this.clearEffect(area); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below needs to be fixed, or there is a bug when you move the mouse around the top of the page on about:config or about:preferences, etc. It's because they are in the parent process so their true event targets pass all the way up to the chrome window listener (normally the event target we see would be <browser>
).
It's better if we just use MousePosTracker.
let x = MousePosTracker._x - this.getOffset(area).left - window.scrollX;
let y = MousePosTracker._y - this.getOffset(area).top - window.scrollY;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't know about this bug, so my first thought was this check was added just for UX convenience. So now this should be handled by new getComputedStyle(el).pointerEvents == "none"
check, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, there is still some minor cleanup to do though
This PR:
pointerEvents=none
on navbar button, which resulted in script not working for me on FF 115filterDy
, which don't process mouse movements greater than {gradientSize}px from top of the screen, in order to reduce system load (default: true)cacheButtons
, which stores all toolbar buttons on script init and then reuses the list on each draw iteration (default: false)includeUrlBar
, which applies effect to url bar as well (when it's not focused) (default: true)