Unify checkbox handling and match wording with function on the User Profile screen

[mjdewitt]

[The focus of this ticket shifted from discussing a potential functional bug to an improvement of wording and a more consistent behavior of checkboxes.
Using texts like "Disable" for toggled-on checkboxes can be confusing to users. For a better user experience, the toggled-on state should mean "enabled".]

Original ticket description:

CodeMirror highlighting, line numbers, code-folding, brace-matching and other customizations are disabled when highlighting is disabled. This is a real issue for plugins like code-snippets which rely on these features.

Here is the condition in question:

function wp_enqueue_code_editor( $args ) {
	if ( is_user_logged_in() && 'false' === wp_get_current_user()->syntax_highlighting ) {
		return false;
	}

I believe that the user-form-variable syntax_highlighting is probably misnamed as it should be something like disable_syntax_highlighting to reflect the choice being made.

As it is, when ticked (true), highlighting should be disabled. When un-ticked (false), highlighting should be enabled.

	if ( ! is_user_logged_in() || 'true' === wp_get_current_user()->syntax_highlighting ) { 
		return false;
	}

[TobiasBg]

Thanks for your report. However, this logic looks correct to me. If that syntax_highlighting setting is set to false, that wp_enqueue_code_editor() function exits early, *without* enqueuing/loading the code editor.

[mjdewitt]

@tobiaBg

This is one of the pitfalls of negatively phrased questions and the naming of the variable associated with it.

Disable syntax highlighting? => syntax_highlighting <!--this is how it is -->

Vs
(rewrite the question in a positive sense)
Enable syntax highlighting? => syntax_highlighting <!-- I think this is better -->
or
(rename the variable to match the question)
Disable syntax highlighting? => syntax_highlighting_disabled <!--the negative nature of the variable

Mixing negative questions and positive variables leads to confusion.

Mike

[TobiasBg]

Hi @mjdewitt,

thanks for providing feedback here!

I looked at this again, and still believe that the code logic is correct here.
The variable is called syntax_highlighting. If its value is true, the corresponding features are loaded (by using CodeMirror). Also, the logic in wp_enqueue_code_editor() is correct: If the value of syntax_highlighting is false, the function is left early (without loading the CodeMirror features).

Now, I do understand that the presentation of that checkbox on the User's Profile screen might not be ideal from a usability point of view (in terms that toggling a checkbox on turns a feature off). The functionality behind this is however working correctly: If the "Disable syntax highlighting when editing code" checkbox is toggled on, the internal value of the syntax_highlighting variable is set to false.

So, to improve usability, it might indeed be worthwhile to look into rephrasing the checkbox text here, to e.g. "Enable syntax highlighting when editing code" and to then reverse the checkbox states. The same should then however be done with the "Disable the visual editor when writing" checkbox right above. Given that the "Show Toolbar when viewing site" checkbox a bit further down is also using "positive wording" this would then indeed bring consistency to that screen a bit.

But again, the logic behind all this is correct from everything that I can see.

[TobiasBg]

For more reference, this was introduced for the "Disable the visual editor when writing" checkbox in #6403.
The idea was to rephrase "Use ..." to something more suitable, and "Allow ..." was actually considered. I think "Enable ..." would also work well here.

In https://core.trac.wordpress.org/changeset/41376#file12 this was then added (I guess to follow the same style) for "Disable syntax highlighting when editing code".

[mjdewitt]

Thank you for your time and consideration of this issue.

You are absolutely right that when syntax_highlighting is true, codemirror loads up with all the features. But can this really be considered as correct without considering the prompt which it reflects?

My issue here is that the way it is now is counter-intuitive or backwards: if disabled is checked, disabled is true meaning that codemirror does not load up features.

I use the plugin code-snippets a lot. A year ago or so, many of the codemirror features just stopped working. It was inconvenient, but hardly a show stopper. I finally got fed up with it's brokenness to look into why was this happening.

After I came to the conclusion that the problem was not in the plugin, I turned to WordPress. Much to my surprise, disabling syntax highlighting fixed codemirror's features. If you have the time, try it yourself.

Thanks again,

Mike

[TobiasBg]

Replying to mjdewitt:

My issue here is that the way it is now is counter-intuitive or backwards: if disabled is checked, disabled is true meaning that codemirror does not load up features.

That still sounds totally right to me. If the "Disable ..." checkbox is checked, the value of syntax_highlighting is false. If the value of syntax_highlighting is false, WordPress will not load the CodeMirror integration (as the function wp_enqueue_code_editor() will be left early).

If plugins like Code Snippet also interpret the syntax_highlighting value, it's on them to load relevant JavaScript files (like for CodeMirror) themselves.

Given that these checkboxes have been in WordPress for multiple years (see https://core.trac.wordpress.org/changeset/41376#file12), I believe that any functional issues would already have been detected.

That said, I do agree that the wording of the checkbox texts can be improved. I have added a patch that reverses the checkbox behavior and adjusts the wording of the checkbox texts, and brings some consistency to the handling of that code.
I think that this might indeed make sense here, and I'll move this suggestion for potential inclusion in WP 6.2, if a Core committer agrees, and I'll update the ticket title and description to reflect the suggested changes.

[mjdewitt]

I can see at the heart of this issue is semantics.

I'm not going to push this issue anymore as I don't think it's productive and I know the work-around for codemirror failures.

I will leave you with this:

Given that the code-snippets plugin only works when the box is checked, making syntax_highlighting false, and therefore disabled, codemirror is fully featured and works great.

According to the docs: ​https://wordpress.org/documentation/article/users-your-profile-screen/

"Syntax Highlighting – Checking this box Disable[s] syntax highlighting when editing code. "

I don't believe that the code agrees with this statement.

Thanks again for your consideration of this issue.

Mike

[TobiasBg]

Replying to mjdewitt:

According to the docs: ​https://wordpress.org/documentation/article/users-your-profile-screen/
"Syntax Highlighting – Checking this box Disable[s] syntax highlighting when editing code. "

This only refers to the WordPress Core "Plugin Editor" and "Theme Editor" screens - The WordPress Code docs can not speak for plugins like Code Snippets.

If the state of the checkbox has a (negative) influence on Code Snippets, that really something for their developers to look into, I think.

[costdev]

This ticket was discussed during the bug scrub. As this ticket still needs discussion and 6.2 Beta 1 is being released today, I'll move this to Future Release.

In addition, the patch may need a unit test for edit_user(). Adding needs-unit-tests.

Additional props: @mukesh27

Trac ticket: https://core.trac.wordpress.org/ticket/57393