Search dropdown doesn't display when create wikitext link was recently used

[hoelzro]

Steps to reproduce:

• Load https://tiddlywiki.com (I loaded a version I built from d50e2df - the latest revision at the time of writing)
• Click new tiddler button
• Hit Ctrl-L in the editor to prompt for a wikitext link - type something (I did tag)
• Click on a link to insert a link to that tiddler into the tiddler being edited
• Click on the search editor in the sidebar, typing something (I did full)
• Note that the search results popup doesn't display

I've managed to reproduce this on Firefox (62.0.3) and Chromium (69.0.3497.100-1)

Here's a GIF demoing the bug:

I think I've seen something similar with the tag dropdown, but I haven't taken the time to reduce that to a series of reproducible steps.

[hoelzro]

Looking at the code, I think what's happening is that the tc-popup-keep is preventing the popup mechanism from cleaning up the popup state when the link is clicked; I'm not sure what the best way to fix this is. My feeling, having only superficial experience with the codebase, would be to monitor for the destruction of the popup state tiddler (which is deleted by the link dropdown actions) and cancel the corresponding popup, but maybe Jeremy has a better idea. An alternative would be that when creating a new popup, any existing popups with an ancestor hierarchy independent of the new popup would be canceled.

[twMat]

I suspect the wikitext link button doesn't delete or reset the dropdown state tiddler properly.
If that is the case, then this is probably a sub-question to #2959 - where I actually note something similar to your observation on March 18.

[BurningTreeC]

Hi @hoelzro , @twMat ... the bug here is that the popup gets cancelled clicking on the page, but not when clicking within input fields. I solved that "my way" in the keeboord plugin at https://burningtreec.github.io/KeeBoord (in development) / http://keeboord.tiddlyspot.com (stable)

[hoelzro]

@twMat You're right - it doesn't delete the dropdown state, because tc-popup-keep prevents the popup logic from doing so. Thanks for linking to that issue - it looks like this issue is related, if not a duplicate!

@BurningTreeC Is that with all input fields, or just with input fields with tc-popup-handle set? AFAICT avoiding canceling a popup when an element has tc-popup-handle set is the intended behavior, but I think it would probably be better if a popup handle element were somehow associated with its popup, so that, for example, I can click on the wiki search text edit field to cancel the tag selection dropdown.

[hoelzro]

I wrote up a change to fix this in #3490; I don't know if this is how @Jermolene would like it done, but I'm going to experiment with my change over the next few days and make sure it doesn't break other things.

[BurningTreeC]

Is that with all input fields, or just with input fields with tc-popup-handle set?

with all input fields. before triggering a popup, my solution cancels all. I do it in core/modules/editor/engines/framed.js and core/modules/editor/engines/simple.js in the corresponding handleFocusEvent prototype

your solution I guess wouldn't handle the tags-dropdown when clicking within the text editor, right?
I find it very annoying that I have to click somewhere outside to cancel that popup

[hoelzro]

@BurningTreeC

your solution I guess wouldn't handle the tags-dropdown when clicking within the text editor, right?

Correct - my solution doesn't handle that.

[BurningTreeC]

@hoelzro there's a Pull requests I made once but I closed it. It contains my version of the fix and adjustments for <Tab> navigation in the edit input fields: #3309

do you think we can mix the best of both (yours and mine) approaches?

[hoelzro]

@BurningTreeC I'd welcome integrating our solutions, as long as @Jermolene thinks they both merit inclusion!

[Jermolene]

Hi @hoelzro thanks for the very clear bug report, and thanks @BurningTreeC and @twMat too. It'd be great if you could put your heads together on this.

[BurningTreeC]

@hoelzro @Jermolene @twMat

Currently, without further modifications, popups get cancelled when clicking on the page, but not when clicking within input fields or textareas or within the text editor

There already is a listener for clicks on the page, which cancels popups when clicking. My approach then inserts in the focus listeners of the input, textarea and text-editor iframe fields.
I preferred to cancel popups when those fields get focus, because of my work on keyboard shortcuts to focus inputs. See https://burningtreec.github.io/KeeBoord - when editing a tiddler, alt-Tab and alt-shift-Tab lets you jump to the next/previous editor input and popups get cancelled without having to click somewhere

Input fields can have popups that open when they get focus and focus can set not just by clicking, also using Tab

