Bug 29389 - [Qt] isAccepted() from mouse event handling has a strange behavior on buttons of google.com
Summary: [Qt] isAccepted() from mouse event handling has a strange behavior on buttons...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Minor
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2009-09-18 07:14 PDT by Tor Arne Vestbø
Modified: 2011-04-13 15:46 PDT (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-09-18 07:14:42 PDT
This bug report originated from issue QTBUG-2959
<http://bugreports.qt.nokia.com/browse/QTBUG-2959>

--- Description ---

For webkit page showing www.google.com, clicking in the empty white areas produces mouse events that QWebPage marks as isAccepted(), whereas clicking on links and buttons produces mouse events that QWebPage marks as !isAccepted().

This seems barkwards.
Comment 1 Joseph Ligman 2009-10-21 14:31:13 PDT
I can look at this bug. Not sure what the procedure is for unassigned bugs.
Comment 2 Ariya Hidayat 2010-02-12 19:18:04 PST
Joseph, any progress on this?
Comment 3 Joseph Ligman 2010-02-15 13:11:38 PST
(In reply to comment #2)
> Joseph, any progress on this?

No, sorry.
Comment 4 Robert Hogan 2010-03-08 13:09:36 PST
(In reply to comment #0)
> This bug report originated from issue QTBUG-2959
> <http://bugreports.qt.nokia.com/browse/QTBUG-2959>
> 
> --- Description ---
> 
> For webkit page showing www.google.com, clicking in the empty white areas
> produces mouse events that QWebPage marks as isAccepted(), whereas clicking on
> links and buttons produces mouse events that QWebPage marks as !isAccepted().
> 
> This seems barkwards.

I think it's correct - insofar as WebCore seems to be only interested in accepting mousePressEvents that initiate or manage a selection. Clicking links and buttons can't be the start of a selection in practice, no matter how hard you try. ;-)

bool EventHandler::handleMousePressEvent(const MouseEventWithHitTestResults& event)
{
<..>
    // If we got the event back, that must mean it wasn't prevented,
    // so it's allowed to start a drag or selection.
    m_mouseDownMayStartSelect = canMouseDownStartSelect(event.targetNode());
<..>
    bool swallowEvent = false;
    m_frame->selection()->setCaretBlinkingSuspended(true);
    m_mousePressed = true;
    m_beganSelectingText = false;

    if (event.event().clickCount() == 2)
        swallowEvent = handleMousePressEventDoubleClick(event);
    else if (event.event().clickCount() >= 3)
        swallowEvent = handleMousePressEventTripleClick(event);
    else
        swallowEvent = handleMousePressEventSingleClick(event);

    return swallowEvent;

}

// Whether or not a mouse down can begin the creation of a selection.  Fires the selectStart event.
bool EventHandler::canMouseDownStartSelect(Node* node)
{
    if (!node || !node->renderer())
        return true;
    
    // Some controls and images can't start a select on a mouse down.
    if (!node->canStartSelection())
        return false;
            
    for (RenderObject* curr = node->renderer(); curr; curr = curr->parent()) {
        if (Node* node = curr->node())
            return node->dispatchEvent(Event::create(eventNames().selectstartEvent, true, true));
    }

    return true;
}

bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestResults& event)
{
    Node* innerNode = event.targetNode();
    if (!(innerNode && innerNode->renderer() && m_mouseDownMayStartSelect))
        return false;
<..>

}
Comment 5 Yael 2010-03-08 13:38:16 PST
This is going to be a problem for Orbit.
In Orbit, if you don't accept the MousePress event, you will not receive MouseMove or MouseRelease events.
Comment 6 Benjamin Poulain 2011-01-10 18:28:04 PST
Input element of button types behave as you would expect. The page google.com is not using the default behavior.
I have no idea what is going there.
Comment 7 Rafael Brandao 2011-04-06 12:32:39 PDT
Could anyone provide an example that does not behave like that? I could reproduce it everywhere I tried. It's just like Robert said, it's only interested on such events that could start selection, so a button may not behave like that.
Comment 8 Rafael Brandao 2011-04-07 17:51:21 PDT
QWebPage::event returns true if it has any function that handles that kind of event, no matter if the event has been accepted or not by the event handler:

bool QWebPage::event(QEvent *ev)
{
    switch (ev->type()) {
    case QEvent::Timer:
        d->timerEvent(static_cast<QTimerEvent*>(ev));
        break;
<..>
    default:
        return QObject::event(ev);
    }

    return true;
}

So I suppose it won't propagate this event to the parents. How could this flag isAccepted() make any difference in this case? Another thing I would like to ask, if QWebPage handle the event and then mark it as ignored to allow any parents to keep track of that event as well, how bad this could be?
Comment 9 Rafael Brandao 2011-04-12 12:22:41 PDT
QGraphicsWebView and QWebView ignores the flag set by QWebPage as you can see above:

void QGraphicsWebView::mousePressEvent(QGraphicsSceneMouseEvent* ev)
{
    if (d->page) {
        const bool accepted = ev->isAccepted(); // Stores the original value.
        d->page->event(ev); // Handle the event with QWebPage.
        ev->setAccepted(accepted); // Restores the original accepted value.
    }

    if (!ev->isAccepted())
        QGraphicsItem::mousePressEvent(ev);
}


void QWebView::mousePressEvent(QMouseEvent* ev)
{
    if (d->page) {
        const bool accepted = ev->isAccepted();
        d->page->event(ev);
        ev->setAccepted(accepted);
    }
}

QWebView in particular never propagates the event, so I can't see how this bug could affect anything at all.
Comment 10 Rafael Brandao 2011-04-13 12:36:32 PDT
In order to use QWebPage in a context that propagating mouse events could be relevant would be if you use it as a QWidget or a QGraphicsItem (correct me if I am wrong). Then you get QWebView and QGraphicsWebView.

Both of them ignore what QWebPage does with the event's accept flag as I've shown before, and they always accept the event, no matter where or what you click, as long as you click inside its area.

So what's the real point of changing the QWebPage's mousePressEvent accept flag in particular without changing how QWebView or QGraphicsWebView manages that change? Then I've checked the changelog looking for anything that would explain why QWebView always accept the event, and I've got this one:

2008-10-23  Simon Hausmann  <hausmann@webkit.org>

        Reviewed by Tor Arne.

        Fix handling of mouse events when embedding QWebView into the
        QGraphicsView.

        QWebPage accepts or ignores events to indicate whether the web page
        handled the event. However for QWebView to behave like a good widget
        it should always accept the mouse events to indicate that it handled
        them and that they should not be subject to event propagation.

        The graphics view relies on acceptance of the initial mouse click to
        make the embedded widget the focus item.

        * Api/qwebview.cpp:
        (QWebView::mouseMoveEvent):
        (QWebView::mousePressEvent):
        (QWebView::mouseDoubleClickEvent):
        (QWebView::mouseReleaseEvent):
        (QWebView::contextMenuEvent):
        (QWebView::wheelEvent):

If I got this right, it is expected that both QWebView and QGraphicsWebView accept that click event when it gets there.

Maybe the most appropriated bug fix for this one is to always accept events like mousePressEvent with QWebPage, as its widget (QWebView) or graphics item (QGraphicsWebView) does, so they would behave the same way and this would be more consistent.

Any opinions or observations are most certainly welcome.
Comment 11 Rafael Brandao 2011-04-13 15:46:12 PDT
After a discussion on #qtwebkit, it was raised that this change should not be done as Apple bubbling events interest may be different and it could fail into some pretty uncommon test cases.