Page MenuHomePhabricator

CharInsert markup inserted in wrong place when syntax highlight is on
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
The markup is inserted in the edit summary

What should have happened instead?:
The markup should be inserted in the edit article edit area where the cursor was

**Note:
The only way to restore temporary the normal behavior, is to click on one of the top button (e.g. bold) before clicking on the desired markup

Event Timeline

  • Click on any wikimarkup

Where / how? Under Per inserire del testo tra questi segni selezionalo e clicca sul link: ?

Can you reproduce the very same problem when you perform your steps on https://it.wikipedia.org/w/index.php?title=Regione_dell%27Araucan%C3%ADa&action=edit&safemode=true ?

@Aklapper, yes, in Wikipedia is "Under "Per inserire del testo tra questi segni selezionalo e clicca sul link:", while in Wikivoyage is next to "Inserisci:" and "Wiki markup:".

And yes, I can reproduce it also in safemode, in both wiki-projects.

I cannot reproduce using Firefox 124 on Fedora 39, thus closing. Feel free to bring this up on https://meta.wikimedia.org/wiki/Tech as this does not seem to be a problem in software code in Wikimedia code repositories but likely the web browser with local add-ons or extensions - thanks!

@Aklapper I've tried Firefox latest version on Windows and:

  • If I'm logged I have the same issue
  • if I'm NOT logger I don't have it

I suppose there's an issue with one gadget that suddenly create this issue.
In this case, which would be the best place for this support request?

@Aklapper on Locating_broken_scripts is written that if the issue persist on safemode=1 I'm supposed to open a ticket here :-)

This a part, I would ask you to reperform the test because I've noticed that the issue occurs when I'm logged only when "syntax highlight" is on. When it's off, all works correctly.
Please let me know if in this way you can reproduce the issue.

Andyrom75 renamed this task from Clickable wikimarkup is not properly inserted on Italian Wikivoyage and Wikipedia to Clickable wikimarkup is not properly inserted on Italian Wikivoyage and Wikipedia when syntax highlight is on.Apr 3 2024, 12:49 PM
Andyrom75 updated the task description. (Show Details)

In the meanwhile I reopen the ticket to allow someone from SyntaxHighlight team to give a look

