Page MenuHomePhabricator

jquery.textSelection behavior changed when given `undefined` in command options
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue
A code used since 2011 is broken on deWP since today, distribution of recent MW version.

What happens?:
When on source text editing something is marked and string overwriting is requested, undefined is additionally inserted into wikitext after selection end.

What should have happened instead?:
Working as always.

Software version
Independent of browser family.

  • MW of today.

Other information
rMW2af1c3c901a6117fe062e1fd88c0146cffa1481d shows on jquery.textSelection diff that the string $.extend( { has been simply replaced by Object.assign( {

  • T368029 demonstrated already that this is not a good idea. This is not the identical functionality, and must not be flipped just from one name to another.
  • I did not open all rMW2af1c3c901a6117fe062e1fd88c0146cffa1481d but I could imagine that the same bug has been injected in many many codes.

Event Timeline

Esanders triaged this task as Unbreak Now! priority.

When on source text editing something is marked and string overwriting is requested, undefined is additionally inserted into wikitext after selection end.

Can you give more detailed steps, I can't reproduce?

Looking at the code I can't see where this might be going wrong either. In T368029, there was an issue with the first argument being undefined, but in this case the first argument is a literal object, and only the second argument can be undefined, which isn't a problem.

> Object.assign( {a: 1}, undefined )
< {a: 1}

I did not open all rMW2af1c3c901a6117fe062e1fd88c0146cffa1481d but I could imagine that the same bug has been injected in many many codes.

I re-audited that patch, and in every other case with either pass an object literal as the first argument, or a variable that is guaranteed to be an object.

The autofixer will be amended to be more conservative: https://github.com/wikimedia/eslint-plugin-no-jquery/pull/330

The error occurred first immediately after distribution, but the code is of 2010.

  • Here is the executed code unchanged since 2020 in this particular usage.
  • Used more than thousand times every day for every source editing on dewiki.
  • You took me already hours to track until this point, time I do not have and resources missing in other issues.

I found your discussion on de.wp which gives actual steps to reproduce: https://de.wikipedia.org/wiki/Wikipedia:Fragen_zur_Wikipedia#c-PerfektesChaos-20240620175000-BUG:_Wort_„undefined“_erscheint_bei_der_Quelltextbearbeitung

  1. Open the old wikitext editor
  2. Type and select some text
  3. While holding the Ctrl key, click one of the special characters below the editor

image.png (2×3 px, 660 KB)

Result: &nbsp;undefined
Expected: &nbsp;
(When not holding Ctrl, the expected result is &nbsp;asdf)

To be honest, I doubt this affects anyone except you, since that discussion also says (if I understand correctly) that this hidden feature was never documented correctly. But it does seem like a bug.

I wonder what's the actual cause. I doubt it's the jquery.textSelection changes.

Minimal reproduction: $( '#wpTextbox1' ).textSelection( 'encapsulateSelection', { pre: 'A', peri: undefined, replace: true } )

peri: undefined was previously treated like peri: ''. Now it is treated like peri: 'undefined'.

This is actually a change caused by the patch that you identified, causing by this difference in behavior:

Object.assign( { a: 1 }, { a: undefined } )
= { a: undefined }

$.extend( { a: 1 }, { a: undefined } )
= { a: 1 }
matmarex renamed this task from jquery.textSelection broken since today to jquery.textSelection behavior changed when given `undefined` in command options.Fri, Jun 21, 1:08 PM

The documentation bug on dewiki was just misspelling of the key, but the functionality is known to other people even those who never read such fine manuals.

The error occurred first immediately after MW distribution to dewiki, and it worked since 2010 already.

My code is following a documentation of .textSelection(encapsulateSelection) maintained since about 2010, meanwhile deleted, in mw:ResourceLoader/Core modules now pointing to doc.wikimedia.org which tells 404.

Therefore no proof and no valid documentation is available, what the contract on using textSelection did contain and what has been promised for a decade.

I swear I did follow the documentation always and in all details.

I am on summer vacation, and I do not have access to my development environment.

but the functionality is known to other people

I think @matmarex's point is that passing a value of undefined is not documented as supported behaviour in jquery.textSelection.js.

Change #1048483 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/core@master] jQery.textSelection: Option values of `undefined` revert to defaults

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

It is common practice to use $.extend() for merging defaults and options in a cascading style.

  • Code will provide defaults, subsequently combined with siteuserpage modification.
  • And yes, it is common practice to ignore undefined components since they are – deliberately undefined for this stage!
  • Whether this is stated explicitly in any documentation or not, it is common understanding for decades. Undefined components are regarded as not provided. No difference was made ever between undefined or not occurring components; both have the same meaning. Even false may be regarded as refused assignment in certain cases.

Look at this application:

harvesting = { apples: getApples(),
               pears:  getPears() };

Since I do not own any apple tree, undefined will be returned rather than 0 apples. It would be difficult while not impossible to delete such a component entirely from the object to obey the new WMF rules of 2024.

The implementation provides default values:

fruits = { apples: 0,
           pears:  0 };

The defaults will be mixed in current request.

That is also common practice in building mw.API requests which I see frequently occurring in the recent project wide change.

Since the documentation for jquery.textSelection is lost or at least available for mediawiki.org sysops only, nobody can prove what the explicit statement has been since 2010, and what has been demanded explicitly or which types were asked and that undefined has been forbidden.

  • However, in all library functions undefined values were always treated as if not provided, at least since the 2010 new JS code age.

I am really looking forward to read the new documentation for .textSelection( "encapsulateSelection", request ) with all details about components and behaviour of missing request components.

My code is following a documentation of .textSelection(encapsulateSelection) maintained since about 2010, meanwhile deleted, in mw:ResourceLoader/Core modules now pointing to doc.wikimedia.org which tells 404.

Therefore no proof and no valid documentation is available, what the contract on using textSelection did contain and what has been promised for a decade.

I swear I did follow the documentation always and in all details.

I fixed the link there. You can find the current documentation at https://doc.wikimedia.org/mediawiki-core/master/js/module-jquery.textSelection.html#.encapsulateSelection. You can also find older documentation at doc.wikimedia.org if you look; the oldest version that has this module documented (dated around 2018) is here: https://doc.wikimedia.org/mediawiki-core/1.31.0/js/#!/api/jQuery.plugin.textSelection.

Both of them document that peri is a string, optional. This means that it does not expect undefined as a value, otherwise it would be documented as string|undefined. The parameter should be a string or unset.

You brought up mw.Api, we can compare to its documentation, which states: "Parameter values set to false or undefined will be omitted from the request" (https://doc.wikimedia.org/mediawiki-core/master/js/mw.Api.html / https://doc.wikimedia.org/mediawiki-core/1.31.0/js/#!/api/mw.Api).

Whether this is stated explicitly in any documentation or not, it is common understanding for decades. Undefined components are regarded as not provided. No difference was made ever between undefined or not occurring components; both have the same meaning. Even false may be regarded as refused assignment in certain cases.

I don't think that's true.

JavaScript distinguishes variables and properties that are set to undefined from those that are unset. Consider these examples:

x = {};
console.log( 'a' in x ); // => false
x.a = 1;
console.log( 'a' in x ); // => true
x.a = undefined;
console.log( 'a' in x ); // => true
delete x.a;
console.log( 'a' in x ); // => false
console.log( a ); // => Uncaught ReferenceError: a is not defined
a = 1;
console.log( a ); // => 1
a = undefined;
console.log( a ); // => undefined
delete a;
console.log( a ); // => Uncaught ReferenceError: a is not defined

There are some functions that ignore undefined, but they're usually clearly documented, see e.g. the mw.Api example above. $.extend also documents this behavior: https://api.jquery.com/jQuery.extend/ "Undefined properties are not copied".


But that's academic discussion. Even though the behavior for this case was not specified, and probably no one ever consciously decided whether it should work, and IMO it should not have worked :), it has worked before and now doesn't work, and that's bad. @Esanders's patch will make it work again.

Hopefully we'll remember about this edge case in the future.

Change #1048483 merged by jenkins-bot:

[mediawiki/core@master] jQuery.textSelection: Option values of `undefined` revert to defaults

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

[...]

Object.assign( { a: 1 }, { a: undefined } )
= { a: undefined }

$.extend( { a: 1 }, { a: undefined } )
= { a: 1 }

Oh wow, thank you for pointing this difference out! 🙏 I was not aware of that the behavior also differed in that regard.

The fix will be deployed to Wikimedia wikis this week on the usual schedule.