53966 - [IMGBTN]dragging from image button submits form (input type="image")

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Blake Ross]

Build ID: just pulled

Steps to Reproduce:

(1) Go to http://www.kde.com/
(2) Mousedown on one of the Go image buttons
(3) Drag away from the image button and release the mousebutton

Actual Result:
Form submission, as if you had clicked on the button

Expected Result:
Nothing -- the image button should behave like all other buttons, such that 
dragging away from the button and releasing the mousebutton shouldn't trigger 
the button action (so as to give the user a way out once he/she has already 
begun clicking on the buttom)

I believe the problem here is that d&d is somehow interfering.  However, button 
behavior should override image d&d behavior in this case.

Comment 1

[rods (gone)]

Attachment: reduced testcase

Comment 2

[rods (gone)]

I think capturing is being turned on because the event state manager see the 
mouse up event coming from the image control element instead of from the html 
element. This click-drag-out-of-element-release-mouse seems to work fine for the 
submit button. What is the difference?  Mike, is capturing being turned on for 
images?

Comment 3

[Mike Pinkerton (not reading bugmail)]

dnd doesn't do any mouse capture.

Comment 4

[bsharma]

Updating QA contact.

Comment 5

[Blake Ross]

also happens with the new Search button on netcenter

Comment 6

[Vladimir Ermakov]

QA Contact Update

Comment 7

[Jesse Ruderman]

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

Comment 8

[Jesse Ruderman]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hey Jesse? You want to copy relevant ccs when you dup.... ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Hermann Schwab]

Have a look at testcase in bug 211398 :
Here is simple example for 'mousedown' event inside
DIV with style="position: absolute;
http://dwarf.mostcom.ru/~jimson/mousemove.html
Handler inside DIV receive 'mousemove' and 'mouseup' events even if
mouse pointer moved outside browser window (!)

It is a square showing the mouse coordinates if you are in it.
If you leave with mouse-down, the coordinates are still counting, even when you
are leaving the browser window, hovering over the browser´s chrome, or windows
desktop, or windows task bar, start button, tray.

Comment 13

[[e-mail]]

Looks like some sort of mouse capture to me, despite comment 3.

Comment 14

[Brian 'netdragon' Bober]

My testcase from bug 219209 might have a remote chance of being of service:
attachment 131454 [details]

Comment 15

[Brian 'netdragon' Bober]

I don't see any drag and drop occuring, btw. Compare it to, say an image link
from  http://www.google.com/

Its obvious there is a difference in mouse cursors. I agree it appears to be a
capture issue. I'm going to check it with spy++

Comment 16

[Brian 'netdragon' Bober]

It is a capture issue. I made the window small and moved the mouse around
outside the window, and spy++ showed nothing. I did the same after mouse downing
on the Google from attachment 131454 [details] and I was still getting the messages
outside the window. This didn't happen on images on http://www.google.com/

Not only that, but the cursor looks different on those two cases.

Comment 17

[Boris Zbarsky [:bzbarsky]]

See
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsImageControlFrame.cpp#270

roc, any idea why this is being done?  This looks like old troy code with little
in the way of explanations in the checkin comments...

Comment 18

[Boris Zbarsky [:bzbarsky]]

So... if I remove that view code, this bug goes away... unless I set an opacity
on the image or something.  Then it gets a view, of course, and this bug reappears.

roc, why's the view capturing the mouseup?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oooh boy. The nsImageControlFrame code was copied from nsHTMLButtonControlFrame,
or vice versa, I'm pretty sure. But the view creation in
nsHTMLButtonControlFrame::Reflow is #if 0'ed out with comments similar to what
you're observing here. They seemed to have added a view so to make mouse
grabbing work, but I think mouse grabbing works anyway. By the way, the
functions GetTranslatedRect in nsHTMLButtonControlFrame/nsImageControlFrame, and
their mTranslatedRect members, are unused. We should at least remove all that junk.

Here's what's happening when the button has a view. Clicking on the button goes
into nsFrame::HandlePress which always calls nsFrame::CaptureMouse:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrame.cpp#1375
That makes the enclosing view capture all mouse events. The user moves the mouse
outside the button and then releases. The release event is captured by the view
and passed to nsPresShell::HandleEvent. nsPresShell::HandleEvent tries to find
the frame containing the event point. In this case, no frame under the capturing
view contains the event, but the view manager (in a an attempt to make sure that
the event is handled *somewhere*) has set aForceHandle and so
nsPresShell::HandleEvent targets the event at the view's frame anyway, here:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsPresShell.cpp#5870
It's all downhill from there. The event dispatches, no-one checks that it's
actually in bounds, and eventually nsHTMLInputElement fires a submit.