Charinsert has a function that determines the last focused text input.

		$currentFocused = $( '#wpTextbox1' );
		// Apply to dynamically created textboxes as well as normal ones
		$( document ).on( 'focus', 'textarea, input:text', function () {

Clearly this does not get triggered when someone refocuses the syntaxhighlight area.

Aklapper renamed this task from Clickable wikimarkup is not properly inserted on Italian Wikivoyage and Wikipedia when syntax highlight is on to CharInsert markup inserted in wrong place when syntax highlight is on.Apr 3 2024, 9:32 PM

Charinsert has a function that determines the last focused text input.

		$currentFocused = $( '#wpTextbox1' );
		// Apply to dynamically created textboxes as well as normal ones
		$( document ).on( 'focus', 'textarea, input:text', function () {

Clearly this does not get triggered when someone refocuses the syntaxhighlight area.

@TheDJ, this buggy code is on server side o it's in a local gadget?
I suppose the first one, but I ask to avoid misunderstanding.

@TheDJ, this buggy code is on server side o it's in a local gadget?
I suppose the first one, but I ask to avoid misunderstanding.

CharInsert is an extension providing this JS.
https://github.com/wikimedia/mediawiki-extensions-CharInsert/blob/master/modules/ext.charinsert.js#L30

Ok, in this case I'm not able to support.
Thanks for your clarification.

@Andyrom75 is there a chance you could take a video recording of the issue, so I better understand? Thank you

@JWheeler-WMF , I'm not skilled on video, so I've tried to do "something close to it" :-)

image.png (617×900 px, 100 KB)

So according to what I wrote initially, you can follow the numbers in the attached picture:

  1. Be sure that syntax highlight is on
  2. Go to Edit summary and write something (e.g. "xxx")
  3. Click anywhere on article edit area to focus it (in this example I've highligthed it)
  4. Click on any wikimarkup (in this example the intention is to click on "[[]]")

When syntax highlight is off, all works correctly and in the attached example, "regione" become "[[regione]]".
When syntax highlight is on, "regione" remains "regione", and "xxx" become "xxx[[]]".

I hope this picture + the explanation is helpful.
If not, please let me know.

JWheeler-WMF raised the priority of this task from Medium to High.Apr 5 2024, 4:03 PM
JWheeler-WMF added a subscriber: MusikAnimal.

Super helpful, thank you. We'll take a look next week when @MusikAnimal is back.

Change #1018740 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] Use jQuery.trigger instead of triggerHandler for focus/blur events

https://gerrit.wikimedia.org/r/1018740

MusikAnimal added a subscriber: Bhsd.

Looks like using trigger() instead of triggerHandler() fixes it. This is sort of a follow-up to T197632, so pinging @Bhsd to see if they think there are any issues in switching to trigger().

This is sort of a follow-up to T197632, so pinging @Bhsd to see if they think there are any issues in switching to trigger().

My understanding is that trigger( 'focus' ) sets focus on the hidden textarea element. However, it might be a problem only for IE.

This is sort of a follow-up to T197632, so pinging @Bhsd to see if they think there are any issues in switching to trigger().

My understanding is that trigger( 'focus' ) sets focus on the hidden textarea element. However, it might be a problem only for IE.

I see you already +1'd the patch, but to confirm: I think that's what we want, right? Clients such as CharInsert only work off the textarea, so the event needs to be delegated there. IE compatibility should no longer be an issue as we no longer support it for JS features :)

My naive understanding is unlike triggerHandler(), trigger() will:

  • Not bypass default behaviour of an event
  • Operate on all matched elements
  • Return value is different (not relevant here)

It seems bypassing default behaviour is the questionable bit. Indeed, it's odd to set focus on a hidden textarea, but focus is still maintained on the CodeMirror contenteditable. My guess is the reason why CharInsert doesn't work with CodeMirror and triggerHandler() is that it's attaching the listener to the document instead of the textarea directly. That could be fixed in CharInsert, but it stands to reason other clients out there are listening to these events in a similar way.

This messing with the focus events is sloppy though (always has been). You are basically moving the cursor... IF this is a problem for CM6, an alternative is to add a named event and have charinsert listen to that perhaps.

How about manually dispatch a focus/blur event then? This will not reset the focused element.

I can't even say for sure the cursor is being moved. From a user standpoint, it seems it's not, or if it is, it happens too quickly to notice. I didn't code anything to keep focus on CodeMirror -- it's doing that on its own which leads me to believe we're not breaking any rules, per se.

How about manually dispatch a focus/blur event then? This will not reset the focused element.

How might I do that? What you're saying sounds like what triggerFocus() is supposed to do.


We can also change CharInsert to do the following:

$( 'textarea, input:text' ).on( 'focus', function () {
	$currentFocused = $( this );
} );

This puts the listener directly on the textarea, so triggerFocus will work. The current use of a delegated event handler (ala $( document ).on( 'focus', 'textarea, input:text', …) seemingly doesn't come with any advantage, since we're scanning the whole document anyway. This is opposed to say $( '.mw-body-content' ).on( 'focus', 'textarea, input:text' ) which would isolate the DOM scanning to .mw-body-content.

Right?

How about manually dispatch a focus/blur event then? This will not reset the focused element.

How might I do that? What you're saying sounds like what triggerFocus() is supposed to do.

For instance,

this.$textarea[0].dispatchEvent(new Event('focus'));

This prevents the focus() method to be executed on the hidden textarea, unlike $.event.trigger (ref).

The current use of a delegated event handler (ala $( document ).on( 'focus', 'textarea, input:text', …) seemingly doesn't come with any advantage, since we're scanning the whole document anyway.

I guess the purpose of event delegation here is to handle any textarea element dynamically created later. This is common for some user scripts.

For instance,

this.$textarea[0].dispatchEvent(new Event('focus'));

Brilliant! Vanilla JS for the win :)

I guess the purpose of event delegation here is to handle any textarea element dynamically created later. This is common for some user scripts.

Bah, that should have been obvious. Thanks for clarifying!

JWheeler-WMF lowered the priority of this task from High to Low.May 17 2024, 5:01 PM

@JWheeler-WMF, since this bug affect ALL the wikis, are you sure that its priority shall be classified as "low"?

JWheeler-WMF raised the priority of this task from Low to High.Jun 5 2024, 2:18 PM

@Andyrom75 - this is a meaningful bug. I was using priority statuses to better visualize tasks on our phab board. As such, I've updated the priority however it may linger for some time as we have other priorities relating to the Wishlist.

The solution has been discussed and submitted to Gerrit for 2 months, and we just need someone to review it (+2).

Change #1018740 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] Use dispatchEvent instead of jQuery triggerHandler for focus/blur events

https://gerrit.wikimedia.org/r/1018740

The solution has been discussed and submitted to Gerrit for 2 months, and we just need someone to review it (+2).

And now we don't need to worry about these things as Bhsd has +2! :)

With the patch now merged, it's slated to go out on next week's train. I'll wait until it's live to close the task.

Bhsd moved this task from Bugs to Done on the MediaWiki-extensions-CodeMirror board.