Bug 35259 - [Qt] QGraphicsScene mousePressEvent does not set the clickCausedFocus flag
Summary: [Qt] QGraphicsScene mousePressEvent does not set the clickCausedFocus flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All S60 3rd edition
: P2 Minor
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-02-22 13:37 PST by Joseph Ligman
Modified: 2010-03-30 13:11 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (2.11 KB, patch)
2010-03-30 10:21 PDT, Joseph Ligman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Ligman 2010-02-22 13:37:17 PST
Probably not too likely, but this flag not being set has impact on clients using QGraphicsWebView and QStyle::RSIP_OnMouseClickAndAlreadyFocused. And there are no tests for these flags, which would be good to add as well.
Comment 1 Kent Hansen 2010-03-17 08:08:21 PDT
Hi Joseph,
Could you add a testcase that demonstrates the issue? Thanks.
Comment 2 Joseph Ligman 2010-03-17 08:34:21 PDT
Hi Kent, 

This is probably a minor issue. If you look at the following lines in qwebpage.cpp, you can see the issue. To me it would make sense to remove the handleSoftwareInputPanel altogether and just let the clients request it when needed.

QGraphicsSceneMouseEvent
771	void QWebPagePrivate::mousePressEvent(QGraphicsSceneMouseEvent* ev)
772	{
773	    WebCore::Frame* frame = QWebFramePrivate::core(mainFrame);
774	    if (!frame->view())
775	        return;
776	
777	    if (tripleClickTimer.isActive()
778	            && (ev->pos().toPoint() - tripleClick).manhattanLength()
779	                < QApplication::startDragDistance()) {
780	        mouseTripleClickEvent(ev);
781	        return;
782	    }
783	
784	    bool accepted = false;
785	    PlatformMouseEvent mev(ev, 1);
786	    // ignore the event if we can't map Qt's mouse buttons to WebCore::MouseButton
787	    if (mev.button() != NoButton)
788	        accepted = frame->eventHandler()->handleMousePressEvent(mev);
789	    ev->setAccepted(accepted);
790	}


QMouseEvent 
792	void QWebPagePrivate::mousePressEvent(QMouseEvent *ev)
793	{
822	    if (newNode && oldNode != newNode)
823	        clickCausedFocus = true;
824	}



928	void QWebPagePrivate::handleSoftwareInputPanel(Qt::MouseButton button)
929	{
930	#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
931	    Frame* frame = page->focusController()->focusedFrame();
932	    if (!frame)
933	        return;
934	
935	    if (client && client->inputMethodEnabled()
936	        && frame->document()->focusedNode()
937	        && button == Qt::LeftButton && qApp->autoSipEnabled()) {
938	        QStyle::RequestSoftwareInputPanel behavior = QStyle::RequestSoftwareInputPanel(
939	            client->ownerWidget()->style()->styleHint(QStyle::SH_RequestSoftwareInputPanel));
940	        if (!clickCausedFocus || behavior == QStyle::RSIP_OnMouseClick) {
941	            QEvent event(QEvent::RequestSoftwareInputPanel);
942	            QApplication::sendEvent(client->ownerWidget(), &event);
943	        }
944	    }
945	
946	    clickCausedFocus = false;
947	#endif
948	}
Comment 3 Simon Hausmann 2010-03-17 08:46:10 PDT
You're right, that's indeed missing
Comment 4 Joseph Ligman 2010-03-30 10:21:19 PDT
Created attachment 52051 [details]
Proposed patch
Comment 5 Antonio Gomes 2010-03-30 10:27:40 PDT
(In reply to comment #4)
> Created an attachment (id=52051) [details]
> Proposed patch

I think it looks good, but code could be improved.

you do "page->focusController()->focusedFrame()" four times. It would be stored in aux vars, so it would like nicer.
Comment 6 Antonio Gomes 2010-03-30 10:29:47 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=52051) [details] [details]
> > Proposed patch
> 
> I think it looks good, but code could be improved.
> 
> you do "page->focusController()->focusedFrame()" four times. It would be stored
> in aux vars, so it would like nicer.

actually 6 times :)

also, here

+    RefPtr<WebCore::Node> oldNode;

are you sure you want to keep hold a reference just to compare it later one? 
it can be destroyed when unfocused, no?
Comment 7 Kenneth Rohde Christiansen 2010-03-30 10:33:45 PDT
> +    RefPtr<WebCore::Node> oldNode;
> 
> are you sure you want to keep hold a reference just to compare it later one? 
> it can be destroyed when unfocused, no?

He is not, they get out of scope after the method ends.
Comment 8 WebKit Commit Bot 2010-03-30 13:11:00 PDT
Comment on attachment 52051 [details]
Proposed patch

Clearing flags on attachment: 52051

Committed r56805: <http://trac.webkit.org/changeset/56805>
Comment 9 WebKit Commit Bot 2010-03-30 13:11:05 PDT
All reviewed patches have been landed.  Closing bug.