Closed Bug 53966 Opened 21 years ago Closed 17 years ago

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

Categories

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

x86
Windows 98
defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: bugzilla, Assigned: roc)

References

(Blocks 2 open bugs, )

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached file reduced testcase
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?
Status: NEW → ASSIGNED
Target Milestone: --- → Future
dnd doesn't do any mouse capture.
Updating QA contact.
QA Contact: ckritzer → bsharma
also happens with the new Search button on netcenter
Summary: d&d is interfering with button behavior of input type="image" → [IMGBTN]d&d is interfering with button behavior of input type="image"
QA Contact Update
QA Contact: bsharma → vladimire
Priority: P3 → --
*** Bug 127451 has been marked as a duplicate of this bug. ***
Summary: [IMGBTN]d&d is interfering with button behavior of input type="image" → [IMGBTN]dragging from image button submits form (input type="image")
*** Bug 200969 has been marked as a duplicate of this bug. ***
Hey Jesse? You want to copy relevant ccs when you dup.... ;)
*** Bug 201603 has been marked as a duplicate of this bug. ***
*** Bug 219209 has been marked as a duplicate of this bug. ***
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.
Looks like some sort of mouse capture to me, despite comment 3.
My testcase from bug 219209 might have a remote chance of being of service:
attachment 131454 [details]
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++
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.
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...
Blocks: 211398
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?
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.
Removes the unnecessary view code plus a few unused fields and functions.
Assignee: rods → roc
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
:-)
Attachment #142498 - Flags: superreview?(bzbarsky)
Attachment #142498 - Flags: review?(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
Attachment #142498 - Flags: superreview?(bzbarsky)
Attachment #142498 - Flags: superreview+
Attachment #142498 - Flags: review?(bzbarsky)
Attachment #142498 - Flags: review+
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.
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.
So hy are we capturing at all?  What breaks if we don't?  (select-to-scroll, I
guess?)
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.
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).
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.
Depends on: 20022
Attached patch work in progress (obsolete) — Splinter Review
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.
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.
Blocks: 236438
Blocks: 237170
Does this cause bug 238492 too?
Note --- this won't be fixed for 1.7final. So you may want to investigate
workarounds.
Blocks: 238492
Hmm... That checkin may have caused bug 240903 
Attached patch fixSplinter Review
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.
Attachment #152106 - Flags: superreview?(dbaron)
Attachment #152106 - Flags: review?(dbaron)
Comment on attachment 152106 [details] [diff] [review]
fix

I don't know much about this code, but r+sr=dbaron.
Attachment #152106 - Flags: superreview?(dbaron)
Attachment #152106 - Flags: superreview+
Attachment #152106 - Flags: review?(dbaron)
Attachment #152106 - Flags: review+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 270683
You need to log in before you can comment on or make changes to this bug.