Bug 98007 – A Spin button should release mouse event capturing when a modal dialog opens

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=145489

If a web page opens alert() or something in a 'change' event handler of input[type=number] and a user press the spin button in it, a modal dialog opens but mouse events are still captured by the spin button. We should release mouse event capturing in such case.

[Kent Tamura]

Created attachment 166602 [details]
Patch

[Hajime Morrita]

Comment on attachment 166602 [details]
Patch

Is this frame local?

We could add a listener/observer on Chrome object for detecting willOpenMocalDialog(),
and let SpinButtonElement to implement that listener and register itself to Chrome object.
Adding a virtual method to Node only for this purpose looks a bit too abusive for me.

[Darin Adler]

Comment on attachment 166602 [details]
Patch

I don’t like the assumptions here. Not all clients will use modal dialogs for things like a JavaScript alert. And many clients will use modal dialogs in other situations, like for a file chooser in an input element. It would be better if we did not have to hardcode the assumption that a modal dialog will come up, but could instead respond to the fact that one did come up.

[Hajime Morrita]

(In reply to comment #3)
> (From update of attachment 166602 [details])
> I don’t like the assumptions here. Not all clients will use modal dialogs for things like a JavaScript alert. And many clients will use modal dialogs in other situations, like for a file chooser in an input element. It would be better if we did not have to hardcode the assumption that a modal dialog will come up, but could instead respond to the fact that one did come up.

Yeah, that is my point. If Chrome object can notify the fact, 
then native implementation which uses mousemove can register itself to
handle modal dialogs regardless who does it.

[Kent Tamura]

Comment on attachment 166602 [details]
Patch

Yeah, we need a generalized fix.

http://code.google.com/p/chromium/issues/detail?id=154329 ':hover isn't cleared for alert() or confirm()' is a similar problem.

[Kent Tamura]

Created attachment 172727 [details]
WIP

[Kent Tamura]

(In reply to comment #6)
> Created an attachment (id=172727) [details]
> WIP

I still need to add a function to Node in this new patch because EventHandler assumes m_capturingMouseEventsNode is Node.

> could instead respond to the fact that one did come up.

IMO, it's ok to cancel capturing on any events which *might* open dialogs/popups.

[Kent Tamura]

Created attachment 174066 [details]
Patch 2

[Hajime Morrita]

CC-ing haraken.
This addition might increase sizeof(Node) but I'm sure if it really does. 
Haraken, do you remember the bottom line of past discussion?

[Kentaro Hara]

Comment on attachment 174066 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=174066&action=review

> Source/WebCore/dom/Node.h:125
> +class Node : public EventTarget, public ScriptWrappable, public TreeShared<Node, ContainerNode>, public PopupOpeningObserver {

Yeah, this will increase sizeof(Node) by 8 byte, which wouldn't be acceptable.

A virtual function table pointer is needed for each multiply-inherited class that has virtual functions. Currently only EventTarget has virtual functions and thus each Node has only one virtual function table pointer. If you add PopupOpeningObserver, which has virtual functions, it will add one more virtual function table pointer to each Node.

[Kent Tamura]

Comment on attachment 174066 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=174066&action=review

>> Source/WebCore/dom/Node.h:125
>> +class Node : public EventTarget, public ScriptWrappable, public TreeShared<Node, ContainerNode>, public PopupOpeningObserver {
> 
> Yeah, this will increase sizeof(Node) by 8 byte, which wouldn't be acceptable.
> 
> A virtual function table pointer is needed for each multiply-inherited class that has virtual functions. Currently only EventTarget has virtual functions and thus each Node has only one virtual function table pointer. If you add PopupOpeningObserver, which has virtual functions, it will add one more virtual function table pointer to each Node.

I see.  I'll update the patch so that I don't change Node parents.

[Kent Tamura]

Created attachment 174088 [details]
Patch 3

Node doesn't inherit PopupOpeningObserver

[Hajime Morrita]

Comment on attachment 174088 [details]
Patch 3

This version is much better than before!

[WebKit Review Bot]

Comment on attachment 174088 [details]
Patch 3

Clearing flags on attachment: 174088

Committed r134886: <http://trac.webkit.org/changeset/134886>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.