CodeMirror: HTML attributes values hints not fully operable with a keyboard

[afercia]

To reproduce, add an HTML widget either in the Widgets screen or in the Customizer:

• start typing a HTML tag, for example "<i" for an input field:

• a list of hints appears and it is possible to use the down and up arrow keys to select one of the hints in the list, pressing Enter inserts the tag
• after the input, start typing the "type" attribute followed by a "="
• a list of possible values for the type attribute appears
• press the down arrow key to try to select one of the hints
• as soon as you release the down arrow key (on keyup), the selection moves back to the first hint thus making impossible to select any of the other hints

Worth noting:

• typing a letter after the "=" seems to make selection via arrow keys work again, for example type type=t and selection via keyboard works
• seems the attribute values hints don't work at all in the theme/plugin editors, where typing for example <input type= does nothing
• doesn't seem to happen in the Additional CSS in the Customizer, where all the hints appear to be operable with a keyboard

I'm really not sure if this is related to something specific to the HTML widget or if it's something that should be fixed upstream. Setting the "Widgets" component for now.

[afercia]

Agreed during today's bug-scrub this is something that should be fixed, hopefully by someone more familiar with CodeMirror and the HTML widget. Moving to future release. /cc @westonruter

[jorbin]

There is some hope that this is fixed upstream in CodeMirror which #48456 aims to upgrade. That's not a small task and will require an owner to see it through. Alternatively, it may make sense to move to something else entirely (see ​GB#10423) for the project. Also, this is not currently the default experience, it's only the experience if you install the classic widgets plugin.

This also seems to be inconsistent and could use more testing to reliably determine in what cases this takes place (is it just widgets? Is it the theme/plugin editor as well?)

[SirLouen]

Replying to jorbin:

This also seems to be inconsistent and could use more testing to reliably determine in what cases this takes place (is it just widgets? Is it the theme/plugin editor as well?)

Wrong workflow keyword, I think you meant, needs-test-info (aka we need more use-cases). Switching.

[westonruter]

It seems this isn't fixed with the latest v5 (#48456).

It is fixed in v6, however: ​https://codemirror.net/examples/autocompletion/

[siliconforks]

This bug actually looks incredibly similar to #63161.

[siliconforks]

First draft of a fix. Note that I've only tested it with the old version of CodeMirror - I haven't tested it with the new version. So don't merge this (yet).

[joedolson]

#63161 was marked as a duplicate.

[joedolson]

The duplicate of this ticket was milestoned for 7.0; milestoning this one.

[joedolson]

In response to @jorbin's earlier question: yes, this also happens in the theme/plugin editor, for PHP files. (Which is the duplicate issue I closed for this one.) I'm operating on the assumption that it's the same root issue.

It does *not* happen for CSS or JS.

Uses the work from @siliconforks, and adds a fix for PHP editors.

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

[westonruter]

@siliconforks Can you chime in on the PR to explain the reasoning behind the changes? How do they fix the problems?

Just to summarize the situation ... there are two (actually three) separate fixes required here:

1. The main problem in #63161 and #42822 is that codemirror.showHint() is getting called every time the up arrow or down arrow key is pressed. This means that the autocomplete list gets refreshed and the first item gets selected (again), so it is impossible to use the arrow keys to move to any other item - the first item just keeps getting re-selected. To fix this, a condition needs to be added checking which key was pressed. Note that this is already done for CSS and JavaScript autocompletion - the code checks whether isAlphaKey is true or event.key contains some other character which should trigger autocompletion. By adding the isAlphaKey condition to the PHP autocompletion code, we should fix the issue in #63161.

2. The problem for HTML autocompletion in #42822 is almost exactly the same thing - the code is not checking which key was pressed. Actually, it is slightly more complicated because the code for HTML autocompletion really has five separate conditions all joined together with the || operator. The first four conditions appear to be fine - they already check which key was pressed. The fifth condition (which handles HTML attribute value autocompletion) is the problem - it apparently never had a check for this. By adding a check for event.key === '=', we should fix the HTML autocompletion.

3. The final problem appears to be a separate bug not really related to the first two - it seems that CodeMirror has changed the structure of the token.state object. When this code was first written (in r41376), there was a token.state.htmlState property. At some point CodeMirror changed this so that this property no longer exists. Instead they added a token.state.curState.htmlState property and token.state.html.htmlState property, which appear to be the same thing.

@westonruter - are you testing with the old version of CodeMirror (the one that shipped with WordPress 6.9) or the newer version of CodeMirror (the one you're working on upgrading to in #48456)?

are you testing with the old version of CodeMirror (the one that shipped with WordPress 6.9) or the newer version of CodeMirror (the one you're working on upgrading to in #48456)?

@siliconforks I'm testing with the latest CodeMirror v5 as Core-48456 was committed (438132fd3658943b02fa382be8990bf8ad61c245) and is part of this PR's commit history.

For reference, in the latest CodeMirror, here's what a token contains when editing an HTML file at the moment that = is pressed when typing out <input type=:

{
    "start": 11,
    "end": 12,
    "string": "=",
    "type": null,
    "state": {
        "inTag": null,
        "localMode": null,
        "htmlState": {
            "indented": 0,
            "tagName": "input",
            "tagStart": 0,
            "context": null
        }
    }
}

On further investigation, it looks like I was wrong about CodeMirror changing the token structure in different versions - it's not actually a version issue, it's an issue with different modes:

• If you edit a .html file, you get a token.state.htmlState.tagName property.
• If you edit a .php file, you get a token.state.curState.htmlState.tagName property and a token.state.html.htmlState.tagName property.

So, the it seems that the code needs to handle both cases.

This should work:

shouldAutocomplete = (
                                                '<' === event.key ||
                                                ( '/' === event.key && 'tag' === token.type ) ||
                                                ( isAlphaKey && 'tag' === token.type ) ||
                                                ( isAlphaKey && 'attribute' === token.type ) ||
                                                /*
                                                 * In .html files the tag name is in token.state.htmlState.tagName;
                                                 * in .php files the tag name is in token.state.curState.htmlState.tagName.
                                                 */
                                                ( '=' === event.key && '=' === token.string && ( token.state.htmlState?.tagName || token.state.curState?.htmlState?.tagName ) )
                                        );

@siliconforks And what purpose does ( token.state.htmlState?.tagName || token.state.curState?.htmlState?.tagName ) serve? Just to verify that the token is in the context of a tag?

@siliconforks And what purpose does ( token.state.htmlState?.tagName || token.state.curState?.htmlState?.tagName ) serve? Just to verify that the token is in the context of a tag?

Yes, I'm assuming the original code (token.state.htmlState && token.state.htmlState.tagName) was there simply to avoid the possibility of autocompletion being triggered by an equals sign outside a tag. But it was also having the effect of preventing autocompletion _inside_ a tag in .php files (although it seemed to be working fine for .html files). That's why I'm suggesting changing it to ( token.state.htmlState?.tagName || token.state.curState?.htmlState?.tagName ).

That's why I'm suggesting changing it to ( token.state.htmlState?.tagName || token.state.curState?.htmlState?.tagName ).

OK, I applied that change in 8cd4589. Please test.

[westonruter]

In 61579:

Code Editor: Fix keyboard accessibility of autocompletion for HTML attribute values and PHP keywords/variables.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10807

Follow-up to [41376].

Props siliconforks, joedolson, westonruter, afercia, jorbin.
Fixes #42822.

Committed in r61579 (7965dec)

[westonruter]

Note: The event used to trigger the autocompletion was updated in [61588].