Make WordPress Core

Opened 17 months ago

Last modified 16 months ago

#57393 new enhancement

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

Reported by: mjdewitt's profile mjdewitt Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9
Component: Administration Keywords: has-patch needs-unit-tests
Focuses: ui Cc:

Description (last modified by TobiasBg)

[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;
	}

Attachments (1)

57393.patch (3.0 KB) - added by TobiasBg 16 months ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
17 months ago

  • Description modified (diff)

#2 @TobiasBg
17 months ago

  • Keywords reporter-feedback added

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.

#3 @mjdewitt
16 months ago

@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


#4 @TobiasBg
16 months ago

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.

#5 @TobiasBg
16 months ago

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".

@TobiasBg
16 months ago

#6 follow-up: @mjdewitt
16 months ago

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

#7 in reply to: ↑ 6 @TobiasBg
16 months ago

  • Component changed from External Libraries to Administration
  • Description modified (diff)
  • Focuses ui added
  • Keywords has-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 6.2
  • Summary changed from CodeMirror Features Disabled by wp_enqueue_code_editor Activation Conditions to Unify checkbox handling and match wording with function on the User Profile screen
  • Type changed from defect (bug) to enhancement
  • Version changed from 6.1.1 to 4.9

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.

#8 follow-up: @mjdewitt
16 months ago

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

#9 in reply to: ↑ 8 @TobiasBg
16 months ago

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.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#11 @costdev
16 months ago

  • Milestone changed from 6.2 to Future Release

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

#12 @costdev
16 months ago

  • Keywords needs-unit-tests added
Note: See TracTickets for help on using tickets.