Page MenuHomePhabricator
Create Task
Maniphest T29894

Move 'editondblclick' event listener down from body to div#bodyContent
Closed, ResolvedPublic
Actions

Description

Author: theevilipaddress

Description:
Currently, when you activate the "edit on double click" setting, any double-click anywhere on the body will open the edit form. I have found that to be somewhat confusing already, for example when mistakenly double-clicking on the sidebar, the search box or other parts outside the actual page content. Thus, I believe that this script should probably be added via Resource Loader as an event listener on div#bodyContent instead of on the <body> tag, as it currently is the case.

Version: unspecified
Severity: minor

Details

Reference
bz27894

Event Timeline

• bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:36 PM
• bzimport added a project: MediaWiki-JavaScript.
• bzimport set Reference to bz27894.
• bzimport created this task.Mar 6 2011, 1:34 PM
Platonides added a comment.Mar 6 2011, 2:54 PM

It was added directly to the body tag.

$bodyAttrs['ondblclick'] = "document.location = '" . Xml::escapeJsString( $this->getTitle()->getEditURL() ) . "'";

However, <div id="bodyContent"> is added dirrectly by the skin, it doesn't seem a good idea to add it there. So yes, an event listener seems the way to go.

Krinkle added a comment.Mar 9 2011, 12:12 AM

The id and locatin of that tag is variable per-skin.

The mw.util class has a $content variable though which contains a jQuery object of the content element in the current skin.

should be as simple as attaching a .dblclick() to mw.util.$content

• bzimport added a comment.May 12 2011, 6:09 AM

niknyby wrote:

proposed patch for this bug

Krinkle, I used the skin-independent mw.util.$content variable like you suggested. I removed the php-generated 'ondblclick' attribute to <body> and made a jQuery event listener. This solution works for me.

attachment 0001-the-edit-on-double-click-setting-now-creates-the-dou.patch ignored as obsolete

Krinkle added a comment.May 16 2011, 9:21 PM

(In reply to comment #3)

Created attachment 8531 [details]
proposed patch for this bug

Krinkle, I used the skin-independent mw.util.$content variable like you
suggested. I removed the php-generated 'ondblclick' attribute to <body> and
made a jQuery event listener. This solution works for me.

Hi Nik,

Thanks for your patch.

Two notes though:

  1. document.location was originally a read-only property, although Gecko browsers (like Firefox) allow you to assign to it as well. For cross-browser safety, use window.location instead. [1]
  1. Appending '&action=url' to the url is not safe and prone to error. The following three urls are all examples of how MediaWiki can be configured that are popular and supported:

Appending '&action' only works for the last one. I suggest using the wgScript mw.config variable ( mw.config.get( 'wgScript' ) ), and manually building a url with the title (wgPageName) and action (edit) parameters.

Krinkle

[1] https://developer.mozilla.org/en/document.location#Notes

attachment 0001-the-edit-on-double-click-setting-now-creates-the-dou.patch ignored as obsolete

• bzimport added a comment.May 18 2011, 6:29 AM

niknyby wrote:

patch revision

Krinkle, thanks for your suggestions. I've revised the patch, the edit URL is now reliable since I'm building it manually instead of appending to the existing URL.

However, this means that when double-clicking to edit, the edit URL will always be in this format:

Regardless of the URL format of the rest of the wiki.

attachment 0001-the-edit-on-double-click-setting-now-creates-the-dou.patch ignored as obsolete

• bzimport added a comment.Jun 9 2011, 1:42 PM

theevilipaddress wrote:

Nice to see the follow-ups on this bug. I've recently noted another problem with this event listener that would be nice to fix already in this revision. We shouldn't use an anonymous function here, as these can't be removed via Element.removeEventListener(). Removing them would be useful for scripts like Twinkle: Currently, double-clicking a page while using Twinkle opens the edit mode, which is quite annoying.

Krinkle added a comment.Jun 9 2011, 3:07 PM

(In reply to comment #6)

Nice to see the follow-ups on this bug. I've recently noted another problem
with this event listener that would be nice to fix already in this revision. We
shouldn't use an anonymous function here, as these can't be removed via
Element.removeEventListener(). Removing them would be useful for scripts like
Twinkle: Currently, double-clicking a page while using Twinkle opens the edit
mode, which is quite annoying.

Remember that this is an opt-in preference. Either Twinkle users shouldn't enable that preference or Twink should $content.unbind('dblclick') or something like that (perhaps namespace the event)

Krinkle added a comment.Jul 6 2011, 10:10 PM

@nik: I've reviewed your patch. It looks good.

If you're familiar with event namespaces (in particular in jQuery) it would be nice if you could bind it to "dblclick" with the "mw-editondblclick" namespace so that scritps can use .unbind() for that namespace only without removing all events.

It's a bit beyond the scope of "easy" though, so if you don't feel like doing that I will apply your good patch as-is and then apply an additional fix for the namespace.

Thanks in advance,

• brion added a comment.Jul 6 2011, 10:31 PM

Patch has several bugs:

  • fails to take revision ID (oldid parameter) into account, and possibly others
  • hardcodes use of script & action=edit instead of using $wgActionPaths
  • clutters mediawiki.util.js with rarely-used code for all users

I would recommend:

  • grab the edit URL from the edit tab to avoid having to recalculate it manually (which causes the first two bugs above)
  • move from mediawiki.util.js to its own module, and only add that module when the option is being used
• brion added a comment.Jul 6 2011, 10:31 PM

(patch also doesn't do URL escaping on the title, which can cause failures when eg title contains a literal &)

Krinkle added a comment.Jul 8 2011, 7:40 PM

nice catc(In reply to comment #9)

Patch has several bugs:

Nice catches.

A few more:

  • Loading of module needs to be moved out of OutputPage->headElement probably
  • Should account for the situation in which there is no edit link on the page (ie. not bind the event, because if MediaWiki decided not to output that tab, it probably has a good reason)
  • There ar differences in skins in where this link will be. This is a common problem we are encountering more and more often lately (just look at the code that makes mw.util.$content and addPortletLink work, it's disguisting). Probably need to make some radical changes to skins in the future, but oh well. This can be fixed in the long term by not allowing skins to use different IDs for core elements or by requiring skins to log into a method how/where elements are) – back to the present this can be solved by either:
    • exporting the edit-tab link through a config variable (alert! this is not a config variable)
    • exporting it as a piece of information in the per-module configuration (sounds good, except that such a thing doesn't exist yet)
    • scrapping HTML for "#ca-edit a" and not caring about other skins.
    • a switch-case for all skins.

The last two options will be ugly and not extendable by custom skins.

Krinkle added a comment.Jul 8 2011, 7:43 PM

Created attachment 8759
Follow-up patch implementing the points above

Attached:

bug27894diff.patch1 KBDownload

Krinkle added a comment.Aug 1 2011, 10:13 PM

Sorry for the long delay.

Patch applied in r93664.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL