In Qt, the QWebView widget ignores a custom palette set with QWebView::setPalette(), but does use a custom palette set with QApplication::setPalette(). QWebView should use the palette set specifically for it. There is no reason it can't. For instance (code written from memory, may not compile): QPalette palette; QBrush brush( QColor( 170, 3, 148, 255 ) ); // deep purple brush.setStyle( Qt::SolidPattern ); palette.setBrush( QPalette::Active, QPalette::Highlight ); webview->setPalette( palette ); This should cause QWebView to use a deep shade of purple for highlighted text. It doesn't. This on the other hand, does work: QPalette palette; QBrush brush( QColor( 170, 3, 148, 255 ) ); // deep purple brush.setStyle( Qt::SolidPattern ); palette.setBrush( QPalette::Active, QPalette::Highlight ); qApp->setPalette( palette ); Which means that the current style is not overriding anything. Analyzing the RenderThemeQt.cpp file in webkit sources clearly shows that the custom widget palette is being ignored. The color is read directly from the application palette. This is all on Win7 x64, Qt 4.5.2. I don't know which version of webkit that is so I just left the default.
Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
If you could provide a testcase, that would be great.
Created attachment 50724 [details] Palette test case Here is the test case.
I can reproduce this.
*** Bug 29480 has been marked as a duplicate of this bug. ***
*** Bug 35649 has been marked as a duplicate of this bug. ***
The problem is in RenderThemeQt.cpp . It takes palette from widget which comes in PaintInfo, but not from webpage.
Created attachment 80671 [details] Use QWebView custom palette if application provides one
I think some of those could be tested with autotest (selection color).
Attachment 80671 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 80934 [details] Reworked patch to remove tab from ChangeLog
Tried to add qt autotest to tst_qwebview (setPalette test) but had problems getting visual confirmation that fix is working. Basic approach to test was set the highlight palette on the view of a page containing text with same color as highlight, set the page, trigger select all and confirm that all pixels were same color. Even if I would be able to confirm it is working from visual inspection, not sure on what condition I assert to report the test result. I can confirm that original attached test case is working, however, and no regressions in layout tests.
(In reply to comment #12) > Even if I would be able to confirm it is working from visual inspection, not sure on what condition I assert to report the test result. What is done in those cases is rendering the selected element to QImage, and check the pixel values. For example, look at tst_QWebElement::render(). I think this is a good opportunity to make the test. You will learn how to make rendering test for Qt. And WebKit will enjoy better test coverage :)
Aha, I see. Let me try that, thanks for the description on how to do it.
Created attachment 81387 [details] Added api tests to verify fix, minor cleanup based on check-webkit-style results
Created attachment 81388 [details] Update ChangeLogs
Attachment 81388 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/qt/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 81389 [details] fix style check failure: remove tab from ChangeLog
Attachment 81389 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/qt/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 81390 [details] more tab removals
Ping for review, anyone?
Comment on attachment 81390 [details] more tab removals View in context: https://bugs.webkit.org/attachment.cgi?id=81390&action=review The test needs some rework to make it simpler and test one feature at the time. > Source/WebCore/platform/qt/RenderThemeQt.cpp:1435 > { > - return QApplication::cursorFlashTime() / 1000.0 / 2.0; > + return QApplication::cursorFlashTime() / 1000.0 / 2.0; > } This is unrelated. You should a separate patch for this. > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:34 > -{ > +class tst_QWebView : public QObject { You should fix the style in a separate patch. > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:325 > + // Test 1: BEGIN: Selection background color on active QWebView You should split the test in multiple, simpler functions. Each testing one aspect of the patch. > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:349 > + QString strActiveRedBg="activeRedBg.png"; > + imgActiveRedBg.save(strActiveRedBg); Why do you save the image in a test?
Thanks for the feedback. The images were saved for debugging purposes only. I wanted to visually inspect the impact of setting the custom palette. I can remove those in the final patch. Should get another patch in tonight to address your concerns.
Created attachment 81897 [details] rework to address comment #22 Remove style fixes (should be done in another bug/patch), divide setPalette() test into smaller test functions, remove image save from original patch.
Created attachment 81992 [details] fix test names
Comment on attachment 81992 [details] fix test names View in context: https://bugs.webkit.org/attachment.cgi?id=81992&action=review Looks good to me, we should honor the palette enforced on the QWebView. > Source/WebCore/ChangeLog:5 > + Use custom QWebView palette if application provides one. You need to prefix with [Qt] as stated in https://trac.webkit.org/wiki/QtWebKitContrib. > Source/WebKit/qt/ChangeLog:5 > + Tests for setting custom palette on highlighted You need to prefix with [Qt] as stated in https://trac.webkit.org/wiki/QtWebKitContrib. > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:317 > +// and inactive QWebView and highlighted FG and BG, resulting in 4 assertions. combin. -> combination
Comment on attachment 81992 [details] fix test names View in context: https://bugs.webkit.org/attachment.cgi?id=81992&action=review > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:58 > + void setPaletteActiveBG(); > + void setPaletteActiveFG(); > + void setPaletteInactiveBG(); > + void setPaletteInactiveFG(); Can we share some code in these tests by using a data-function?
I will clean the patch and add a test for the QGraphicsWebView because that part seems buggy too :).
(In reply to comment #27) > (From update of attachment 81992 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81992&action=review > > > Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp:58 > > + void setPaletteActiveBG(); > > + void setPaletteActiveFG(); > > + void setPaletteInactiveBG(); > > + void setPaletteInactiveFG(); > > Can we share some code in these tests by using a data-function? Will do :)
Hi Alexis, do you need me to address your review, or are you taking over from here?
I'll finish it :). Thanks Fabrizio for the patch. I will put your name in the changelog.
Thanks, appreciate your help!
Created attachment 83341 [details] Patch with comments taken into accounts and a better test case.
Comment on attachment 83341 [details] Patch with comments taken into accounts and a better test case. LGTM.
The commit-queue encountered the following flaky tests while processing attachment 83341 [details]: media/invalid-media-url-crash.html bug 51138 (author: inferno@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 83341 [details] Patch with comments taken into accounts and a better test case. Clearing flags on attachment: 83341 Committed r79409: <http://trac.webkit.org/changeset/79409>
All reviewed patches have been landed. Closing bug.