Bug 1619751 - Pin blame popup on click, dismiss on scroll or click elsewhere. #308
Conversation
I suspect it won't happen with |
Indeed you are correct. Thanks for the tip! |
static/js/blame.js
Outdated
| } | ||
|
|
||
| BlamePopup.popup.addEventListener("mouseenter", this); | ||
| BlamePopup.popup.addEventListener("mouseleave", this); | ||
| window.addEventListener("click", this, {capture: true}); | ||
| document.getElementById("scrolling").addEventListener("scroll", this, {capture: true}); |
There was a problem hiding this comment.
Also why does this need to be capturing? Same for the scroll listener.
There was a problem hiding this comment.
passive: true?
I don't think passive does anything for scroll events. At least APZ doesn't care about whether scroll listeners are passive or not. Maybe I'm mistaken?
There was a problem hiding this comment.
Also why does this need to be capturing? Same for the scroll listener.
The click needs to be capturing because if you e.g. click on a code fragment that produces a context menu, it seems to consume the click and then we end up with both the blame popup and the context menu. Having a capture listener here ensures we get the click even in that case. I can add a comment to that effect. The scroll listener probably doesn't need to be capturing, I can drop that.
There was a problem hiding this comment.
I don't think passive does anything for scroll events. At least APZ doesn't care about whether scroll listeners are passive or not. Maybe I'm mistaken?
Ah, yeah, maybe I was thinking of wheel events, nvm.
The click needs to be capturing because if you e.g. click on a code fragment that produces a context menu, it seems to consume the click and then we end up with both the blame popup and the context menu. Having a capture listener here ensures we get the click even in that case. I can add a comment to that effect. The scroll listener probably doesn't need to be capturing, I can drop that.
Ah, so this is so that we don't hit the event.stopPropagation() in the context-menu code? That should probably be changed to something else... And actually given now we don't always recreate the menu it may not be needed at all anymore.
There was a problem hiding this comment.
I changed /capture/passive/ for the scroll listener. The passive won't help but it won't hurt either, so whatever.
For the click - I guess it's the stopPropagation() in the context-menu code, but I didn't actually verify. In general it seems like for things like this a capturing listener seems more robust.
There was a problem hiding this comment.
It seems more reliable for hiding the popup, but not for showing it, right? Anyhow nbd.
There was a problem hiding this comment.
It seems more reliable for hiding the popup, but not for showing it, right? Anyhow nbd.
Ah, good point. I was only considering the hiding scenario at the time but you're right it probably is not as good for the showing scenario.
I live-edited the dev web-server to test this, so the patch is applied there and you can try it out if you want.
Also note that if you test in Fenix you may run into mozilla-mobile/fenix#8779 which reopens the blame popup after scrolling. Doesn't happen in Fennec, and I intend to track down and fix the bug in Fenix soonish.