The problem is this: we DO want the mouseup to be dispatched somewhere so that
DOM capturers and other capturers can see it. But where should it be targeted?
Should we capture at the nearest scrolling view or document root view instead of
at the nearest view of any kind? That would probably solve the problem because
scrollframes and root frames aren't going to react to a mouseup, but I wonder
what it might break.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: remove goop from nsImageControlFrame/nsHTMLButtonControlFrame

Removes the unnecessary view code plus a few unused fields and functions.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 142498 [details] [diff] [review]
remove goop from nsImageControlFrame/nsHTMLButtonControlFrame

You doing reviews again yet? :-) This one's all deletion so it hardly counts
:-)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142498 [details] [diff] [review]
remove goop from nsImageControlFrame/nsHTMLButtonControlFrame

>Index: layout/html/forms/src/nsImageControlFrame.cpp

Remove the following lines too?

 50 #include "nsIViewManager.h"
 54 #include "nsViewsCID.h"
 77 static NS_DEFINE_IID(kViewCID, NS_VIEW_CID);

Can probably make similar removals for the button.

This looks fine, but we should sort out the issue with views and events...
either here or in a separate clean bug.  r+sr=bzbarsky

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in. Let's finish the view mouse capturing problem here.

What do you think about this question?
> But where should it be targeted? Should we capture at the nearest scrolling view
> or document root view instead of at the nearest view of any kind?

Maybe David has an opinion.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See my old comment here:
http://bugzilla.mozilla.org/show_bug.cgi?id=34887#c46
and the following comment.

So provisionally I think capturing to the nearest scrolling view (or document
root if there is no scrolling view) is the right thing.

Comment 25

[Boris Zbarsky [:bzbarsky]]

So hy are we capturing at all?  What breaks if we don't?  (select-to-scroll, I
guess?)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. We need to capture at least so that select-to-scroll works.

I wonder if other frame types create a view for themselves because they want to
see captured mouse events.

Comment 27

[Boris Zbarsky [:bzbarsky]]

nsFramesetFrame does, probably unnecessarily.  See
http://lxr.mozilla.org/seamonkey/search?string=NS_VIEW_CID -- the only uses are
in nsHTMLContainerFrame::CreateViewForFrame, nsObjectFrame::CreateWidget, and
nsHTMLFramesetFrame::Init (nsFrameFrame defines the CID but never uses it).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll attach a work in progress patch in a minute. Unfortunately I think fixing
this properly requires a fix to 20022. Here's why: when you're dragging outside
a scrolling view that's capturing mouse events, and the autoscroll timer fires,
we need to figure which content inside the scrolling view the mouse "would" be
over so we can extend selection to that content. In the new world this doesn't
always work; for example on http://mozilla.org/start/, when the cursor is over
where the position:absolute main text would be and the timer fires, we do
GetFrameForPoint on the scrolled canvas frame. But
nsBlockFrame::GetFrameForPoint ignores absolute children because it assumes they
always have views and the view manager will dispatch events to them directly. So
when the autoscroll timer fires we really should be calling into the view
manager somewhow to figure out where the mouse really is (taking into account
z-index too). The upshot is that if you apply my patch and drag-scroll down
http://mozilla.org/start, it will scroll fine but it won't extend the selection
unless you move the mouse.

A fix to 20022 would fix this situation nicely. Then the autoscroll timer would
just scroll the window, and we would automatically generate a synthetic mouse
event which would go through the normal view manager event dispatch path and the
selection would extend.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: work in progress

This is fairly straighforward so far. One wrinkle is that we have to pass the
view we chose as a capture target into nsSelection::StartAutoScrollTimer,
because otherwise it assumes the view is the closest view for the frame and
things go haywire.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm also thinking of adding a method PRBool IsMouseCapturer() to nsIFrame so
that any frames which really want to capture the mouse via frame events can just
set that to TRUE. nsScrollBoxFrame would set it to TRUE. We'll automatically
force-create a view for such frames and GetCapturingView will choose the nearest
view whose frame has it set.

In a perfect world of course capturers would use DOM event capturing and we
wouldn't need this support in frame events.

Comment 31

[[e-mail]]

Does this cause bug 238492 too?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think so.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note --- this won't be fixed for 1.7final. So you may want to investigate
workarounds.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Hmm... That checkin may have caused bug 240903 

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

OK, this seems to work. The autoscrolling stuff seems to Just Work now that
dbaron's fix is in. I had to tidy up other stuff around the edges. The
testcases in the bugs that depend on this one seem to work.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 152106 [details] [diff] [review]
fix

I don't know much about this code, but r+sr=dbaron.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.