150297 - Command-Click should drag-scroll

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement, P4)

Description

[Ben Medina]

In IE (and Opera in OS 9), Command-Clicking on web pages changes the cursor to
the "grabby hand," allowing you to scroll the window simply by moving the cursor
around. This is very classic Mac behavior, dating back to the System 6 days, and
is one of the few things I still envy about IE users. It is also very handy for
those who use graphics tablets, as it allows for very easy scrolling.

Comment 1

[cor bently]

I vote for this bug! - cor

Comment 2

[saari (gone)]

yeah, would be a nice feature, but low priority just this second

->pinkerton

Comment 3

[Lauri Kieksi]

Side note, the functionality exists in the Mac OS X Finder under
Command-Option-Click&Drag (as opposed to OS 9's Command-Click&Drag). This may be
related to the changed Command-Click behavior in OS X (now does what Shift-Click
used to do). I'm not sure if this means that Chimera's drag-scrolling should use
Command-Option-Click&Drag, but it's something to consider. Assuming the feature
is still planned.

Comment 4

[Greg K.]

Note that it seems to work in Finder icons and list views, but not in column view.

Comment 5

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Greg, I think that's because there's no insignificant whitespace in Column view.

Comment 6

[Steve Smart]

*** Bug 175464 has been marked as a duplicate of this bug. ***

Comment 7

[Sören 'Chucker' Kuklau (gone)]

> I'm not sure if this means that Chimera's drag-scrolling should use
> Command-Option-Click&Drag

Unless Apple's HIGs say otherwise, it should be Command-Option-Click&Drag. Mac
OS 9 is out of context with Chimera.

Comment 8

[Nathan Hamblen]

Attachment: patches cocoa childview widget to handle drag scrolling

Expects to find ClosedGrabHandCursor.tiff and OpenGrabHandCursor.tiff as
resources in the main bundle.

Comment 9

[Nathan Hamblen]

Attachment: Closed hand cursor for drag scolling

Lifted from projectbuilder's frameworks.

Comment 10

[Nathan Hamblen]

Attachment: Open hand cursor for drag scrolling

Comment 11

[S Woodside]

can't wait :-)

Comment 12

[S Woodside]

I see some problems with this patch. First, what happens if the user intended to
cmd-click on a link? This code will block that from working unless it gets
trapped at a higher level. In addition, using a loop makes me nervous, it's not
really the "Cocoa way" to do it ... better instead to use mouseDragged instead,
which is the message sent when the mouse is down and being dragged around. For
one thing this patch is going to spin intensively doing nothing, not a very good
concurrent design (better to use nextEventMatchingMask at least which will block
even).

Take a look at my code in (FractalView.mm) mouseDown:/mouseDragged:/mouseUp: in
http://freshmeat.net/projects/fractaltrees/ for an implementation based on what
I'm talking about.

Comment 13

[Nathan Hamblen]

Command click on a link still works. My code is only activated when exactly
command + option are pressed. (Even caps lock prevents activation -- oops!).  

No intensive CPU spinning here, I *do* use nextEventMatchingMask. I got the idea
(ok, the code really) from
http://developer.apple.com/techpubs/macosx/Cocoa/TasksAndConcepts/ProgrammingTopics/BasicEventHandling/Tasks/HandlingMouseEvents.html
so I think that makes it pretty cocoa friendly. I thought about using the
mouseDragged message, but that wouldn't let me track outside the window or frame
that I am in, as I understand it.

Comment 14

[S Woodside]

Hmm... cmd-drag seems more logical and easier, but whatever. I wouldn't worry
about command-clicking to draw a box around a table box, what use is that ??
Besides, if you support Cmd-Click you'll have to pass through normal clicks
anyway, so that will still work too.

As far as the model for dragging, I used the one given in
Examples/AppKit/CircleView for my app. Yes, it does work if you drag out of the
view (although that seems a little odd, well it's smart :-) Honestly it seems
cleaner to me to do it with mouseDragged. One question about doing it with a
loop: what happens if there are views/windows/tabs drawing in the background,
will they be blocked? Any other events that should go through that you will just
throw away?

Comment 15

[Nathan Hamblen]

Excellent question! I checked it out and the hand drag does block all layout
activities and quicktime movies playing for as long as the mouse is down, even
in other windows. I was about to throw up my hands and scream "Why me, cruel
fate!" (because I've spent so long on this implementation alone, mostly getting
to know the Chimera and Mozilla codebase) but then I decided to try using the
scrollbars. Same thing! Wow! Is there a bug on this? It's pretty annoying to
have a qt movie playing in another window and have it stop audio because I'm
dragging the scrollbars in a foreground window.

Anyway, I will see if I can easily fix it for hand scrolling. Maybe mouseDragged
is the answer. But even if I can't fix it, at least it will be no worse than the
scroll bar implementation. (Which might use the same kind of event loop -- wish
I cared enough to find out.) Performing at least as well as standard scrolling
is good enough for me.

Comment 16

[Mike Pinkerton (not reading bugmail)]

yes, there is a bug on this for scrollbars. the solution is non-trivial. the
trivial solution (make the scrollbar use a separate runloop for its tracking)
can cause you to crash if you start loading a new page but before it loads, grab
the thumb and hold on while the new page replaces the old. now moving the thumb
crashes because it's trying to scroll views that aren't there any more. There's
no way to alert the scrollbar to "let go" via any APIs of which i'm aware.

this may or may not have a similar problem, i'm not sure.

Comment 17

[Nathan Hamblen]

Attachment: updated patch to nsChildView to handle drag scrolling

The attached cursor image resources are no longer used or required as I found
that they are  already embedded somewhere in Chimera.

I switched from using a modal event loop to responding to mouseDragged and
mouseUp so that layout, quicktime, etc are not interupted.

The open hand cursor should now only display when it is over a content area. I
seemed to be replacing some code that someone once wrote or thought about
writing to monitor the mouse entering and leaving the content views...? So now
all the ChildView instances monitor NSRects that mirror their bounds. Well
anyway it WFM.

The anchor point is now locked at the mousedown position, like IE, because it
works better and helps to avoid slow diagonal dragging.

Comment 18

[S Woodside]

Nathan: have you tested the situation pink described in comment 16?

Comment 19

[S Woodside]

I was just thinking about comment 16, and it sounds like an architectural
problem. I don't think you should be receiving mouseDragged messages for a view
that isn't there anymore. In other words, when you grab the scroll thumb, the
moment that the scrollview you're controlling goes away, the drag should be
disassociated from the thumb (which should also go away and be replaced by a new
one) and you should stop receiving events for that view. It sounds like the way
that views are set up is a little wacky. 

The problem here is that the view that's receiving the mouseDragged messages
isn't the scroll view. If it was, there would be no problem. The solution should
be to fix that, instead of (for example) constantly checking if the view is
still there for every mouseDragged event. It's a hackish idea and probably
fragile/brittle as well to changes later.

What's the bug #? Where's the relevant code?

Comment 20

[Nathan Hamblen]

Just tried the setup from comment 16 and it didn't blow up. After the new page
loads I still have the grabbing hand but it doesn't scroll anything or crash the
browser.

Comment 21

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Do objects with onClick handlers still work OK?

Comment 22

[Nathan Hamblen]

Yes, all clicks (and events in general) go through as normal unless command +
option are pressed. If they are pressed, the onClick (javascript, right?)
handler wouldn't get the called.

Comment 23

[S Woodside]

*ping*

Has this been tested on the latest cvs by more than one person?

It would be nice to get this into a nightly before it gets stale.

Comment 24

[S Woodside]

found an issue. On this bug page, narrow the window enough such that you get
both horiz and verti scroll bars. Click in the Additional Comments field such
that the text entry cursor is blinking there. Now, cmd-opt-drag scroll and I'm
observing wacky horizontal "oscillations".

Comment 25

[S Woodside]

Other than that, BTW, it's working great for me. And it's very cool and useful
too ;-)

