Page MenuHomePhabricator

Extension:MsUpload causes the webpage to overflow past its boundaries on preview
Closed, ResolvedPublicBUG REPORT

Description

Short Summary

image.png (2,778×1,524 px, 81 KB)

As shown on the image above, the extension MsUpload has a bug wherein it causes the webpage to overflow when creating/editing a page and clicking "Show preview". Tested on a fresh MediaWiki Docker install and on Firefox.

Steps to replicate the issue:

  • Setup the following configuration on LocalSettings.php:
wfLoadExtension( 'WikiEditor' );
wfLoadExtension( 'CodeEditor' );
$wgDefaultUserOptions['usebetatoolbar'] = 1;

wfLoadExtension( 'MsUpload' );
  • Setup Vector 2022 as your user skin.
  • On Special:Preferences, make sure to uncheck the options "Show preview when starting to edit" and "Show preview without reloading the page", and check the option "Show preview before edit box".

image.png (1,776×1,176 px, 119 KB)

  • Create or edit an existing page. In the edit box, add some text that spans several viewport heights long. In this example we generate a 20-paragraph lorem ipsum passage from the Lorem Ipsum generator.
  • Click Show preview. Wait for the new page to load without scrolling the page whatsoever.
  • Once the new page has finished loading, scroll to the very bottom of the page.

What happens?:

image.png (2,778×1,524 px, 81 KB)

The webpage overflows.

What should have happened instead?:
There should not be any empty space under the MediaWiki footer.

Software version:

  • MediaWiki 1.45.1 (Official Docker image, image hash: 4c521816b8e0e6a4f56c349f4c34873876db6f354b8e1fea7b9cffa2b56020ec)
  • Installed the extensions WikiEditor & CodeEditor
  • Installed the extension MsUpload 14.2 (commit hash: 96b6127eed357804855abeb5e80f0d5267281b8f)
  • Firefox Browser 150.0.2 (Windows)

Event Timeline

The issue was found to have been caused by the div.moxie-shim-html5 element inside #msupload-container. This element was instantiated by the library dependency plupload, which sets the position of this inner element by calculating the absolute position of the upload button (#msupload-select) relative to the container (#msupload-container). How this position is calculated is based on the function getPos as detailed on plupload's source code. Previously, it was assumed that the upload button is contained within the container, but after the commit 51f0f6a73a4221616afc27134878223170023175 which moves the upload button to the main section of the wikieditor toolbar, this is no longer the case. As a result, getPos yields an erroneous number, and plupload then positions div.moxie-shim-html5 element far lower than it should have been.

image.png (2,776×1,545 px, 334 KB)

As was suggested by @TheWWRNerdGuy in a Discord chat, the Shim is completely unused and is safe to be moved around.

According to https://github.com/moxiecode/moxie/blob/1d816b071c7f6e29cc825cf97dcd089985106cbd/src/javascript/runtime/html5/file/FileInput.js#L81-L83, the shim is only used when the browser is not capable of summon_file_dialog. This is true for all modern browsers that MW supports according to https://github.com/moxiecode/moxie/blob/1d816b071c7f6e29cc825cf97dcd089985106cbd/src/javascript/runtime/html5/Runtime.js#L95-L101.

So something as simple as

#msupload-container .moxie-shim {
    top: unset !important;
}

Would fix the issue with no side effects. This is corroborated by the fact that 51f0f6a73a42 broke the shim's positioning but did not lead to loss of functionality.

Hi @Samwilson, since you seem to be the de facto maintainer of the extension and introduced the original commit, do you have any thoughts on whether the fix should restore the DOM structure or simply put a band-aid on the shim positioning?

The #msupload-container element that we're adding was originally done so that the loading gif resources/images/msu-loading.png could be displayed. I'm not sure this is actually needed any more, given that the code that loads the gif is loaded as part of the same ResourceLoader payload that it'd ostensibly be waiting for to load.

If we entirely remove the custom container, then the default one is used and that's a 0 x 0 pixel absolutely thing that doesn't interfere with any layout from what I can see.

Oh, no I was misremembering things: plupload is loaded separately and not in the RL module. But I still am not sure the loading indicator is needed, and if it is it should be done as an OOUI pending state on the upload button.

Change #1309834 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/MsUpload@master] Remove container and loading indicator

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

Change #1309834 merged by jenkins-bot:

[mediawiki/extensions/MsUpload@master] Remove container and loading indicator

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

Hi! I was able to reproduce the bug and verify the fix without apparent loss of functionality, so I just +2 the change and Jenkins merged it. Thanks everyone!

@Samwilson It's true that I'm no longer able to keep up with this extension, would you like +2 rights and become the "official" maintainer?

@Sophivorus I'd be happy to, sure! It's been useful having you review things, but if you're no longer using it yourself that's probably a bit annoying for you. I think it's used by a few wikis in the wild.

@Samwilson Awesome, thanks! I just listed you as the maintainer at the extension's documentation page (diff). To get +2 rights, apparently you have to request them yourself, see https://www.mediawiki.org/wiki/Gerrit/Privilege_policy#Requesting_Gerrit_privileges If you file a request, I'll support it. Cheers!

Samwilson claimed this task.

Thanks! I'm already a member of the wmf group, so already have +2.

@Ccxtwf I think this issue is fixed now, but shout if you find anything wrong!

Change #1312041 had a related patch set uploaded (by Peter Li; author: Samwilson):

[mediawiki/extensions/MsUpload@REL1_46] Remove container and loading indicator

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

Since they are on Miraheze I cherry-picked the branch for REL1_46, which Miraheze is expected to upgrade to in August.

Change #1312041 merged by jenkins-bot:

[mediawiki/extensions/MsUpload@REL1_46] Remove container and loading indicator

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