Last modified: 2014-08-26 03:27:58 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T41268, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 39268 - [Regression] Dynamically added textarea within the editform no longer work in the classic edit toolbar
[Regression] Dynamically added textarea within the editform no longer work in...
Status: RESOLVED WORKSFORME
Product: MediaWiki
Classification: Unclassified
Page editing (Other open bugs)
1.20.x
All All
: Normal major (vote)
: ---
Assigned To: Nobody - You can work on this!
: code-update-regression
Depends on:
Blocks: edit-toolbar Wikisource
  Show dependency treegraph
 
Reported: 2012-08-11 16:57 UTC by Tpt
Modified: 2014-08-26 03:27 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Tpt 2012-08-11 16:57:55 UTC
Reported by User: billinghurst : Only tested this at enWS, however, when one tries to insert formatting into the header/footer utilising the old toolbar, we are back to where the toolbar drops that formatting into the main body of the page, not the header or footer.
Comment 1 billinghurst 2012-08-12 13:46:47 UTC
Happens with both older and newer toolbars, so is not the toolbar itself.
Comment 2 Krinkle 2012-10-03 15:48:37 UTC
Exactly what is the problem?

This is what I see: http://cl.ly/image/3T0K1Z2M202J

What should it look like instead?
Comment 3 Tpt 2012-10-03 16:06:31 UTC
This is not a UI problem. The problem is that when the user select a piece of text inside of the header or the footer textarea and, by example, click on the "bold" button of the toolbar, the "'''" are inserted in the main textarea are not in the header/footer one.
Comment 4 Krinkle 2012-10-03 16:15:20 UTC
(In reply to comment #3)
> The problem is that when the user select a piece of
> text inside of the header or the footer textarea and, by example, click on the
> "bold" button of the toolbar, the "'''" are inserted in the main textarea are
> not in the header/footer one.

Not what I asked, but I apparently asked the wrong question :) Now I see the bug yeah, that didn't happen before.
Comment 6 Krinkle 2012-10-03 16:25:17 UTC
I just tested this locally and it seems to be working fine. The reason it doesn't work on Wikisource is because those header/footer textareas are created *afterwards* with javascript, which means they don't have any events bound.

I don't think this ever reliably worked, it must've been a coincidence (whichever executed first). Is there are reason for these to be split up in the front-end instead of the backend?