Comment 26

[Nathan Hamblen]

You're right, diagonal scrolling gets EVEN WORSE when the cursor is in a text
field. Weird. Not my fault! (At least I don't see how it could be.) Other
issues: 1) no workie if caps lock is down 2) Some plugins steal the
mousedown event so you cant grab within their bounds 3) Grabbing in an IFRAME
scrolls the IFRAME, but textfields and multiselects scroll their parent view. 
These are all minor and don't interfere with me using it all the time. I'd be
inclined to fix #1 at least, but I don't know if this thing is ever going into
the tree.

Comment 27

[S Woodside]

diagonal scrolling for "view image" pages works great (no jerkiness at all).
Also, that's the most important use case IMHO.

Comment 28

[Mike Pinkerton (not reading bugmail)]

-    
+  float                 mTwipsToPixels;
+  float                 mPixelsToTwips;

do we want to add these to every single nsChildView? I don't see a lot of need
for that.

+    nsISupports* data = (nsISupports*)clientData;
+    data->QueryInterface(NS_GET_IID(nsIScrollableView), (void **)&mScrollableView);

as this goes through the loop, it will leak the scrollview several times. QI
addrefs. Also, you never release that reference. Dunno if this should be a
comptr, but you definately need to fix the manual refcounting you're (not)
doing. You should also check that data is non-null just to be safe before you
deref it.

 - (void)setFrame:(NSRect)frameRect
 {  
   [super setFrame:frameRect];
+  if (mMouseEnterExitTag)
+    [self removeTrackingRect:mMouseEnterExitTag];
+
+  mMouseEnterExitTag = [self addTrackingRect:[self bounds] owner:self
+                                    userData:nil assumeInside: YES];
 }

