Closed
Bug 53966
Opened 24 years ago
Closed 20 years ago
[IMGBTN]dragging from image button submits form (input type="image")
Categories
(Core :: Layout: Form Controls, defect, P2)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: bugzilla, Assigned: roc)
References
(Blocks 2 open bugs, )
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
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.
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
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
Comment 3•24 years ago
|
||
dnd doesn't do any mouse capture.
Reporter | ||
Comment 5•24 years ago
|
||
also happens with the new Search button on netcenter
Updated•24 years ago
|
Summary: d&d is interfering with button behavior of input type="image" → [IMGBTN]d&d is interfering with button behavior of input type="image"
Updated•23 years ago
|
Priority: P3 → --
Comment 7•23 years ago
|
||
*** Bug 127451 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Summary: [IMGBTN]d&d is interfering with button behavior of input type="image" → [IMGBTN]dragging from image button submits form (input type="image")
Comment 8•22 years ago
|
||
*** Bug 200969 has been marked as a duplicate of this bug. ***
Comment 9•22 years ago
|
||
Hey Jesse? You want to copy relevant ccs when you dup.... ;)
Comment 10•22 years ago
|
||
*** Bug 201603 has been marked as a duplicate of this bug. ***
Comment 11•21 years ago
|
||
*** Bug 219209 has been marked as a duplicate of this bug. ***
Comment 12•21 years ago
|
||
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•21 years ago
|
||
Looks like some sort of mouse capture to me, despite comment 3.
Comment 14•21 years ago
|
||
My testcase from bug 219209 might have a remote chance of being of service:
attachment 131454 [details]
Comment 15•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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
Comment 18•21 years ago
|
||
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?
Assignee | ||
Comment 19•21 years ago
|
||
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.
Assignee | ||
Comment 20•21 years ago
|
||
Removes the unnecessary view code plus a few unused fields and functions.
Assignee: rods → roc
Assignee | ||
Comment 21•21 years ago
|
||
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 22•21 years ago
|
||
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+
Assignee | ||
Comment 23•21 years ago
|
||
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.
Assignee | ||
Comment 24•21 years ago
|
||
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•21 years ago
|
||
So hy are we capturing at all? What breaks if we don't? (select-to-scroll, I
guess?)
Assignee | ||
Comment 26•21 years ago
|
||
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•21 years ago
|
||
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).
Assignee | ||
Comment 28•21 years ago
|
||
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
Assignee | ||
Comment 29•21 years ago
|
||
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.
Assignee | ||
Comment 30•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Priority: -- → P2
Comment 31•21 years ago
|
||
Does this cause bug 238492 too?
Assignee | ||
Comment 32•21 years ago
|
||
I think so.
Assignee | ||
Comment 33•21 years ago
|
||
Note --- this won't be fixed for 1.7final. So you may want to investigate
workarounds.
Comment 34•21 years ago
|
||
Hmm... That checkin may have caused bug 240903
Updated•21 years ago
|
Assignee | ||
Comment 35•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #142541 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #152106 -
Flags: superreview?(dbaron)
Attachment #152106 -
Flags: review?(dbaron)
Comment 36•20 years ago
|
||
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+
Assignee | ||
Comment 37•20 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•