Bug 31742 – [Qt] QWebView ignores a palette set with QWebView::setPalette()

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Strahinja Markovic]

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.

[Tor Arne Vestbø]

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]

If you could provide a testcase, that would be great.

[Strahinja Markovic]

Created attachment 50724 [details]
Palette test case

Here is the test case.

[Robert Hogan]

I can reproduce this.

[Jocelyn Turcotte]

*** Bug 29480 has been marked as a duplicate of this bug. ***

[Jocelyn Turcotte]

*** Bug 35649 has been marked as a duplicate of this bug. ***

[Viatcheslav Ostapenko]

The problem is in RenderThemeQt.cpp . It takes palette from widget which comes in PaintInfo, but not from webpage.

[Fabrizio]

Created attachment 80671 [details]
Use QWebView custom palette if application provides one

[Benjamin Poulain]

I think some of those could be tested with autotest (selection color).

[WebKit Review Bot]

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]

Created attachment 80934 [details]
Reworked patch to remove tab from ChangeLog

[Fabrizio]

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]

(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]

Aha, I see.  Let me try that, thanks for the description on how to do it.

[Fabrizio]

Created attachment 81387 [details]
Added api tests to verify fix, minor cleanup based on check-webkit-style results

[Fabrizio]

Created attachment 81388 [details]
Update ChangeLogs

[WebKit Review Bot]

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]

Created attachment 81389 [details]
fix style check failure:  remove tab from ChangeLog

[WebKit Review Bot]

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]

Created attachment 81390 [details]
more tab removals

[Fabrizio]

Ping for review, anyone?

[Benjamin Poulain]

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]

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]

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]

Created attachment 81992 [details]
fix test names

[Alexis Menard (darktears)]

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 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)]

I will clean the patch and add a test for the QGraphicsWebView because that part seems buggy too :).

[Alexis Menard (darktears)]

(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]

Hi Alexis, do you need me to address your review, or are you taking over from here?

[Alexis Menard (darktears)]

I'll finish it :). Thanks Fabrizio for the patch. I will put your name in the changelog.

[Fabrizio]

Thanks, appreciate your help!

[Alexis Menard (darktears)]

Created attachment 83341 [details]
Patch with comments taken into accounts and a better test case.

[Andreas Kling]

Comment on attachment 83341 [details]
Patch with comments taken into accounts and a better test case.

LGTM.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 83341 [details]:

media/invalid-media-url-crash.html bug 51138 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.