Bug 31742

Summary: [Qt] QWebView ignores a palette set with QWebView::setPalette()
Product: WebKit Reporter: Strahinja Markovic <strahinja.markovic>
Component: WebKit QtAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fabrizio.machado, kent.hansen, menard, ostap73, rion4ik, robert, vestbo, webkit.review.bot
Priority: P2 Keywords: EasyFix, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 55029    
Bug Blocks:    
Attachments:
Description Flags
Palette test case
none
Use QWebView custom palette if application provides one
none
Reworked patch to remove tab from ChangeLog
none
Added api tests to verify fix, minor cleanup based on check-webkit-style results
none
Update ChangeLogs
none
fix style check failure: remove tab from ChangeLog
none
more tab removals
benjamin: review-
rework to address comment #22
none
fix test names
menard: review-, menard: commit-queue-
Patch with comments taken into accounts and a better test case. none

Strahinja Markovic
Reported 2009-11-20 13:29:53 PST
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.
Attachments
Palette test case (1.39 KB, application/zip)
2010-03-15 12:49 PDT, Strahinja Markovic 		no flags
Details
Use QWebView custom palette if application provides one (2.45 KB, patch)
2011-01-31 12:40 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
Reworked patch to remove tab from ChangeLog (2.44 KB, patch)
2011-02-02 11:09 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
Added api tests to verify fix, minor cleanup based on check-webkit-style results (16.02 KB, patch)
2011-02-05 21:26 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
Update ChangeLogs (16.86 KB, patch)
2011-02-05 21:45 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
fix style check failure: remove tab from ChangeLog (16.87 KB, patch)
2011-02-05 21:57 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
more tab removals (16.87 KB, patch)
2011-02-05 22:02 PST, Fabrizio 		benjamin: review-
DetailsFormatted DiffDiff
rework to address comment #22 (14.85 KB, patch)
2011-02-09 17:15 PST, Fabrizio 		no flags
DetailsFormatted DiffDiff
fix test names (14.82 KB, patch)
2011-02-10 09:24 PST, Fabrizio 		menard: review-
menard: commit-queue-
DetailsFormatted DiffDiff
Patch with comments taken into accounts and a better test case. (7.93 KB, patch)
2011-02-22 10:54 PST, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.
Tor Arne Vestbø
Comment 1 2010-03-10 06:26:40 PST
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
Kent Hansen
Comment 2 2010-03-12 02:43:56 PST
If you could provide a testcase, that would be great.
Strahinja Markovic
Comment 3 2010-03-15 12:49:38 PDT
Created attachment 50724 [details] Palette test case Here is the test case.
Robert Hogan
Comment 4 2010-03-16 11:13:15 PDT
I can reproduce this.
Jocelyn Turcotte
Comment 5 2010-03-16 11:33:10 PDT
*** Bug 29480 has been marked as a duplicate of this bug. ***
Jocelyn Turcotte
Comment 6 2010-03-16 11:34:05 PDT
*** Bug 35649 has been marked as a duplicate of this bug. ***
Viatcheslav Ostapenko
Comment 7 2010-04-08 08:08:04 PDT
The problem is in RenderThemeQt.cpp . It takes palette from widget which comes in PaintInfo, but not from webpage.
Fabrizio
Comment 8 2011-01-31 12:40:55 PST
Created attachment 80671 [details] Use QWebView custom palette if application provides one
Benjamin Poulain
Comment 9 2011-01-31 12:53:14 PST
I think some of those could be tested with autotest (selection color).
WebKit Review Bot
Comment 10 2011-02-01 11:10:49 PST
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.
Fabrizio
Comment 11 2011-02-02 11:09:35 PST
Created attachment 80934 [details] Reworked patch to remove tab from ChangeLog
Fabrizio
Comment 12 2011-02-02 11:16:53 PST
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.
Benjamin Poulain
Comment 13 2011-02-03 01:38:53 PST
(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 :)
Fabrizio
Comment 14 2011-02-03 04:15:31 PST
Aha, I see. Let me try that, thanks for the description on how to do it.
Fabrizio
Comment 15 2011-02-05 21:26:29 PST
Created attachment 81387 [details] Added api tests to verify fix, minor cleanup based on check-webkit-style results
Fabrizio
Comment 16 2011-02-05 21:45:50 PST
Created attachment 81388 [details] Update ChangeLogs
WebKit Review Bot
Comment 17 2011-02-05 21:47:25 PST
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.
Fabrizio
Comment 18 2011-02-05 21:57:39 PST
Created attachment 81389 [details] fix style check failure: remove tab from ChangeLog
WebKit Review Bot
Comment 19 2011-02-05 22:00:05 PST
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.
Fabrizio
Comment 20 2011-02-05 22:02:56 PST
Created attachment 81390 [details] more tab removals
Fabrizio
Comment 21 2011-02-09 09:48:12 PST
Ping for review, anyone?
Benjamin Poulain
Comment 22 2011-02-09 10:01:06 PST
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?
Fabrizio
Comment 23 2011-02-09 10:54:32 PST
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.
Fabrizio
Comment 24 2011-02-09 17:15:29 PST
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.
Fabrizio
Comment 25 2011-02-10 09:24:54 PST
Created attachment 81992 [details] fix test names
Alexis Menard (darktears)
Comment 26 2011-02-22 08:26:48 PST
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
Tor Arne Vestbø
Comment 27 2011-02-22 08:41:10 PST
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?
Alexis Menard (darktears)
Comment 28 2011-02-22 08:46:23 PST
I will clean the patch and add a test for the QGraphicsWebView because that part seems buggy too :).
Alexis Menard (darktears)
Comment 29 2011-02-22 08:46:48 PST
(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 :)
Fabrizio
Comment 30 2011-02-22 10:04:16 PST
Hi Alexis, do you need me to address your review, or are you taking over from here?
Alexis Menard (darktears)
Comment 31 2011-02-22 10:11:18 PST
I'll finish it :). Thanks Fabrizio for the patch. I will put your name in the changelog.
Fabrizio
Comment 32 2011-02-22 10:12:15 PST
Thanks, appreciate your help!
Alexis Menard (darktears)
Comment 33 2011-02-22 10:54:09 PST
Created attachment 83341 [details] Patch with comments taken into accounts and a better test case.
Andreas Kling
Comment 34 2011-02-22 10:58:26 PST
Comment on attachment 83341 [details] Patch with comments taken into accounts and a better test case. LGTM.
WebKit Commit Bot
Comment 35 2011-02-22 22:52:46 PST
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.
WebKit Commit Bot
Comment 36 2011-02-22 22:54:51 PST
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>
WebKit Commit Bot
Comment 37 2011-02-22 22:54:58 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.