Page MenuHomePhabricator

ProofreadPage conflict in text box size
Closed, ResolvedPublic3 Story Points

Description

CodeMirror forces the edit box to be full width, pushing the other page fields and the proofreading image down:

Reported at https://www.mediawiki.org/wiki/Topic:Tylh06eprgqv6nya

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 21 2017, 10:52 PM
kaldari triaged this task as Normal priority.Oct 3 2017, 10:40 PM
kaldari set the point value for this task to 3.
Niharika claimed this task.Oct 12 2017, 9:24 PM
Niharika moved this task from Ready to In Development on the Community-Tech-Sprint board.

Change 389646 had a related patch set uploaded (by Niharika29; owner: Niharika Kohli):
[mediawiki/extensions/CodeMirror@master] Make textbox height flexible with CodeMirror

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

I'm hoping the above patch fixes this but since I don't have ProofreadPage setup (I tried), it'd be awesome if someone can help test this. Maybe on commtech wiki?

Change 389646 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] Make textbox height flexible with CodeMirror

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

Testing in Firefox, the edit box is correctly on the left and syntax highlighting works:

Actually, I spoke too soon. :-( It doesn't resize when switching between horizontal and vertical proofreading (i.e. when the edit box is at the left, vs. at the top). I can resize by height in both views, but not by width.

Niharika added a comment.EditedNov 7 2017, 7:24 PM

Actually, I spoke too soon. :-( It doesn't resize when switching between horizontal and vertical proofreading (i.e. when the edit box is at the left, vs. at the top). I can resize by height in both views, but not by width.

I'm not sure what you mean. So when the edit box is on the top why do we want to resize the box by width? It doesn't ket me resize by width on prod without CodeMirror either. Maybe screenshots will help?

Can you also try with WikiEditor (instead of the classic editor)? Thank you so much!

This bug may not be related to CodeMirror after all, but is the result of user script interaction (i.e. it doesn't appear with safemode=1).

But it does still happen...

Can you reproduce it here?:
https://en.wikisource.org/wiki/Page:Gissing_-_The_Nether_World,_vol._III,_1889.djvu/182?action=edit&debug=1

It's giving this error:

TypeError: fn[command] is undefined[Learn More]  ext.CodeMirror.js:267:3

This bug may not be related to CodeMirror after all, but is the result of user script interaction (i.e. it doesn't appear with safemode=1).
But it does still happen...
Can you reproduce it here?:
https://en.wikisource.org/wiki/Page:Gissing_-_The_Nether_World,_vol._III,_1889.djvu/182?action=edit&debug=1
It's giving this error:

TypeError: fn[command] is undefined[Learn More]  ext.CodeMirror.js:267:3

Hmm, I can't reproduce it on that page. No console errors from CodeMirror.

kaldari added a subscriber: kaldari.EditedNov 8 2017, 10:26 PM

@Niharika, @Samwilson: It seems to be caused by the combination of CodeMirror and the "Easy LST" default gadget, although it's not consistently reproducible. (Note that CodeMirror has to actually be activated to reproduce the bug.)

I can't reproduce this bug with Easy LST enabled, but I can with the (old) OCR gadget. Does that work for any of you?

OP replied with:

I've found what seems to be the confliction, if you could confirm. "OCR: Enable OCR button in Page: namespace." seems to be the culprit.
After enabling, the problem happens when loading the page with CodeMirror enabled.

From my testing, this seems to be caused by both the OCR gadget and the Easy LST gadget. Much less with LST than with OCR though. Same error in both cases:

Uncaught TypeError: Cannot read property 'call' of undefined
    at jQuery.fn.init.cmTextSelection [as textSelection] (<anonymous>:232:115)
    at setupWikitextEditor (<anonymous>:130:114)
    at HTMLDocument.<anonymous> (<anonymous>:130:915)
    at mightThrow (load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:49)
    at process (load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50)

Easy LST is a default gadget while OCR has been enabled by 315 people, with 58 active users.

Interestingly, when I made loaded both as user scripts for testing and disabled the gadgets, the bug disappeared. This makes me believe that user scripts are somehow loading later in the load order which mitigates the race condition between the gadget and CodeMirror. I haven't heard anyone confirm my theory yet, but if that is true, we might be able to fix the problem by adding a dependency in the gadget on CodeMirror.

Currently I'm awaiting my staff rights from Jamesofur to be able to play around with editing gadget dependencies. I'd ask Sam but I'm not sure what will work and need to try a few different dependencies and I don't to annoy Sam. :P

I added Resourceloader dependencies for the gadgets but that didn't work so I updated the gadget code for both Easy LST and OCR to explicitly wait on CodeMirror lib before loading. The downside is that it'll load CodeMirror for everyone, whether they have the beta feature or not. But on the plus side, it seems to have fixed all loading problems.
You can test it out here. I think it's a reasonable trade off.

Hmm, not quite the solution I was hoping for, but I guess it works. @DannyH: Any thoughts?

I don't think it's a good idea. CodeMirror still has some quirks, and if somebody on the PlayStation3 browser has a problem with it, we want to be able to say "turn it off". It's not ready for full-scale mandatory deployment.

We need something else... What are the other options?

We might be able to do something similar to Niharika's solution, but only if the user has actually enabled CodeMirror, i.e. use the API to check the user options before adding the CodeMirror module as a dependency.

Otherwise, I think we'll have to delay the loading of those two gadgets. It may work to just set a zero millisecond setTimeout (to move the gadgets to the bottom of the current execution stack).

I don't think it's a good idea. CodeMirror still has some quirks, and if somebody on the PlayStation3 browser has a problem with it, we want to be able to say "turn it off". It's not ready for full-scale mandatory deployment.
We need something else... What are the other options?

Wait, I think there was some miscommunication. It'll load the Codemirror library in the background for everyone but they definitely won't see it unless they have CodeMirror turned on and enabled.

@Niharika: Ah, I was confused when you said "it'll load CodeMirror for everyone, whether they have the beta feature or not." If you mean it's just loading the module, that's not as bad :) It still seems wasteful to load unneeded JS for everyone. What do you think of the other ideas above?

@Niharika: Ah, I was confused when you said "it'll load CodeMirror for everyone, whether they have the beta feature or not." If you mean it's just loading the module, that's not as bad :) It still seems wasteful to load unneeded JS for everyone. What do you think of the other ideas above?

Yep, it's just loading the module. I like the first option you suggested "use the API to check the user options before adding the CodeMirror module as a dependency". Will try that.

Ankry added a subscriber: Ankry.EditedDec 1 2017, 6:47 AM

Yep, it's just loading the module. I like the first option you suggested "use the API to check the user options before adding the CodeMirror module as a dependency". Will try that.

@Niharika , @kaldari : Some users (including me) practice the following technique:

  • open ~50 Page ns pages in new tabs for offline work
  • wait after they finish loading
  • activate the tabs when the users are already offline

Some browsers render the pages not while being loaded, but when their tabs are activated (assuming: to save resources). I assume, most scripts are also run offline then.

Will this work? Or the pages would not render because waiting for API call?
Or, maybe, the problem described here is enwikisource specific and does not inflict other wikisources that also use the OCR gadget?

@Ankry I don't follow. What are you suggesting?

Ankry added a comment.Dec 1 2017, 7:25 AM

@Ankry I don't follow. What are you suggesting?

Just asking whether planned changes might be a problem for users that work (render pages while being) offline and whether they are enwikisource specyfic or affects also other wikis.

@Ankry I don't follow. What are you suggesting?

Just asking whether planned changes might be a problem for users that work (render pages while being) offline and whether they are enwikisource specyfic or affects also other wikis.

This change is not affecting anything in that way. It's meant to change the order in which gadgets are loaded to avoid a bug. As for about being enwikisource specific or not, we haven't heard about the bug from anyone besides folks on enwikisource. It'd be difficult for us to check all other wikis but if I had to guess I'd say other wikisources using the same gadgets could be affected.

Ankry added a comment.EditedDec 1 2017, 9:52 AM

@Niharika : OK. I will make some tests and I will report if I notice any regression.

@Samwilson : screenshot in your initial report shows enhanced toolbar, while the later one shows the "old" toolbar. It seems to me that the edit window may be quite OK (half-screen) with the "old" toolbar, but still full-width with the enhanced toolbar. Is this really fixed?

Same effect in plwikisource (where at the moment only 4 users test this feature - and they may be inactive in last weeks/months so a few months/years delay in reporting an error is quite possible).

@Ankry I was testing with the enhanced toolbar. Do you still see the bug?
I can check this out on plwikisource too.

Ankry added a comment.EditedDec 1 2017, 12:32 PM

@Ankry I was testing with the enhanced toolbar. Do you still see the bug?
I can check this out on plwikisource too.

Yes. Just tested a while ago on both: en and pl.ws.
I use monobook and an older FF (NSAPI-compatible) if it does matter.
After switching to the enhaned toolbar my edit window looks like on the screenshot.
Tested in private window to avoid any caching effects.

Just noticed in is not 100% reproducible:

  • in most cases the scan appers to the right , and moves down while the enhanced tool is being loaded
  • in some cases (10-20%) the view is OK.

Tested also in Chrome, the statistics are reversed there: in most cases it is OK, sometimes the scan is moved below the edit window.

It seems to me that the behaviour is dependent on some scripts (gadgzts?) loading sequence.

Let me know, if I can made any debugging and how.

@Ankry can you try this in Chrome? And can you tell me if there's anything in your custom js?

Ankry added a comment.Dec 1 2017, 12:51 PM

@Ankry can you try this in Chrome? And can you tell me if there's anything in your custom js?

Another behaviour in chrome, as pointed above.
I have some private gadgets on pl.ws (https://pl.wikisource.org/wiki/Wikiskryba:Ankry/common.js) and none on en.ws.

I will test also with a clean account.

Ankry added a comment.Dec 1 2017, 12:54 PM

Also, when adding '&debug=1' to the URI everything load fine. Any hint?

Ankry added a comment.Dec 1 2017, 3:20 PM

OK. I made some tests in pl.ws with chrome and a fresh account with wiki syntax hihlighting took enabled:

  • with standard "default" config (almost all gadgets disabled) - no problem - edit window always left to the scan
  • with most gadgets enabled - the problem still appears in one per 10-20 page reloads, regardless of used skin (vector/monobook).

With FF the problem appears almost always regardless of gadget setings.

OK. I made some tests in pl.ws with chrome and a fresh account with wiki syntax hihlighting took enabled:

  • with standard "default" config (almost all gadgets disabled) - no problem - edit window always left to the scan
  • with most gadgets enabled - the problem still appears in one per 10-20 page reloads, regardless of used skin (vector/monobook).

With FF the problem appears almost always regardless of gadget setings.

@Ankry this is super helpful. Thank you. I'm clueless as to what is causing the problem in Firefox but I will test further and try the other solutions Kaldari suggested above. Hopefully that will fix it.

Ankry added a comment.Dec 1 2017, 6:02 PM

I am unsure if they are related, but this problem seems to have a bit similar (random) behaviour to the image moving problem described in T145365 .
The latter is still unresolved, but users have a workaround (clicking a zooming tool button).

Maybe the scan's div is not available yet while tools / gadgets are being initialized? Just guessing...

I went with the option Kaldari suggested - to load the library only if the user has codemirror enabled, for both gadgets. It'd be great if someone can confirm if the gadgets are working as expected.

Sorry, I'm still getting the error (with old-OCR, and CodeMirror toggled on prior to page load). I've tried loading the gadget as a user-script, but the error seems completely unreproducable then. I'll keep trying.

Niharika closed this task as Resolved.Dec 6 2017, 2:26 AM
Niharika moved this task from In Development to Q1 2018-19 on the Community-Tech-Sprint board.

Thanks to loads of help from Sam, this bug is now fixed! Rejoice, all!

DannyH moved this task from Estimated to Archive on the Community-Tech board.Dec 19 2017, 1:13 AM