Closed Bug 1717680 Opened 6 months ago Closed 5 months ago

Remove document.popupNode and document.tooltipNode

Categories

(Core :: XUL, task)

task

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(7 files)

popup.triggerNode can be used in most places. For the image related commands in the docshell (copy image, etc) , docShell.setCommandNode can be used to assign the command node for the context menu.

Blocks: 1717684
Blocks: 428712
Blocks: 1706004
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16e42b5f4819
remove places where document.popupNode and document.tooltipNode are checked within tests, as these checks are no longer needed, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/949c53f7d7e2
modify the test browser_bug423833.js to not set popupNode and instead open a context menu for testing 'Show Only This Frame' command, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/48bd6448afa8
have the test test_bug444800.xhtml just use setCommandNode instead of setting the popupNode, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7847029fc5a2
use popup.triggerNode instead of document.triggerNode when determining bookmarks tooltips, r=mak
https://hg.mozilla.org/integration/autoland/rev/a9166de3f917
use popup.triggerNode instead of document.popupNode in browser menu commands, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3f29b77115a5
manage the last context menu trigger node in places utils rather than using document.popupNode, r=mak
https://hg.mozilla.org/integration/autoland/rev/141b15981515
remove the now unused document.popupNode and document.tooltipNode properties, r=smaug

how is a tooltip supposed to know what it's to be anchored to? I'm completely lost here

The tooltip element has a triggerNode property for this purpose.

but you have to reference the tooltip itself to get there. so the only way to find it would be if the event happens to target the tooltip, or if the function already knows upfront what tooltip it cares about. document.tooltipNode and document.popupNode exist on the document, needless to say. so anything can find it. the reason this concerns me is for the sake of this. the tooltip situation is less alarming, since usually you wouldn't need to do much with a tooltip from anywhere except the tooltip's popup events. but for context menus this causes a big problem.

I previously made a mouseleave handler that stops what it's doing with a single expression if there's a context menu open. the pane is built to work like microsoft edge's, so it automatically expands when hovering or focusing within it. but let's say you right-click something in the pane. that sends a mouseleave event. with document.popupNode I can literally just say if (document.popupNode) return; or whatever. it's very cheap and easy, I don't see what the downside is. is there some kind of security risk? or is this just for the sake of consolidation?

to do the same thing without document.popupNode, I need to search the whole document for context menus (or search the whole element for context attributes and use them to find relevant context menus) and check every one of those context menus to see if its triggerNode property is null. only then can I say definitively whether the mouse left the element (or focus was lost) because a context menu was opened. I just did all that to un-break my vertical tabs pane, but hopefully there will be a better way, since I feel dirty having written all that.

I might just be missing something obvious, I have only gotten into firefox relatively recently, but it looks like the fullscreen nav toolbox-hiding behavior does essentially the same kind of thing, and always has. but I can understand why in that case, because it's concerned with any popups, across the whole document and beyond. but really, it shouldn't be necessary to do so much aimless searching just to determine whether a context menu is open. I thought this is exactly what things like the xul popup manager are for. so computation isn't wasted on multiple components simultaneously searching the DOM for the exact same things.

oh, I think I misrepresented the fullscreen handler. in that case, it sets up popuphidden and popupshown events on the whole chrome document that toggle a boolean, which it can reference to say whether a relevant popup is open. which seems like a decent solution since the parameters are so broad in that case. it only needs to determine whether the popup is meant to appear anchored to the toolbox. but in my case it needs to determine if the popup is anchored to the pane. it seems crazy to have the pane consuming events every time a popup is opened throughout the entire document, just on the off chance that one of them happens to be opened on the pane.

I figure most javascript developers would rather check if a popup is opened at the moment when it's actually relevant to behavior, (e.g. upon the blur or mouseleave events) rather than setting up listeners for a whole new category of events we wouldn't otherwise care about, whose sole purpose is to basically create a property identical to what document.popupNode already did.

and how many components will end up doing that in the future now, recreating this property that previously existed on a global object? if others end up requiring this basic functionality too, they will probably end up creating event listeners for exactly this purpose, local to the specific component they are developing. and then fast forward a few years and there would be a bunch of redundant listeners doing the same thing, all over the browser.

anyway I just think it's a misjudgment that these properties are unnecessary or redundant somehow. afaik there isn't any real substitute for them. the removal of these properties requires approaching some things in a completely different way. I can see how, just looking at the places utils and tooltip popupshowing functions, etc., it looks like a simple change. but it would significantly increase the complexity of anything similar to what I was doing, preventing the auto-hide behavior when a context menu is open.

Blocks: 1719985
Regressions: 1719856
Regressions: 1721047
Regressions: 1724395
You need to log in before you can comment on or make changes to this bug.