Page MenuHomePhabricator

[regression] Unable to get old toolbar in namespace Page: whatsoever the preference request for the old toolbar and break the wikieditor toolbar
Closed, ResolvedPublic

Description

With preferences --> Edit --> deselect "Enable enhanced editing toolbar" we should get the old toolbar, this work in all namespace except in Page: namespace. Bug classified as Mediawiki extensions/ProofreadPage as I guess it comes from the javascript. Tpt if it doesn't come from the extension can you reclassify it please.


Version: unspecified
Severity: major
URL: https://en.wikisource.org/w/index.php?title=Page:U.S._Department_of_the_Interior_Annual_Report_1873.djvu/11&action=edit

Details

Reference
bz69447

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:41 AM
bzimport added a project: ProofreadPage.
bzimport set Reference to bz69447.
bzimport added a subscriber: Unknown Object (MLST).
Phe created this task.Aug 12 2014, 10:52 PM

Maybe related to gerrit 141293 because the content model is not wikitext for that page.

Tpt added a comment.Aug 13 2014, 7:48 PM

The root cause of the bug is that now MediaWiki core only displays the old edit toolbar in pages with CONTENT_MODEL_WIKITEXT.

I believe that it's ProofreadPage that should display itself the toolbar so I keep this bug in ProofreadPage.

TheDJ added a comment.Aug 13 2014, 7:51 PM

eh those pages are not wikitext ?

what are they then ?

Tpt added a comment.Aug 13 2014, 8:07 PM

They have their own content model that is mostly the composition of 3 Wikitext areas (header, body and footer) with some data about the proofreading level of the text. Currently the storage serialization is still wikitext but it may be changed to a cleaner format like JSON. See the implementation of the PagePageContent: https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FProofreadPage.git/1c5685425ba4bc41c174552d5e61b1d4de343043/includes%2Fpage%2FPageContent.php

Change-Id: Ifafe41f7bc91183e0db4c10d8340a8b620b380bc.

Change 156009 had a related patch set uploaded by Legoktm:
Revert "Toolbar: Only show on WikiText pages"

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

Change 157054 had a related patch set uploaded by Tpt:
Adds a method to EditPage that allows extensions to show/hide the toolbar

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

Phe added a comment.Aug 31 2014, 11:39 AM

Tpt, there should be a second patch depending on this one to overload this new function in the proofreadpage code ?

I increase priority, I didn't notice it but it's not only the old toolbar which is broken but the enhanced does not behave correctly too. (inserting an internal link does not open a popup window but insert [[Link title]]).