Also, as a workaround it could be done from a top-loading module, which will have its document ready handler executed first.
Comment 7 Tpt 2012-10-03 17:03:51 UTC
(In reply to comment #6)
I don't think there is really a reason for these to be split up in the
front-end instead of the backend: I believe that the code have been written before the introduction of the ability to override cleanly the editform and has never been rewrite after.
This rewriting is on my TODO list but I don't know when I'll have time to do it.
It might be a good idea to write at the same time a new content storage system using ContentHandler in order to move from:

<noinclude><pagequality level="3" user="Billinghurst" /><div class="pagetext">HEADER</noinclude>
CONTENT
<noinclude>FOOTER</noinclude>

to something like:

{
  "quality":  {
    "level":3,
    "user":Billinghurst"
  },
  header: "HEADER",
  content: "CONTENT",
  footer: "FOOTER"
}
Comment 8 Helder 2012-10-03 22:19:08 UTC

*** This bug has been marked as a duplicate of bug 37084 ***
Comment 9 Krinkle 2012-10-04 19:47:46 UTC
Un-duping. bug 37084 may appear to be the same, bug it in fact not at all related.

This bug 39268 is:
* about legacy toolbar
* applies to elements being added to the page after the toolbar initialised, which means it doesn't have the event listener bound.

Bug 37084 is:
* about WikiEditor
* applies to the edit summary field which is on the page from the very beginning.

different extensions, different causes, different circumstances, seeking for different solutions.
Comment 10 billinghurst 2012-10-04 22:22:18 UTC
Can I comment that it is not solely the legacy toolbar.  As I noted above, I have the same issue in the WikiEditor toolbar too.
Comment 11 Krinkle 2012-10-05 00:01:54 UTC
(In reply to comment #10)
> Can I comment that it is not solely the legacy toolbar.  As I noted above, I
> have the same issue in the WikiEditor toolbar too.

Yep, that is expected. WikiEditor binds to 1 area only (the text area it is attached to). legacy toolbar binds to all input on the page. But if the inputs are added to the page afterwards, it won't have an event listener attached to it.
Comment 12 Rob Lanphier 2012-10-12 16:19:02 UTC
Does this only happen on Wikisource, or does it happen on all wikis?  I'm removing this as a blocker since the cats out of the bag with 1.21wmf1, and blocking 1.21wmf2 won't fix the issue.
Comment 13 Tpt 2012-10-12 16:32:19 UTC
It happen on all wiki.
Comment 14 Krinkle 2012-10-13 04:47:30 UTC
(In reply to comment #13)
> It happen on all wiki.

What other textareas exist on other wikis that are added after the toolbar has initialised?
Comment 15 Tpt 2012-10-13 06:02:42 UTC
I think there is a missunderstanding: ProofreadPage is used by not-Wikisource wikis like wikilivres.ca
but don't know if there is another extension that add a textarea after the toolbar is initialized.
Comment 16 Nemo 2013-01-13 16:54:43 UTC
I don't understand, is this bug still current?
Comment 17 Krinkle 2013-01-13 19:30:34 UTC
(In reply to comment #16)
> I don't understand, is this bug still current?

Yes. For reasons explained in comment 6 and comment 9, on the following url:

https://en.wikisource.org/w/index.php?title=Page:The_Irish_in_Australia.djvu/99&action=edit

... clickig in the "header" textarea and then on "Bold" will insert `'''Bold text'''`` in the Page textarea as opposed to the textarea that was focussed (Header).
Comment 18 Krinkle 2013-01-13 19:37:24 UTC
Removing "code-update-regression, " and lowering severity to "enhancement". I misinterpreted this bug originally.

Though it is a valid bug, at this point it is a bug in the Proofread script not a bug in the MediaWiki editor. The editor currently only supports inserting wikitext snippets in textarea's created by MediaWiki core.

Developing an extension that dynamically inserts foreign textarea's into the page (which the editor never supported) and complaining it doesn't work (as you can understand) doesn't make the editor broken, it is a wrong expectation.

I mean.. One could run up to a random person and ask him to sing a song, then don't complain if he doesn't sing well. He wasn't claiming to be a singer in the first place :)

Converting this bug into a feature request for the editor to support binding additional textarea's afterwards.

Could could file a bug against Proofread extension that depends on this.


API should probably look something like this:

> mw.toolbar.addToolbar(HTMLElement);
Comment 19 Nemo 2013-01-13 19:45:08 UTC
(In reply to comment #18)
> Though it is a valid bug, at this point it is a bug in the Proofread script
> not
> a bug in the MediaWiki editor. 

So perhaps this bug should be kept as it was and moved to the extension's component? Well either you or tpt will know what's best for fixing the problem.
Comment 20 billinghurst 2013-01-14 00:35:26 UTC
Hang on. IT USED TO WORK!!!

The toolbar used to work in the three component fields and it was a programming change in more recent MW changes that caused this to stop functioning. It is simply not an enhancement, it is base functioning.

In the end the data is all mediawiki core, the overlay is a javascript script to present the three components, each edit sits in the same revision.


--the file [[s:en:Page:The Irish in Australia.djvu/99]]--

--<##1st section (header)###
<noinclude><pagequality level="3" user="Billinghurst" /><div class="pagetext">{{RunningHeader||A BIRTHPLACE OF FREEDOM.|85}} ...


</noinclude>##2nd section (main)## text text text (actual text removed)...

##3rd section (footer)##<noinclude></div></noinclude>
<-- <<EOF>>--


This is simply a request to have text inserted at the point where the cursor is located, wherever the cursor is located (within the three false sections).  It is not a request to say always place this coding in the header or the footer.  We have and can do those and do it with bits like

	var editbox = document.getElementsByName('wpTextbox1')[0];
        var headerbox = document.getElementsByName('wpHeaderTextbox')[0];
        var footerbox = document.getElementsByName('wpFooterTextbox')[0];


* Wikisource is a major sister, one feels that we almost have to apologise for taking time away from enWP projects.
* Proofread page is our major tool to present works and to have texts available
* It is not an insignificant issue.

I don't see why it is seemingly dismissed as not important, or not an issue.
Comment 21 Krinkle 2013-01-14 17:47:51 UTC
(In reply to comment #20)
> Hang on. IT USED TO WORK!!!
> 
> The toolbar used to work in the three component fields and it was a
> programming change in more recent MW changes that caused this to stop
> functioning. It is simply not an enhancement

That it used to work doesn't mean the problem isn't in the Proofread extension. The classic toolbar never supported dynamically inserted textarea's. It always bound to specific (or any) textarea's when the page is ready, and keeps event handlers on them.

However since you seem to be convinced the regression was caused by a change in core and not in the Proofread extension, I've dug into the history of the classic toolbar.

Turns out there was indeed a point where a certain implementation detail was lost in a rewrite:

* r59832 (fixups r59655): In 2009, Dantman optimised the toolbar binding logic by using event delegation on document.editform instead of binding to all known textareas found by  document.createElement('textarea').

The purpose of it was to support multiple textareas on the editor - as registered and output by the server side.

It wasn't for the purpose of Proofread and not to support dynamically inserted textarea's per se. That was a coincidental consequence of the more efficient approach for event binding. But only as long as it is created and attached to the dom *within* the subtree of the id="editform" element.

* r86603: In April 2011, DieBuche rewrite the toolbar code from legacy/common/edit.js to a new module: mediawiki.action.edit.

In doing so all features were preserved, including supporting edit forms with more than 1 textarea (such as in SemanticMediaWiki). However the new implementation only worked for textarea's that exist on the page before the editor is initialised. This wasn't a problem as nothing in the previous version suggested this was an intentional side-effect. It was just how it was implemented.

Somewhere along the line the Proofread extension was written and basically just happened to work as it was because of it.

> This is simply a request to have text inserted at the point where the cursor
> is located, wherever the cursor is located (within the three false sections). 


Easier said than done. That would require the editor to know about the textarea. And know which textarea's are relevant and which ones are not. For example, if another widget presents a balloon dialog somewhere else on the page, the toolbar may not be related to it at all.

Visually it is easy to see which ones logically belong to the editor, but technically it isn't so trivial. Especially if one can pop up at any time.

It may not appear that way to you, but the Proofread extension inserts these additional textarea's with javascript, not with PHP. Which means that unless the editor waits for the proofread extension javascript to finish, they are inserted too late.

Yes, it can be implemented. But it hasn't been done yet, and it never was.

As explained earlier it worked by coincidence and stopped working in a rewrite that didn't cause any regressions in code using supported and documented features.


> * Wikisource is a major sister, one feels that we almost have to apologise
> for taking time away from enWP projects.

I don't give shiny rats' ass about English Wikipedia (not in particular anyway). A bug is a bug.

> * Proofread page is our major tool to present works and to have texts
>   available.

So who maintains it? Has it been thoroughly reviewed? Does the maintainer keep up with bug reports, latest standards and release notes and changes in MediaWiki core?

> * It is not an insignificant issue.
> 
> I don't see why it is seemingly dismissed as not important, or not an issue.

It is not considered unimportant. I was merely saying that it doesn't seem to be caused by MediaWiki core and I don't know enough about Proofread or Wikisource to fix it myself. 


---


So a lot of information on the table. Lots of different approaches to take from here.

I'm going to recommend we change the implementation of the classic edit toolbar again to use even delegation, and this time document that it is with the intention of supporting dynamically inserted textarea's (as opposed to being an implementation detail for efficiency reasons).
Comment 22 Krinkle 2013-01-14 17:50:06 UTC
(In reply to comment #21)
> * r59832 (fixups r59655): In 2009, Dantman optimised the toolbar binding
> logic  by using event delegation on document.editform instead of binding to [..]
> 
> * r86603: In April 2011, DieBuche rewrite the toolbar code from
> legacy/common/edit.js to a new module: mediawiki.action.edit.
> 

* r91750: July 2011, DieBuche made some final updates and removed the old toolbar code, including the seemingly no longer needed/relevant code for event delegation on document.editform (since we now use $('input, textarea') which supports them all. Except that it doesn't include elements that don't exist yet, which you get with event delegation.
Comment 23 Tpt 2013-01-14 21:13:39 UTC
(In reply to comment #21)
> So who maintains it? Has it been thoroughly reviewed? Does the maintainer
> keep
> up with bug reports, latest standards and release notes and changes in
> MediaWiki core?
The creator of the extension have stopped to contribute in 2011 and the extension have been without maintainer between 2011 and July 2012 when I get a commit access to it. But I haven't a lot of free time and:
1) I do it for fun so, I make things when I want to make it, not really when users want.
2) Programming is just an hobby and I'm not very comfortable with some advanced standards.

The extension have been reviewed at its deployment on Wikisource in 2007 and, because nobody pay attention to this extension beyond its "maintainer" (ThomasV and, since 2012, me), and, when there is a critical bug, Sam Reed, Aaron Schulz or Brion Vibber, changes since 2007 haven't been really reviewed.

I agree that the issue is more in ProofreadPage than in the MW toolbar.
I have begun to refactor some part of the back-end of the extension in order to implement the edit form of page pages in PHP in a more cleanly way than the one used by the very hacky current implementation in JavaScript. I have already done something like that for index pages (gerrit Idc00bddac8abc58e0b6414bdd9dc3f32cab3337a ). So I have let this bug wait for this rewriting that will solve, I hope, this issue.



I want also to say that, even if billinghurst reaction is maybe a little out of proportion a lot of Wikisource contributors feel that the Wikimedia Fondation staff doesn't care about Wikisource. Things like bug 34788 doens't help to make this fact change.
Comment 24 billinghurst 2013-05-21 15:51:36 UTC
This isn't an issue for me now.  Either there has been a change in the code that has resolved this, OR (and I think more likely) the changes that were made to how to implement (ye olde) custom edit buttons on the standard toolbar has resolved this matter.  I have asked locally on enWS and the one response says that it is resolved for them, though without a broader sample, it is difficult to be definitive.
Comment 25 Andre Klapper 2013-06-11 17:51:55 UTC
(In reply to comment #24)
> This isn't an issue for me now.  Either there has been a change in the code
> that has resolved this, OR (and I think more likely) the changes that were
> made
> to how to implement (ye olde) custom edit buttons on the standard toolbar has
> resolved this matter.  I have asked locally on enWS and the one response says
> that it is resolved for them, though without a broader sample, it is
> difficult
> to be definitive.

Thanks for the update. Closing this ticket for the time being. 
Also see bug 47872 about custom buttons in the toolbar.
Comment 26 Helder 2014-08-26 03:27:58 UTC
I believe this was fixed by change Id9469f9dfcbb92854780c63252cd9c5069e94487.

See also LQT bug 41220.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links