[BurningTreeC]

@hoelzro I've played with your PR and I think your approach is good!

I'd like to consider two things:

• can we move your checks into its own prototype in popup.js (I call it detectCancellablePopups here) so that we can use the focus event in editor/engines/simple.js and editor/engines/framed.js to call it with $tw.popup.detectCancellablePopups...
• can we add an option that, if set to true, detects if another passed domNode contains the popup and if so, doesn't cancel it? What I have in mind for this are the popups in the editor toolbar. I'd like the editor to cancel the tags dropdown or the fieldname dropdown for example, but keep the dropdowns from the editor toolbar buttons, if selecting text in the editor or the like. So in the focus event handler of the editor we could call $tw.popup.detectCancellablePopups({ownerDomNode: this.widget.parentDomNode}) and if that option isn't missing and the popup is contained in that node, don't cancel it.. ?

[hoelzro]

@BurningTreeC

With regards to moving the logic I wrote into its own function - definitely!

With regards to the DOM node option, I'm not sure if I understand you entirely. I'm thinking what you want can be done by just passing a different DOM node to the function I'll be creating to encapsulate the logic I wrote, but maybe not. In your example, you mention that closing the editor toolbar popups should not be closed by clicking on the editor; what are some examples of popups that should be closed, in your opinion?

[BurningTreeC]

In your example, you mention that closing the editor toolbar popups should not be closed by clicking on the editor; what are some examples of popups that should be closed, in your opinion?

In my opinion, clicking the editor (and any other input field, too) should behave like clicking anywhere else on the page, thus, close all popups.

My idea was to eventually be able to keep the toolbar popups by passing the node that contains the toolbar to your function and add a test if the popup is contained within that node and don't cancel it if so.

But that's probably something I should add on my own in a plugin, so we can let that aside.

The more interesting question is if we call your function from popup.show or if we call it from within framed.js and simple.js (see core/modules/editor/engines/... ) in a handleFocusEvent Method. The difference is that it would do what we want, thus, close popups when focusing input fields that have their own dropdowns, and it would make it possible to cancel popups with clicks in textareas and inputs that don't have dropdowns

[BurningTreeC]

we also need a check if the input or textarea is contained within the popup, and don't cancel it in that case neither

[hoelzro]

Ok, I see - one roadblock, then, is things currently using tc-popup-handle, such as the search text field. TiddlyWiki currently (and deliberately, I believe) ignores clicks on those elements with regards to closing popups, so our continued work should as well, right?

Here's how I'm thinking the popup behavior should be specified:

• Clicking anywhere, including on input fields, should close any open popups, with the following exceptions:
  • Clicking on a popup with tc-popup-keep shouldn't close that popup
  • Clicking on an element with tc-popup-handle shouldn't close the corresponding popup (currently it blocks any popup from being closed)
• When a popup is closed, it should not interfere with new popups being opened (which is what my patch focuses on)
• Nested popups (eg. export tiddler) should still work

Sorry for the verbosity and redundance - I have a nine week old daughter at home, so focus and concentration are lacking these days! Please let me know if you think these rules need adjusting.

[Jermolene]

I have a nine week old daughter at home, so focus and concentration are lacking these days!

Congratulations @hoelzro! Now's the time to start on that TiddlyWiki-fied multimedia baby progress record...

Going back the popup manager, I think there may well be some fundamental design flaws in there. It was implemented very early, before TW5 had a lot of the expressiveness it has today, and before I'd got my present understanding of the design issues involved in balancing state between the DOM and the tiddler store.

Soooo, I suspect that this might be one of those situations where the choice is between:

• Minor tweaks to fix the most egregious problems, maintaining backwards compatibility
• A wholesale redesign benefitting from what we've learned in the last 5 years (!) at the cost or partial or complete incompatibility. This could be done by fixing the existing primitives (reveal, button etc.) or by creating a new parallel set of primitives and deprecating the old ones.

And of course it might make sense to do both....

[BurningTreeC]

I have a nine week old daughter at home, so focus and concentration are lacking these days!

Congratulations also from me @hoelzro ... ! No problem, we continue the discussion here, no need to feel forced to join in

