Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IE doesn't show "Success: your edit has been sent to moderation" message #9

Closed
tansaku opened this issue May 10, 2017 · 45 comments

Comments

Projects
None yet
3 participants
@tansaku
Copy link

commented May 10, 2017

We've added both the moderation and the visual editor extensions to our mediawiki installation and they are both working, however it seems like the moderation notices don't appear when the user is using the text editor.

Has anyone encountered similar before?

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 10, 2017

This is a delivery problem (e.g. emails ending up in "Spam" folder). It is not related to Moderation.

The code that sends notification emails is completely independent from the frontend.
It works exactly the same for VisualEditor and for the usual "Edit source" mode.

If it works with VisualEditor, then it works.

@edwardspec edwardspec added the invalid label May 10, 2017

@edwardspec edwardspec closed this May 10, 2017

@tansaku

This comment has been minimized.

Copy link
Author

commented May 11, 2017

I'm not talking about email notifications - I'm talking about the popups that appear on the html pages when you save a page and the user is being moderated

@tansaku

This comment has been minimized.

Copy link
Author

commented May 11, 2017

@edwardspec i.e. these:

popup

@tansaku

This comment has been minimized.

Copy link
Author

commented May 11, 2017

we are finding they work find with the visual editor, but don't seem to get displayed when using the default editor or wikieditor ...

@edwardspec edwardspec reopened this May 11, 2017

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 11, 2017

I just re-tested them in MediaWiki 1.27/1.28 with Firefox/Chrome.
This message is shown after I save the edit in default editor ("Edit source" mode).

Are they any error messages in the browser console?
When you save an edit and are redirected back to the article, is there "modqueued=1" in the URL?

@edwardspec edwardspec added not reproduced and removed invalid labels May 11, 2017

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

it just worked fine for me creating a test page in the basic editor our staging instance:

message displaying correctly

but there was one error in the console, and I notice that I was auto-logged out - see #11

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

I re logged in and was pleased to see the moderation message still there and updated relating to having signed in:

updated moderation message

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

It also worked fine with the wikieditor on this instance. Where I'm seeing the problem is on our main site which has the visual editor and SSL enabled. I see the moderation popup with the visual editor there.

moderation popup with visual editor on SSL enabled site

it also worked in the wikieditor ... the issues I saw testing with users last week were on IE and safari ...

I just tested on IE11 on saucelabs using the wikieditor and saw the problem replicated:

IE11 failing to display moderation notice

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

the console was empty of errors in IE:

console empty

but the URL does indeed redirect to a URL with modqueued=1 in it ...

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

it worked fine on my local version of Safari ...

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

The error in the console (404 Not found) is irrelevant (it says "Test Page2 doesn't exist yet", which is true).

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

IE is notorious for having all kinds of problems due to not following standards.
I am surprised anyone still uses it in 2017.

I don't know what its problems are, nor can I investigate (as a Linux user).
You need to find someone familiar with IE to troubleshoot this.

@edwardspec edwardspec changed the title moderation notice not showing up when using text editor (appears when we use visual editor) IE doesn't show "Success: your edit has been sent to moderation" message May 15, 2017

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

yes, ironically thousands of people are still required to use IE dy their employer - in the case of our wiki lots of healthcare professionals and local authority employees in the UK.

I'm on OSX so can't investigate directly there, although saucelabs offers free sessions for open source projects to test on multiple browsers:

https://saucelabs.com

I've also got a windows rig to hand which I just fired up and replicated the issue on both our live and staging instance. Inspecting the html on my windows box, IE11 just doesn't have the postedit-containerdiv that would show the moderation message.

Is that being added to the page by javascript that might be failing?

As I say this is a fairly serious issues for thousands of our users so I am motivated in getting to the bottom of it. Your help in understanding how the moderation message is added to the page is very much appreciated it - perhaps I will be able to fix it ...

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

I tested this with saucelabs and (most likely) found the cause.