can you wait until the hand-scroll starts before you create this tracking rect?
won't it impact performance even when there's no scrolling?

+}
+// update the scroll position based on the new mouse coordinates
+- (void) updateHandScroll:(NSEvent*)theEvent {

stylistic comment. put the bracket for a function on a line by itself. also, put
a blank line between the close bracket of the last function and the comment.

Comment 29

[Simon Fraser [no longer active]]

> +    nsISupports* data = (nsISupports*)clientData;
> +    data->QueryInterface(NS_GET_IID(nsIScrollableView), (void
**)&mScrollableView);
> 
> as this goes through the loop, it will leak the scrollview several times. QI
> addrefs. Also, you never release that reference. Dunno if this should be a
> comptr, but you definately need to fix the manual refcounting you're (not)
> doing.

Views are not refcounted. So this is OK. But I'm not sure we want to stack a ref
to the mScrollableView; it's pretty cheap to get.

Comment 30

[Nathan Hamblen]

Attachment: New hand drag scroll patch based on comments from Pinkerton and Fraser

Thanks for the review guys. 

- I've removed the twip conversion factors (Mike) and also the scrollable view
pointer (Simon) from the class / interface. I'm fetching them as needed now. 

- Mike, I have to have an active tracking rect all the time because the user
could be holding down the modifier keys when entering the content area. I don't
know if I would get the flagsChanged event if the keys were pressed while the
mouse pointer was outside of the content area. Besides, I don't think that
processing a message every time the mouse enters and leaves the content area is
a big deal, especially cosidering that mouseMoved messages are processed and
converted to gecko events every time the moves (MOVES!) inside the content
area. ;)

- I fixed the modifier checking code to ignore the caps lock state.

Comment 31

[Nathan Hamblen]

Comment on attachment 116839 [details] [diff] [review]
New hand drag scroll patch based on comments from Pinkerton and Fraser

Oops, darn header "nsIScrollableView.h" still in there. I'll take care of that
later.

Comment 32

[Mike Pinkerton (not reading bugmail)]

landed, but there's a minor focus problem. followup bug filed

 http://bugzilla.mozilla.org/show_bug.cgi?id=197230

and assigned to Nathan directly. Thanks for the great work!

Comment 33

[Jasper]

This patch seems to be busted under 10.3 panther.

Comment 34

[Ben Medina]

I can confirm that it's busted in 10.3.

Comment 35

[Mike Pinkerton (not reading bugmail)]

if this is a problem on panther, please open a new bug so we can track it.