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

[BUG] ThemeTweak changes need to be validated #6818

Open
pmario opened this issue Jul 23, 2022 · 4 comments
Open

[BUG] ThemeTweak changes need to be validated #6818

pmario opened this issue Jul 23, 2022 · 4 comments

Comments

@pmario
Copy link
Contributor

pmario commented Jul 23, 2022

Describe the bug

It's way to easy to create a defunct wiki by changing 1 parameter in the ControlPanel area

no-new-tiddler-no-control-panel.zip

In the wiki above the New Tiddler button and the ControlPanel button don't work anymore.

The problem can be solved if ControlPanel : Appearence : ThemeTweeks : StoryWidth is set back to 770px. ... But most users would not be able to open the ControlPanel anymore.

This problem did "bite" me quite some time ago and I did fix it by export / import my content tiddler to a new wiki.

I think there has been a thread about a similar problem back in GG. And this quote reminded me about it: https://talk.tiddlywiki.org/t/nah-not-for-me-why/3989/62?u=pmario

Expected behavior

We should set up protections against "misconfigurations" that are "invisible" and close to impossible to find for ordinary users.

To Reproduce

No response

Screenshots

No response

TiddlyWiki Configuration

v5.2.2

Additional context

No response

@Jermolene
Copy link
Owner

Thanks @pmario, I agree that this is a big footgun, and should be fixed.

Interesting to think about the best way to approach a fix.

One approach would be to treat is an input validation problem. The user experience would be that any attempt to set a value outside an acceptable range would be rejected, with a visual indication of some kind. It's an attractive approach but I'm not sure that we've got a clean implementation. The challenge is that values in the tiddler store are unconstrained. Thus one might specify that an input control can only accept numbers from 1 to 100, but it's not clear how to handle the possibility that the bound tiddler field contains a value outside that range.

It's also problematic from a user experience perspective: imagine that we want to restrict the text size to be above 6pt, and the user wants to set the text size to 11pt. The user might approach it by first selecting the existing text in the control and then typing the digit "1". Our fancy validator promptly rejects the digit because it would cause an underflow, and the user is once again looking at the original text.

Another possibility is to not try to restrict the values that are entered into the text boxes, but to have validation logic within the stylesheets themselves so verify values before using them. Here's a simplified approach that ignores the fact that the CSS units are also included in $:/themes/tiddlywiki/vanilla/metrics/fontsize:

body.tc-body {
	font-size: {{{ [{$:/themes/tiddlywiki/vanilla/metrics/fontsize}compare:number:gteq[6]compare:number:lteq[100]else[X]]] }}};
...

If we took that approach it would be helpful if users could see the actual value that were being used, or at least see whether the actual value differs from the specified value.

With #6666 in place, we could define global functions for each of the theme tweak transformations, and then re-use them in the user interface as well as in the stylesheet.

@twMat
Copy link
Contributor

twMat commented Jan 21, 2023

I think the following is what you're talking about but sorry if it is not:

To see the problem:

  1. The "Sidebar layout" has to be "Fixed story, fluid sidebar"
  2. The "Storywidth" is changed from 770px to e.g 870px...
  3. ...but actually, to really see it, instead type 870px; outline: 3px dotted red

When considering alternatives to remedy this, I note that this styling prevents the problem: .tc-sidebar-scrollable { z-index:1 }. The sidebar currently has no z-index (and neither does .tc-story-river)

...but I think the whole "story width" is confusing to begin with, as I already raised here. In that thread, Jeremy explains that the width of the tiddlers are independent of the "story width" setting (this is not intuitive), but the AboveStory and BelowStory areas do use "story width" (but this is also not intuitive). If the story width value exists primarily for those two areas... then the "story width" value would be better if renamed.

...but the simplest, no nonsense, way would simply be to remove the setting UI from the Controlpanel Theme tweaks. Anyone advanced enough can track to find it , out or ask about it, just like for other hidden values.

By the way, the above thread links to what is probably the old GG post that Mario refers to.

@twMat
Copy link
Contributor

twMat commented Jan 21, 2023

When considering alternatives to remedy this, I note that this styling prevents the problem: .tc-sidebar-scrollable { z-index:1 }. The sidebar currently has no z-index (and neither does .tc-story-river)

Another option might be to switch places for those two elements in the DOM. No idea if that is appropriate from other perspectives.

@Alzacon
Copy link
Contributor

Alzacon commented Jan 22, 2023

The issue is having two positioned elements in the same stacking context.

In the current layout, in each change in a element tiddlywiki would check the size and position of the other element, if there is overlap then the position (or size) of the other element should be changed. But I think it does more complicated the layout.

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

No branches or pull requests

4 participants