Not "critical". See [[mw:Bugzilla/Fields#Severity]]

GOIII added a comment.Aug 31 2014, 1:37 PM

(In reply to Philippe Elie from comment #8)

I increase priority, I didn't notice it but it's not only the old toolbar
which is broken but the enhanced does not behave correctly too. (inserting
an internal link does not open a popup window but insert [[Link title]]).

Fwiw... the pop-up windows (known as 'dialogs' in the WikiEditor extension) only work if both 'Enable enhanced editing toolbar' and 'Enable wizards for inserting links, tables as well as the search and replace function' are enabled in your User preferences. What you described is normal for when ONLY 'Enable enhanced editing toolbar' is selected.

Since the change that caused this bug happened, disabling 'Show edit toolbar' in your User preferences has help some Users: get over similar issues even though that option should have nothing to with the enhanced toolbar (WikiEditor)
if both are enabled [allegedly, that is - others say disabling it should disable both the classic and WikiEditor toolbars while that is obviously not the case, if it ever was]

That said - can we get some movement on this or what? The quirkiness being discovered only grows with every passing day now.

Phe added a comment.Aug 31 2014, 1:42 PM

I enabled both of them but I can't get the wizard to work, is it working for you ?

Tpt added a comment.Aug 31 2014, 1:54 PM

(In reply to Philippe Elie from comment #8)

Tpt, there should be a second patch depending on this one to overload this
new function in the proofreadpage code ?

Not yet as I wait the core change to be merged. The ProofreadPage change will very small but I would like to see the core change to be reviewed first in order to don't have to change the Prp change twice or more because of changes in the core change done during the review process.

GOIII added a comment.Aug 31 2014, 1:55 PM

(In reply to Philippe Elie from comment #11)

I enabled both of them but I can't get the wizard to work, is it working for
you ?

Yes, the dialogs work for me when I have both the enhanced & wizards enabled and 'show edit toolbar' disabled. I typically don't use the wizards so I will disable them again after I post this.

When I have all three enabled, dialogs still work for me but tangent items like the CharacterInsert extension (via a local gadget) and similar customizations start "breaking down" and are no longer consistently generated nor reliable when selected/executed.

GOIII added a comment.Aug 31 2014, 2:15 PM

(In addition to George Orwell III from comment #13)

When I have all three enabled, dialogs still work for me but tangent items
like the CharacterInsert extension (via a local gadget) and similar
customizations start "breaking down" and are no longer consistently
generated nor reliable when selected/executed.

bug 70233 is similar to what I meant by "breaking down" along with several other User:s on en.WS who have reported much the same.

There are so many pending conflicting patches and related bugs, I am lost. Can somebody summarize the proposed solutions and their corresponding patch groups, and preferably also say which one is the best and should be merged?

He7d3r added a comment.Sep 1 2014, 1:03 PM

One week ago I suggested the original patch to be reverted (as a "quick" fix)
https://gerrit.wikimedia.org/r/156009
but it seems it'll get as much time as any other patch to get merged...

Change 157679 had a related patch set uploaded by Bartosz Dziewoński:
Revert "Toolbar: Only show on WikiText pages"

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

Change 157680 had a related patch set uploaded by Bartosz Dziewoński:
Revert "Toolbar: Only show on WikiText pages"

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

Should have poked someone about it. I +2'd the revert (jenkins seems stuck though) and scheduled it from SWAT deployment today in about an hour (https://wikitech.wikimedia.org/wiki/Deployments#Near-term).

Now, can somebody summarize the proposed solutions and their corresponding patch groups, and preferably also say which one is the best and should be merged? :D

Change 156009 merged by jenkins-bot:
Revert "Toolbar: Only show on WikiText pages"

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

He7d3r added a comment.Sep 1 2014, 2:21 PM

I believe there are only two proposed solutions:

  1. Create a function shouldShowToolbar which returns true if the content model is wikitext (extensions would subclass EditPage I think):

https://gerrit.wikimedia.org/r/#/c/157054/1/includes/EditPage.php,unified

  1. Create a hook to allow extensions to redefined the value of the variable $showToolbar:

https://gerrit.wikimedia.org/r/#/c/120357/6/includes/EditPage.php,unified

GOIII added a comment.Sep 1 2014, 9:50 PM

Forgive my "newbie-ness" here, I know 'soap-box' like speeches are frowned up here, but if...

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

... is indeed the reversion of the change(s) that caused the problem reported in this bug in the first place, what else is there to actually "solve" here?

Maybe I reached this conclusion because I'm in the same boat as the comments made back in #c15 - I can't keep the needed/wanted regressions separate from the pseudo-patches and/or semi-enhancements that all seem to be lumped together for no discernable good reason now.

Granted, my participation here is rather recent but I assure you I've been lurking here for some time now and my observation has always been that there are too many conflicting interests at play all at once. Eventually these interests seem to 'muddy the waters' of a particular bugzilla by arbitrarily usurping some reports as duplicates without proper rationale, diffuse the focus of report by peppering the "see also" list with tangent issues made in other reports or build little self-serving cabals by making other reports blocking/dependent on what should be stand-alone issues or concerns.

That said, if the above linked merge resolves the reported issue(s) then I'm of the opinion that this bug should be closed and any further changes should be taken up in the appropriate bugzillas (if they exist) or open a new one addressing whatever needs resolving/discussion.

You're right, the revert fixes this bug, and we should discuss how to re-implement the thing that caused it on bug 29908 (without breaking things this time ;) ).

The revert hasn't been deployed on the affected wikis yet, I was confused when I wrote comment 19. It will actually be deployed tomorrow. Nevertheless let's close this bug.

GOIII added a comment.Sep 2 2014, 6:02 AM

Errr.... possible stupid question but please humor my ignorance just in case its not:

Did we inadvertently remove some "stuff" from EditPage.php with the merging of...

https://gerrit.wikimedia.org/r/#/c/156009/3/includes/EditPage.php

... instead of merging the "more reflective" current state of the file - also containing the reversion of the troublemaking code - in ...

https://gerrit.wikimedia.org/r/#/c/157680/1/includes/EditPage.php

...?

I only ask because if you toggle between the two, you can clearly see the part dealing with "blankPages" right after the section we're dealing with in the reversion is present in #157680 (as well as in #157679 while we're at it), but not in #156009.

And, as I far as I can tell, the part dealing with "blankPages" is also present in the current .php file.

Change 157679 merged by jenkins-bot:
Revert "Toolbar: Only show on WikiText pages"

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

Change 157680 merged by jenkins-bot:
Revert "Toolbar: Only show on WikiText pages"

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

The revert has been deployed now.

Re comment 24, that just means that the patches are based on a slightly different version of MediaWiki. That piece of code dealing with blank pages was added in change c3fcaba0, which wasn't yet merged when the original patch was written, but was already merged when it was backported to 1.24wmf19 for deployment. Nothing to worry about :)

He7d3r set Security to None.
GOIII moved this task from Backlog to Closed on the WikiEditor board.Apr 3 2016, 9:10 AM
GOIII moved this task from Backlog to Done on the ProofreadPage board.Jun 12 2016, 3:57 AM