There WAS a console error in IE, about the line
mw.moderation.notifyQueued = function( options = [] ) {

Apparently IE doesn't support default values of function attributes ("= []" above).

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

Should be fixed in 1.0.20 (c0e2457).
Please confirm it works.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

I have pulled your latest code onto staging and am now seeing moderation popups in IE - many thanks!

@edwardspec edwardspec closed this May 15, 2017

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

although I'm still having the same trouble in production ... where I'm getting the following error

SCRIPT1004: Expected ';'
File; load.php, Line: 2, Column 329

production has visual editor and ssl enabled ... is there a test suite I can run ...?

I need to get a matching staging and production environments ...

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

Reproduced the problem (when opening VisualEditor editing form), investigating.

At the moment, there is no JavaScript testsuite.
I agree that making some Selenium tests would be a good idea.

@edwardspec edwardspec reopened this May 15, 2017

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

thanks @edwardspec - I was just chatting with the mediawiki-visualeditor team about it - they said:

[18:34] <+MatmaRex> tansaku: that error message doesn't look like it's getting stuck due to slow network or whatever, it looks like a syntax error in the script served by MediaWiki… as if it was corrupted it on the way or something? (or as if it was actually using a syntax that IE11 doesn't accept)
[18:38] <+MatmaRex> tansaku: if you click on the "load.php (2,329)" in the error message, it will take you to the script that is failing. specifically, IE11 doesn't like this syntax: "for(pair of sendBody.entries())" in the module 'ext.moderation.ajaxhook' (this is part of some other extension you have installed, not VisualEditor)
[18:38] <+MatmaRex> tansaku: for…of is a relatively new syntax and it won't work outside of most recent browsers. you should file a bug with whatever this extension is, asking them to support IE11.

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

Indeed, IE11 doesn't support for ( A of B ) syntax.
Will rewrite this shortly.

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

Got an idea.
What we currently get from FormData, can be obtained by wrapping around mw.Api.prototype.api().
This should work in IE.

Investigating.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 15, 2017

great stuff :-)

edwardspec added a commit that referenced this issue May 16, 2017

(compat) Make ajaxhook work in IE: track parameters in mw.Api.ajax(),…
… not in XMLHttpRequest.send()

This solves some of the IE-related problems of #9.
@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

Fixed the FormData issue, but there is another problem:
in IE11, Object.defineProperty() doesn't make XMLHttpRequest.responseText writable.

Investigating further.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 16, 2017

@edwardspec anything I can do to help?

edwardspec added a commit that referenced this issue May 16, 2017

(ajaxhook) Major cleanup of ajaxhook logic: use dataFilter callback
This should solve IE-related problems of #9.
In IE11, it was impossible to make built-in fields writable
via Object.defineProperty, so we couldn't overwrite xhr.responseText.

Because MediaWiki extensions use "mediawiki.Api" module, we can wrap
around its ajax() method and rewrite the response on this level.
This should be browser-independent.
@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

Should be fixed in 1.0.23.
Tested in IE11 + 1.27, both VisualEditor and MobileFrontend seem to work.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

hi @edwardspec - thanks for this - the moderation messages are now showing up in our production system in ie11.0.96 which is a big step forward.

Visual editor complete's it's loading cycle, but then it encounters a couple of errors in the console, and the editor appears, but everything is grayed out.

The two errors are a SyntaxError and an XML5632 error saying that only one root element is allowed - I've put the full details in this gist:

https://gist.github.com/tansaku/71994685d3b8da492cff598173eb5f4c

maybe this is a visual editor specific bug ... I'm going to create a fresh instance to help isolate what's going on. Visual Editing still works on my mac ...

Great work getting the moderation messages working for this set up!

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

I just tested the same IE on https://hlptest.miraheze.org/ where visual editor is enabled and it works fine, suggesting that this latest VE bug is still somehow related to our configuration - it might be something other than the moderation package, but perhaps the errors in the gist above make sense?

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

The exception happens in Extension:Cite.
The following may help them: https://phabricator.wikimedia.org/T138356 (this is how the same problem was fixed in Extension:Flow)

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

Interesting - I just set up a fresh instance with VisualEditor and Cite, and it runs fine on IE11 - so I guess it's some interaction between VisualEditor, Cite, and Moderation that causes the problem no?

Or are you saying that Extension:Cite needs to change?

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

I note that disabling the CiteExtension in our production instance does not fix the problem

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

The error is definitely in Cite.
Basically, the problem is:

  1. VisualEditor supports "plugins" - they give it a piece of HTML.
  2. Extension:Cite contains one of those plugins.
  3. This plugin returns HTML like "<p>something</p><p>else</p>"
  4. IE discards this HTML as incorrect because correct HTML must have one top element, e.g. "<div><p>something</p><p>else</p></div>".
  5. The solution is to wrap HTML returned by Extension:Cite's plugin into one tag, like here: https://gerrit.wikimedia.org/r/#/c/307820/1/modules/flow/ui/widgets/editor/editors/mw.flow.ui.VisualEditorWidget.js
  6. This error is in Extension:Cite and it should be fixed there.

You can't still have an error in "ext.cite.visualEditor" after disabling Cite. It's a cache issue.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

thanks, but so why does a fresh instance with Cite and VisualEditor work fine in IE11? Or on Wikipedia where both Cite and VisualEditor are working (I just tested on IE11)?

If the problem is with Cite and VE combination and is unrelated to the Moderation extension, then why isn't it encountered in those other instances where Cite and VisualEditor are used together ...?

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

@MatmaRex

This comment has been minimized.

Copy link

commented May 17, 2017

This isn't well documented (although technically it is mentioned here: https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/mw.loader-event-resourceloader_exception), but Exception in load-callback in module ext.cite.visualEditor doesn't mean exception is in the 'ext.cite.visualEditor' code – it's an exception in the calling code, for example if you did:

mw.loader.using( 'ext.cite.visualEditor', function () {
    // Something here throws an exception
} );
@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

I also note (from @MatmaRex's input) that the problem is not ocurring on the main page, but on the Test_Page where my user has edits outstanding ... (i.e. in the moderation queue)

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

I can confirm that the error only occurs when there are edits outstanding on the page

@tansaku

This comment has been minimized.

Copy link
Author

commented May 17, 2017

I also tried turning on halting on all exceptions as suggested by MatmaRex:

and saw various exceptions (even when the VE editor loaded fine)

the following all raised syntax errors:

https://gist.github.com/tansaku/77f4186063a95352a81bca9e05a812ec

I think it's the last line in the above that is the error that blocks the loading of the visual editor when moderated edits are pending ... as @edwardspec it may well be related to the text that Cite returns, but Cite seems able to return that text for the VisualEditor in the main wikipedia site (and others) on IE11 without problems, so it seems like there could be some interaction with the moderation plugin that's causing this ... at least we won't see this problem on VE on wikipedia as they don't have the moderation plugin.

I'll keep experimenting tomorrow ...

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

I can confirm that the error only occurs when there are edits outstanding on the page

This situation (called "preloading") is handled by the code in [modules/visualeditor/preload.ve.js].

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

Reproduced the problem, investigating.

edwardspec added a commit that referenced this issue May 17, 2017

(compat) Fix "Only one root element is allowed" error in IE11
When preloading into VisualEditor, we supply HTML output of "parsefragment" action.
This HTML is not a complete document (not wrapped in a top-level tag like <body>),
and IE11 may consider it invalid ("Only one root element is allowed" exception
on HTML like "<span>word1</span> <span>word2</span>"). See issue #9.

Therefore we wrap HTML from "parsefragment" in a no-op tag <body>.
We can't use <div>, as VisualEditor would consider it an extra paragraph.
@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

Should be fixed in 1.0.25.
Please confirm it works.

@tansaku

This comment has been minimized.

Copy link
Author

commented May 18, 2017

many thanks @edwardspec - that's fixed it - really appreciate all your hard work.

would love to work towards some acceptance tests to keep all this in place - but super hard to config that to run the tests in IE11 without paying saucelabs or something ...

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 18, 2017

Doesn't IE11 support webdriver?

@edwardspec

This comment has been minimized.

Copy link
Owner

commented May 18, 2017

Closing this as completed.
I opened #13 for the task of creating a JavaScript testsuite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.