Skip to content
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

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

emvaized
Copy link

@emvaized emvaized commented Jul 22, 2023

This PR:

  • Removes check pointerEvents=none on navbar button, which resulted in script not working for me on FF 115
  • Changes default color of effect to browser's color of navbar button's color (--button-hover-bgcolor)
  • Adds new options:
    • filterDy, 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)

Copy link
Owner

@aminomancer aminomancer left a 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.

JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
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);
}

Copy link
Owner

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;

Copy link
Author

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?

JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
@emvaized emvaized requested a review from aminomancer July 23, 2023 22:46
Copy link
Owner

@aminomancer aminomancer left a 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

JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
JS/fluentRevealNavbar.uc.js Outdated Show resolved Hide resolved
@emvaized emvaized requested a review from aminomancer July 25, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants