add button custom recursion detection and protection

[pmario]

This PR fixes #8144

It is intended to stop nesting buttons, because HTML spec clearly states, that button in button is not allowed.
It also prevents transclusion recursion problems if a transclusion without valid parameters is invoked inside a button.

There is a new tv-is-button variable, which can be checked by users to detect, if their code runs inside a button.
Edit: Code has been simplified according to comment: #8155 (comment)

There are 2 new tests which should pass.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
tiddlywiki5	✅ Ready (Inspect)	Visit Preview	Apr 19, 2024 8:40am

[pmario]

[pmario]

@Jermolene -- This PR is related to: implement dynamic config and check for max-widget-tree-depth recursion detection #8147 -- But it only implements the button in button protection, which as a side effect also is a recursion protection.

Together with your PR Fix crash with empty transclusions #8145 this should be a huge "quality of life" improvements

[Jermolene]

Hi @pmario we don't need to use a variable. The button widget can reach up the parent widget chain looking for an instance of itself. There's a similar thing going on in the <$parameters> widget:

TiddlyWiki5/core/modules/widgets/parameters.js

Lines 46 to 57 in a081e58

	// Find the parent transclusions
	var pointer = this.parentWidget,
	depth = this.parametersDepth;
	while(pointer) {
	if(pointer instanceof TranscludeWidget) {
	depth--;
	if(depth <= 0) {
	break;
	}
	}
	pointer = pointer.parentWidget;
	}

[pmario]

@Jermolene - should be ready for review again

[pmario]

@Jermolene - please check this error message

[Jermolene]

Hi @pmario displaying an error message seems heavy handed: I appreciate that stopping the recursion in its tracks is beneficial in the runaway recursion case, I don't think it is appropriate in cases where authors have explicitly nested buttons.

At the moment, nested buttons work perfectly well. For example:

<$button>
<$action-sendmessage $message="tm-modal" $param="HelloThere"/>
Click here to open ~HelloThere
    <$button>
    <$action-sendmessage $message="tm-modal" $param="GettingStarted"/>
    Click here to open ~GettingStarted
    </$button>
</$button>

Therefore, I think there will definitely be wikis out there with nested buttons, and it would not be reasonable to break them all.

So, rather than give an error message, I think perhaps it would make more sense to drop back to a simulated button based on a span that uses CSS to look like a button (eg display: inline-block).

[pmario]

At: #8147 (comment) you wrote

That makes me wonder if an alternate approach might not be to simply suppress rendering nested button widgets? The HTML spec is clear that interactive content is not allowed within buttons.

In your example it's not possible to have a simulated button. Since it also contains an action-widget, which cannot be handled by a span-element.

a) I could include a "depth" counter similar to your "\parameters" example. So we could allow 2 buttons. .. But if you want more we should implement b)

b) If we want to allow buttons in buttons for compatibility reasons, we should implement the "relative limit" as suggested at: #8147 (comment)

[Jermolene]

In your example it's not possible to have a simulated button. Since it also contains an action-widget, which cannot be handled by a span-element.

It would be a <$button> widget that is generating a <span> element instead of a <button> element. The event handlers etc would continue to work as before.

[pmario]

It would be a <$button> widget that is generating a element instead of a element. The event handlers etc would continue to work as before.

But we would be back at the same problem as with the #8147 PR. IMO it's not the html button-element that causes TiddlyDesktop to crash. It's the number of recursive transclusions.

If we create a SPAN we still have the recursion problem. So we can keep the button and just limit the recursion. IMO the "relative" recursion protection code is simple enough to maintain and it seems to work well.

[Jermolene]

But we would be back at the same problem as with the #8147 PR. IMO it's not the html button-element that causes TiddlyDesktop to crash. It's the number of recursive transclusions.

I think I specifically asked about this point in the other thread. As I understood it, the theory was that it was the fact that button elements that were being nested that exacerbated the impact of the recursion, and thus caused the browser to hang earlier than if there were no button elements.

If we create a SPAN we still have the recursion problem. So we can keep the button and just limit the recursion. IMO the "relative" recursion protection code is simple enough to maintain and it seems to work well.

This PR cannot and should not address the recursion problem, because the recursion problem is nothing specifically to do with the button widget. The rationale for this PR is to make TW more compliant with the HTML spec, with the side effect of reducing the impact of recursion involving buttons.

[pmario]

I think I specifically asked about this point in the other thread. As I understood it, the theory was that it was the fact that button elements that were being nested that exacerbated the impact of the recursion, and thus caused the browser to hang earlier than if there were no button elements.

I did a new test with TiddlyDesktop

\whitespace trim
<span>
<$transclude/>
</span>

It shows the error message, which is OK. Open the "DOM Inspector" with right-click -> Inspect crashes to desktop.

So yes. The button-widget makes it worse but spamming the DOM with 1000 nested SPANs does a similar thing to TD. FF on ubuntu can handle the SPANs.

[...] The rationale for this PR is to make TW more compliant with the HTML spec, with the side effect of reducing the impact of recursion involving buttons.

As I wrote. If we want to have backwards compatibility and allow nested buttons, we should add option b). With the relative nesting limit inside buttons. IMO 50 nested widgets inside a button should be enough to allow complex buttons, while still give us recursion-protection.

---

On the other hand as discussed in PR #8147 -- Well defined recursive procedures need the possibility to widen the recursion limit, which is 1000 atm -- I think with the introduction of the UNSAFE_max_widget_tree_depth limit we can have both.

[pmario]

@Jermolene - I did update the PR. It now allows 1 button in button. But as soon as the nesting goes 3 levels deep, there will be an error message. So your example from #8155 (comment) will pass the tests.

[Jermolene]

@Jermolene - I did update the PR. It now allows 1 button in button. But as soon as the nesting goes 3 levels deep, there will be an error message. So your example from #8155 (comment) will pass the tests.

I am concerned that this fix is too narrow. There's no reason that we cannot maintain nearly full backwards compatibility with this change (the glaring exception being that CSS selectors that target the button element directly will not work).

[pmario]

So how deep will you go? 1000 will crash TD. My suggestion was 50 nested widgets in buttons. I doubt anyone will need more than 50 nested widgets inside a button.

The whole TW UI is about 65 in the Edit-Preview pane. And that's deep. So it would be possible to render the Whole TW UI inside a button, which imo is nonsense.

[pmario]

[Jermolene]

Hi @pmario

So how deep will you go? 1000 will crash TD

My comment wasn't about that, it was about how we handle nested buttons. Your implementation responds to the nested button by raising an error, and I was suggesting that we should instead render a "fake" button that is still functional. I'm not sure of the best way to proceed, but strongly suspect that we need to nail the underlying recursion problem before worrying too much about nested buttons breaking the HTML spec.

[pmario]

You are right. Let's focus on the recursion problem first.