Summary: | [Qt] QWebView ignores a palette set with QWebView::setPalette() | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Strahinja Markovic <strahinja.markovic> | ||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | 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
Strahinja Markovic
2009-11-20 13:29:53 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 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. |