@Jermolene the question that stops this here is if inputs and textareas should cancel popups when they get focus (and not just when they are clicked) or not. @hoelzro's solution will cancel popups when an input gets focus that has a dropdown itself. Inputs without dropdowns won't cancel popups neither by clicking or focussing them by <Tab>.

If we have an answer to this question I think we can start figuring out where to change or add something

I believe inputs and textareas (Editor included) should always cancel popups if they get focus.
For the Editor I have in mind adding an option to @hoelzro's function that accepts the domNode that surrounds both the toolbar and the Editor and checks if a popup is contained within it so that there's an option not to cancel them if needed

[twMat]

[Off topic]

• A wholesale redesign benefitting from what we've learned in the last 5 years [...]

@Jermolene ...are you teasing us? ;-) Please speak up about intentions/plans/thoughts - otherwise who could possibly spend time on popup mechanisms that might possibly, if at all, be used in something a decade or two from now?

[Jermolene]

Hi @twMat here I was just talking about the popup mechanism, and pointing out that we might end up wanting to improve it in ways that would break backwards compatibility (maintaining the precise semantics of all the magic CSS classes is a burden), and suggesting strategies for dealing with it if we do.

[hoelzro]

Thanks everyone! I agree with @BurningTreeC in that inputs getting focus (via click or keyboard tabbing) should cancel popups, with the exceptions I mentioned above. I'm not sure what the right design for the editor toolbar is; an alternative would be to have the editor associated with the toolbar popups as a handle (like the search input is a handle), but maybe that's anchoring the idea too much to the existing design.

I think that it might make sense to revisit the popup mechanism's underlying design - I'm hoping we don't need to break backwards compatability too much! @Jermolene what are some of the other design issues that you can think of with the popup code? I'm thinking we might be able to get away with some smaller tweaks around tc-popup-handle by associating handles directly with their popups, and tc-popup-keep by cleaning up internal popup state, either by detecting the deletion of popup state tiddlers, or by storing the internal state in the state tiddlers themselves, but I'm pretty new to this code!

[Jermolene]

Hi @hoelzro @BurningTreeC

@Jermolene the question that stops this here is if inputs and textareas should cancel popups when they get focus (and not just when they are clicked) or not. @hoelzro's solution will cancel popups when an input gets focus that has a dropdown itself. Inputs without dropdowns won't cancel popups neither by clicking or focussing them by <Tab>.

Cancelling popups when focusing inputs that don't have a dropdown sounds reasonable, but I think would require us to introduce a new magic class to convey whether there is a dropdown, hence my comments about backwards compatibility.

@Jermolene what are some of the other design issues that you can think of with the popup code?

I was particularly thinking of the burden of maintaining the same semantics for the magic CSS classes that we use. The JS code is brittle and hard to modify so I wouldn't be surprised if some refactoring was needed to keep things comprehensible.

[hoelzro]

Understood - regarding the "input focus popup cancelation" problem, what do you think about having a data attribute on an input element, rather than tc-popup-handle, to signify a handle as well as to indicate to which popup the handle corresponds? This is the association mechanism I alluded to in my previous posts; I imagine it would replace this:

<$edit-text class="tc-popup-handle" />

...with something like this:

<$edit-text data-popup=<<popupState>> />

[hoelzro]

Thinking about this some more - I wonder how much tc-popup-handle overlaps with focusPopup?

[hoelzro]

A brief search of the code reveals that tc-popup-handle is always used with focusPopup, and there are only two instances of tc-popup-handle without focusPopup (namely, in $:/Manager and the info popup for a tiddler)

[hoelzro]

Sorry for the lack of movement on this - I'm going to start looking at this again. I'm thinking that a good way forward might be to remove tc-popup-handle; this is what that would look like:

• Use focusPopup in $:/Manager and the info popup tiddler
• Modify the popup code to treat focusPopup elements as it currently treats tc-popup-handle
• Further enhance the popup code so that focusPopup elements are only counted towards their corresponding popups
• Remove all references to tc-popup-handle

After this, we can add some code to make sure that changes in focus also trigger popup hiding, as @BurningTreeC mentioned.

[BurningTreeC]

@hoelzro, is this still an issue in the current prerelease?

[BurningTreeC]

@hoelzro, is this still an issue in the current prerelease?

[hoelzro]

@BurningTreeC This seems to be working now, thanks for following up!

[BurningTreeC]

@hoelzro @Jermolene so this issue can be closed