RESOLVED WORKSFORME 40082
[Qt] Dropdown box is seen twice in a webpage. 
https://bugs.webkit.org/show_bug.cgi?id=40082
Summary [Qt] Dropdown box is seen twice in a webpage.
Viatcheslav Ostapenko
Reported 2010-06-02 13:52:26 PDT
Easy to reproduce with QGraphicsWebView: load a page with a select list, open the dropdown, then switch windows and see the page again. The select combobox is drawn twice (possibly need to scroll the page to see the second one). Reproducible on ALL platforms when using the QGraphicsWebView. Drawing the dropdowns consist of two parts: drawing the combobox itself and drawing the dropdown (or popup as it’s called). When not using graphics view, Webkit renders the combobox itself via RenderTheme, and draws the popup using QComboBox::showPopup. When using QGraphicsWebView the combobox gets drawn twice because the popup is rendered via a QGraphicsProxyWidget that is wrapping a QComboBox. So, in graphics scene there is a graphics item having the QComboBox on top of the QGraphicsWebView, and the QGraphicsWebView has already drawn the combobox once. When clicking somewhere on the page, we are calling hidePopup() for the popup and proxy->setVisible(false) gets called specifically. However, when QGraphicsView loses the focus, popups are removed but the graphics item (combobox) itself is not. So, graphics view really works as expected. It’s not even supposed to remove any comboboxes added to it when the focus changes, just to close any open popups. It’s not possible to get focus events to QWebPopup so we cannot just call proxy->setVisible(false) when losing the focus. Anyway, I think the real problem is that we are overdrawing the combobox when using the proxy widget. One way to fix this would be to make sure we are not drawing twice. Just inherit from QGraphicsProxyWidget and overwrite the paint method and don’t draw anything there.
Attachments
Disable painting in QGraphicsWidget below combobox (3.52 KB, patch)
2010-06-02 17:27 PDT, Viatcheslav Ostapenko 		kenneth: review-
DetailsFormatted DiffDiff
Added better comment to the empty paint() (4.99 KB, patch)
2010-06-03 13:54 PDT, Viatcheslav Ostapenko 		kenneth: review-
DetailsFormatted DiffDiff
Patch update by comments. (4.84 KB, patch)
2010-06-14 15:50 PDT, Viatcheslav Ostapenko 		no flags
DetailsFormatted DiffDiff
Updated changelog. (4.83 KB, patch)
2010-06-14 15:56 PDT, Viatcheslav Ostapenko 		kenneth: review-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Viatcheslav Ostapenko
Comment 1 2010-06-02 17:27:24 PDT
Created attachment 57717 [details] Disable painting in QGraphicsWidget below combobox
Kenneth Rohde Christiansen
Comment 2 2010-06-02 20:41:36 PDT
Making the popup close as expected should solve the problem as well. Anyway, I think you need to add part of the description of the problem from the ChangeLog, in the actual code as well. From reading the code, I do not understand why the paint method does nothing. Reading the ChangeLog on the other hand, makes that quite clear.
Kenneth Rohde Christiansen
Comment 3 2010-06-02 20:43:59 PDT
Comment on attachment 57717 [details] Disable painting in QGraphicsWidget below combobox r- due to a required improvements to the comments for QFallbackWebPopupGraphicsProxyWidget::paint(). What 'second combo box' is, is not clear due to the lack of context.
Viatcheslav Ostapenko
Comment 4 2010-06-03 13:54:31 PDT
Created attachment 57809 [details] Added better comment to the empty paint()
Kenneth Rohde Christiansen
Comment 5 2010-06-04 12:43:06 PDT
Comment on attachment 57809 [details] Added better comment to the empty paint() We are getting there! I do not like the ChangeLog entry. First of all, it doesn't start by saying what the actual problem is (neither does the bug title, which just explains the solution), but instead explain very technically what is causing it and how you are fixing in. That part is good! When you write a ChangeLog please consider: what, why, how. What is the issue at hand/what are you implementing, why are you doing this, how are you doing/fixing it. > > +class QFallbackWebPopupGraphicsProxyWidget : public QGraphicsProxyWidget { I would not prefix this with Q* as this is not part of the our API, instead I would use Qt* Actually you are implementing it inside the QtFallbackWebPopup, which has class QtFallbackWebPopupCombo : public QComboBox { So the above is actually what you are wrapping. QtFallbackWebPopupComboProxyWidget would be fine with me. > +public: > + QFallbackWebPopupGraphicsProxyWidget(QGraphicsItem *parent = 0, Qt::WindowFlags wFlags = 0) : > + QGraphicsProxyWidget(parent, wFlags) {} Wrong indentation, and wrong placement of * > +/* > + Drawing the dropdowns consist of two parts: drawing the combobox itself and. > + drawing the dropdown (or popup as its called). When not using graphics view,. > + Webkit renders the combobox itself via RenderTheme, and draws the popup using. > + QComboBox::showPopup. When using QGraphicsWebView the combobox gets drawn twice. > + because the popup is rendered via a QGraphicsProxyWidget that is wrapping a QComboBox.. > + So, in graphics scene there is a graphics item having the QComboBox on top of. > + the QGraphicsWebView, and the QGraphicsWebView has already drawn the combobox once. > + > + When clicking somewhere on the page, we are calling hidePopup() for the popup and. > + proxy->setVisible(false) gets called specifically. However, when QGraphicsView loses. > + the focus, popups are removed but the graphics item (combobox) itself is not. So,. > + graphics view really works as expected. Its not even supposed to remove any comboboxes. > + added to it when the focus changes, just to close any open popups. Its not possible. > + to get focus events to QWebPopup so we cannot just call proxy->setVisible(false). > + when losing the focus. > + > + The real problem is that we are overdrawing the combobox when using the proxy widget. > + One way to fix this would be to make sure we are not drawing twice. Just inherit from. > + QGraphicsProxyWidget and overwrite the paint method and dont draw anything there. > +*/ > + void paint(QPainter*, const QStyleOptionGraphicsItem*, QWidget*) {} > +}; Comments inline in code usuablly uses // and not /* *** */. Please change that. > + > QtFallbackWebPopupCombo::QtFallbackWebPopupCombo(QtFallbackWebPopup& ownerPopup) > : m_ownerPopup(ownerPopup) > { > @@ -115,7 +143,7 @@ void QtFallbackWebPopup::show() > QRect rect = geometry(); > if (QGraphicsWebView *webView = qobject_cast<QGraphicsWebView*>(pageClient()->pluginParent())) { > if (!m_proxy) { > - m_proxy = new QGraphicsProxyWidget(webView); > + m_proxy = new QFallbackWebPopupGraphicsProxyWidget(webView); > m_proxy->setWidget(m_combo); > } else > m_proxy->setVisible(true);
Kenneth Rohde Christiansen
Comment 6 2010-06-04 12:48:35 PDT
[Qt] Disable drawing in combobox proxy widget in QGraphicsWebView mode https://bugs.webkit.org/show_bug.cgi?id=40082 That is NOT the title of this bug, which currently is called 'Dropdown box is seen twice in a webpage' That is actually a confusing title, as I understand from your changelog explanation, the issue is not the popup itself, but that the combobox is being drawn as it is being drawn by the proxywidget and by the RenderTheme.
Jocelyn Turcotte
Comment 7 2010-06-07 04:36:19 PDT
This patch cause a bug where I can't reopen the popup. On Windows I do these steps in QTestBrowser set in graphics web view mode: - Load http://www.google.com/advanced_search - Clic on a combobox to show the popup - While the popup is shown, focus on a different window (this should hide the popup) - Before clicking anywhere in the test browser, scroll it - Re-click on the same combobox I get the popup shown for less than a second then it disapear and I can't open this combobox anymore by clicking on it.
Viatcheslav Ostapenko
Comment 8 2010-06-10 16:22:28 PDT
(In reply to comment #7) > This patch cause a bug where I can't reopen the popup. > > On Windows I do these steps in QTestBrowser set in graphics web view mode: > - Load http://www.google.com/advanced_search > - Clic on a combobox to show the popup > - While the popup is shown, focus on a different window (this should hide the popup) > - Before clicking anywhere in the test browser, scroll it > - Re-click on the same combobox > > I get the popup shown for less than a second then it disapear and I can't open this combobox anymore by clicking on it. Actually, we managed to reproduce this even without patch. With patch it happens a more often. Should be some timing issue - otherwise I cannot imaging, how empty paint method can cause this.
Viatcheslav Ostapenko
Comment 9 2010-06-14 15:50:33 PDT
Created attachment 58716 [details] Patch update by comments.
Viatcheslav Ostapenko
Comment 10 2010-06-14 15:56:43 PDT
Created attachment 58719 [details] Updated changelog.
Kenneth Rohde Christiansen
Comment 11 2010-12-24 02:38:48 PST
Comment on attachment 58719 [details] Updated changelog. View in context: https://bugs.webkit.org/attachment.cgi?id=58719&action=review The comment is very very detailed and not get out of touch with the code. In WebKit we like comments to be concise and to the point. Btw, is this still an issue? Can it be fixed in Qt? > WebKit/qt/WebCoreSupport/QtFallbackWebPopup.cpp:72 > + void paint(QPainter*, const QStyleOptionGraphicsItem*, QWidget*) {} Needs space between { and }. WebKit coding style.
Viatcheslav Ostapenko
Comment 12 2010-12-24 12:03:23 PST
(In reply to comment #11) > (From update of attachment 58719 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=58719&action=review > The comment is very very detailed and not get out of touch with the code. In WebKit we like comments to be concise and to the point. > Btw, is this still an issue? Can it be fixed in Qt? Not reproducible anymore with qt 4.7.1 and webkit trunk.
Note You need to log in before you can comment on or make changes to this bug.