Summary

The inline editor no longer activates when selecting text

Review Request #2991 — Created March 21, 2012 and submitted Nov. 24, 2012, 5:15 p.m.

Information

Owner

ddruska

Repository

Djblets

Branch

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

The inline editor no longer activates when selecting text.

Mouse events are monitored to check if the user is simply clicking on the editor box, or is dragging to copy text in the editor text area.

Testing Done

Latest Chrome, Firefox, Opera - Linux

Not tested on touch devices.

Issues

Description	From	Last Updated
Instead of using this mousedown/mousemove/mouseup combination, could you use dragstart/mouseup? That might be simpler.	mike_conley	March 28, 2012, 11:31 a.m.
A style issue, .mousedown could be on a new line and indented (along with .mouseup).	ME medanat	Aug. 20, 2012, 8:17 a.m.
I think the return false should be outside the if statement to prevent the event bubbling. Mousedown might also need …	ME medanat	Aug. 20, 2012, 8:17 a.m.
Blank line after variable declarations.	chipx86	Aug. 20, 2012, 10:57 a.m.
Can getSelection() ever return null/undefined?	chipx86	Aug. 20, 2012, 8:18 a.m.
The "selection ==" should be on the following line.	chipx86	Aug. 20, 2012, 8:18 a.m.
Blank line between these.	chipx86	Aug. 20, 2012, 10:57 a.m.

Summary:

-	The inline editor no longer activates when selecting text.
+	The inline editor no longer activates when selecting text

Testing Done:

	+	Chrome 19.0.1068.1 Linux
	+	Firefox 11.0 Linux

djblets/media/js/jquery.gravy.js (Diff revision 1)

one really small thing..
do we still need this line here?
isDragging seems to not be used in other places

ddruska March 22, 2012, 1:07 p.m.

We need to set it to false to reset the state. If it's not reset it will never be false again and you can't edit.

JI

jimrrchen March 22, 2012, 1:30 p.m.
```
Cool...I see what you did there..my bad
```

djblets/media/js/jquery.gravy.js (Diff revision 1)

The issue has been resolved. Show all issues

Instead of using this mousedown/mousemove/mouseup combination, could you use dragstart/mouseup?  That might be simpler.

ddruska March 22, 2012, 1:07 p.m.

I looked into jQuery UI's draggable stuff (http://docs.jquery.com/UI/Draggable) but it's not really what we want. That's used to physically drag elements around the page. I couldn't find a way to stop the element from moving while still listening for drag events.
As for binding "dragstart", it seems to only activate this if something is being dragged over the element (eg. I think we use this event for dragging & dropping screenshots and file attachments onto a review).

chipx86 March 24, 2012, 6:55 a.m.

Actually, I think selectstart is the signal we really want to use here. Can you try that? I think you'll be able to simplify this a bit.

Otherwise, the logic you have is very dangerous. It's acting as sole owner over events on the window. I'm pretty sure you don't want to actually remove all registered mouse events on the window object. Look into jQuery's one() function.

Change Summary:

Much cleaner code for the inline editor not activating when selecting text.

Diff:

Revision 2 (+17 -3)

Show changes

djblets/media/js/jquery.gravy.js

I need someone to confirm these issues as I am not 100% sure of them.

djblets/media/js/jquery.gravy.js (Diff revision 2)

The issue has been resolved. Show all issues

A style issue, .mousedown could be on a new line and indented (along with .mouseup).

djblets/media/js/jquery.gravy.js (Diff revision 2)

The issue has been resolved. Show all issues

I think the return false should be outside the if statement to prevent the event bubbling. Mousedown might also need a return false.

Status: Discarded

Status: Re-opened

Diff:

Revision 3 (+19 -4)

Show changes

djblets/media/js/jquery.gravy.js

Change Summary:

New method.

Description:

		The inline editor no longer activates when selecting text.

~		This is fixed by monitoring mousemove events, and only opening the editor
	~	This is done by monitoring if there is a new text selection, and keeping the editor closed if there is.
-		if the mouse was not moved.

Doesn't work well in Opera, which triggers "mouseup" on a right-click.

chipx86 March 28, 2012, 3:34 p.m.

Left-click works, though?

Does right-click fail already with opera?

ddruska March 29, 2012, 2:20 a.m.

Yeah left clicking works. We could either use:
1) This selection method
2) The nested listeners with one/unbinding (like previous diff)
3) Or this one http://jsfiddle.net/kZS5w/1/ which compares the mouse pos and doesn't need unbinding.

chipx86 March 31, 2012, 7:30 p.m.

I think this was mentioned in the meeting, but for the record, can you check the button pressed (e.which)?

Change Summary:

Now also checks for a left-click.

Description:

		The inline editor no longer activates when selecting text.

~		This is done by monitoring if there is a new text selection, and keeping the editor closed if there is.
	~	Text selection in the editor box are monitored, and the editor won't open if text is selected. The mouse button is also monitored.

Testing Done:

~		Chrome 19.0.1068.1 Linux
	~	Latest Chrome, Firefox, Opera - Linux
-		Firefox 11.0 Linux

Diff:

Revision 4 (+20 -4)

Show changes

djblets/media/js/jquery.gravy.js

One case this still doesn't fix is if you start selection, and then in the same drag, unselect it all. Valid operation, but it would cause this to open.

I think what I like much better than storing the selection and comparing is to listen for a click, and listen for a selectstart. If selectstart happens, set a flag. On click, if the flag is set, ignore. If not set, open.

chipx86 April 4, 2012, 4:28 a.m.

And of course we discussed selectstart was IE-only. Turns out WebKit has this too, though, but not Firefox. This can be checked with $.support.selectstart.

I think this is the ideal thing to use, when available. And of course, there would need to be a fallback. This could be it, though the problem above is annoying. Mouse cursor movement is another thing that could be used (comparing mousedown/mouseup event positions), but would need to allow for a tolerance, and that's an annoying area to get into (particularly with high-DPI screens and touch events).

chipx86 July 31, 2012, 4:28 a.m.

Hey Dave,

Don't know if you're available to make these changes, but I'd love to get it into a release. If not, let me know, and I'll go through and fix the remaining issues.

ddruska Aug. 1, 2012, 11:13 a.m.

Hey Christian,

Don't worry! I'm still around and can complete this feature.

djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line after variable declarations.
```
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been dropped. Show all issues
```
Can getSelection() ever return null/undefined?
```
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been dropped. Show all issues
```
The "selection ==" should be on the following line.
```
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

Moved back to a modified version of my original idea. isDragging now starts at "true" so if you are selecting outside and drag inside, the editor will not activate.

Description:

		The inline editor no longer activates when selecting text.

~		Text selection in the editor box are monitored, and the editor won't open if text is selected. The mouse button is also monitored.
	~	Mouse events are monitored to check if the user is simply clicking on the editor box, or is dragging to copy text in the editor text area.