Closed Bug 53966 Opened 21 years ago Closed 17 years ago
[IMGBTN]dragging from image button submits form (input type="image")
464 bytes, text/html
9.87 KB, patch
|Details | Diff | Splinter Review|
18.94 KB, patch
|Details | Diff | Splinter Review|
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.
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
*** 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...
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 :-)
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
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
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.
18 years ago
Priority: -- → P2
18 years ago
Does this cause bug 238492 too?
I think so.
Note --- this won't be fixed for 1.7final. So you may want to investigate workarounds.
Hmm... That checkin may have caused bug 240903
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 #142541 - Attachment is obsolete: true
Comment on attachment 152106 [details] [diff] [review] fix I don't know much about this code, but r+sr=dbaron.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.