[wp-trac] [WordPress Trac] #57393: Unify checkbox handling and match wording with function on the User Profile screen (was: CodeMirror Features Disabled by wp_enqueue_code_editor Activation Conditions)

[WordPress Trac]

#57393: Unify checkbox handling and match wording with function on the User Profile
screen
----------------------------+---------------------
 Reporter:  mjdewitt        |       Owner:  (none)
     Type:  enhancement     |      Status:  new
 Priority:  normal          |   Milestone:  6.2
Component:  Administration  |     Version:  4.9
 Severity:  normal          |  Resolution:
 Keywords:  has-patch       |     Focuses:  ui
----------------------------+---------------------
Changes (by TobiasBg):

 * focuses:   => ui
 * component:  External Libraries => Administration
 * version:  6.1.1 => 4.9
 * milestone:  Awaiting Review => 6.2
 * keywords:  reporter-feedback => has-patch
 * type:  defect (bug) => enhancement


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

New description:

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

--

Comment:

 Replying to [comment:6 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/57393#comment:7>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform