The inline editor no longer activates when selecting text

Review Request #2991 — Created March 21, 2012 and submitted

Information

Djblets

Reviewers

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.
Latest Chrome, Firefox, Opera - Linux

Not tested on touch devices.
Description From Last Updated

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

mike_conleymike_conley

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

ME medanat

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

ME medanat

Blank line after variable declarations.

chipx86chipx86

Can getSelection() ever return null/undefined?

chipx86chipx86

The "selection ==" should be on the following line.

chipx86chipx86

Blank line between these.

chipx86chipx86
DD
JI
  1. 
      
  2. 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
    1. 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.
    2. Cool...I see what you did there..my bad
  3. 
      
mike_conley
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Show all issues
    Instead of using this mousedown/mousemove/mouseup combination, could you use dragstart/mouseup?  That might be simpler.
    1. 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).
    2. 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.
  3. 
      
DD
ME
  1. I need someone to confirm these issues as I am not 100% sure of them.
  2. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    A style issue, .mousedown could be on a new line and indented (along with .mouseup).
  3. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    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.
  4. 
      
DD
DD
DD
DD
DD
  1. Doesn't work well in Opera, which triggers "mouseup" on a right-click.
    1. Left-click works, though?
      
      Does right-click fail already with opera?
    2. 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.
    3. I think this was mentioned in the meeting, but for the record, can you check the button pressed (e.which)?
  2. 
      
DD
chipx86
  1. 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.
    1. 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).
    2. 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.
    3. Hey Christian,
      
      Don't worry! I'm still around and can complete this feature.
  2. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
    Show all issues
    Blank line after variable declarations.
  3. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
    Show all issues
    Can getSelection() ever return null/undefined?
  4. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    The "selection ==" should be on the following line.
  5. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Blank line between these.
  6. 
      
DD
DD
david
  1. Ship It!
  2. 
      
DD
Review request changed
Status:
Completed
Change Summary:
Pushed to master (541abb5). Thanks!