Bug 30173 – [Qt] Make it possible to apply the CSS background color to the native painted widgets instead of to the actual element

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Kenneth Rohde Christiansen]

Created attachment 40795 [details]
Image showing the feature in action (the background of all input elements have been changed to yellow)

Qt has very good theming support, so it is possible to paint our Qt Buttons, TextFields etc with the actual CSS background color.

1) We only want to apply the background color if it has been explicitly specified in the CSS or something similar, similar to the style->backgroundColor() != backgroundColor check in IsControlStyled.
2) We want to apply the color to the native painted widgets and not always to the actual element. Why?

In the case of Qt, our TextField widget doesn't fill out the whole element, there is some padding plus it has rounded corners. For it to look OK, we set the backgroundColor to transparent in the adjust*Style method.

I have made a patch that implement the feature, but I had to add a method to RenderStyle that I am not particular happy with.

If anyone has input on a better/more proper way of implementing this, please do share.

[Kenneth Rohde Christiansen]

Created attachment 40797 [details]
First try

[Dave Hyatt]

Comment on attachment 40797 [details]
First try

You can't add a raw member variable to RenderStyle like that.

[Kenneth Rohde Christiansen]

Discussing more with David Hyatt, it seems that what we need is a way to let WebCore not paint the background and let us handle it.

[Carol Szabo]

Created attachment 40806 [details]
Working copy of patch attempt

This is kind of raw. I already have some improvements to it, but I sent it so that you have a base for discussion with dhyat.

[Carol Szabo]

Created attachment 40811 [details]
Working copy of patch attempt

Removed style fixes and other unnecessary changes. still a few changes left that address other issues such as box sizing and text positioning. Will clean up those too.

[Kenneth Rohde Christiansen]

Created attachment 40816 [details]
WebCore: Add a palette() method that is needed by our RenderTheme.

[WebKit Commit Bot]

Comment on attachment 40816 [details]
WebCore: Add a palette() method that is needed by our RenderTheme.

Clearing flags on attachment: 40816

Committed r49483: <http://trac.webkit.org/changeset/49483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carol Szabo]

Created attachment 41107 [details]
Proposed patch (limited changes)

This patch is the first attempt to fix this problem. It relies on Kenneth's improvement of the PageClient interface to obtain the widget's palette. Kenneth and I worked on this patch together.
This patch renders form controls in QtWebKit using the current Qt theme (QtStyle) (i.e. gtk, plastique, clearlooks, cde, windows, etc). The palette used for colors is that of the PageClient, except if the author of the page alters the background color of the control, in which case that color is used for the control's background in the palette, and for controls where the foreground color is relevant, this color is used for the foreground.
Currently there are some themes such as gtk which ignore the palette most of the time, and some others that selectively ignore it. When these themes are selected the changes to the palette may have no effect (which is consistent with the expectations for style from the HTML spec).
As Qt improves, more styles shall be more compliant with the requests made through the palette.
Also, I have a subsequent patch planned to address background images and anotherone for borders.
Currently text in controls where the text is rendered internally by WebKit is not properly aligned, but this is a separate bug that I plan to address.

[Antonio Gomes]

bot accidentaly marked it as FIXED. reopening

[Carol Szabo]

Created attachment 41114 [details]
Proposed patch - limited

Fixed some errors in reporting changes (ChangeLog) added one more test that is affected by this change.

[Kenneth Rohde Christiansen]

Comment on attachment 41114 [details]
Proposed patch - limited

Great work! 

> +        * platform/qt/RenderThemeQt.cpp:
> +        (WebCore::RenderThemeQt::isControlStyled):
> +        Specialized for Qt in order to capture the default (non-author altered) background color
> +        such that at paint time we can determine whether to paint the theme defined background or
> +        the author selected background.

Maybe a comment in the code for that function would be a good idea?

> +        (WebCore::RenderThemeQt::adjustCommonOptions):

I would suggest 'adjustCommonQStyleOptions)


> +    case SearchFieldCancelButtonPart: {
> +        *(const_cast<QColor*>(&m_defaultButtonBackgroundColor)) = backgroundColor;
> +        break;
> +    }

Why braces here and now below?

> +    case TextAreaPart:
> +        *(const_cast<QColor*>(&m_defaultTextBackgroundColor)) = backgroundColor;
> +        break;
> +    }

Also, please add a comment about the *(const_cast<QColor*>(&m_... line.

> +    return RenderTheme::isControlStyled(style, borderData, fillLayer, backgroundColor);
> +};
>  static int findFrameLineWidth(QStyle* style)

missing newline

> -    style->setBackgroundColor(Color::transparent);

Good change! was conceptual wrong

> +            if (chromeClient) {
> +                PlatformPageClient pageClient = chromeClient->platformPageClient();

as this is in the Qt implementation, maybe better to use QWebPageClient and not PlatformPageClient.

> +                if (pageClient)
> +                    option.palette = pageClient->palette();
> +            }
> +        }
> +    }
> +    RenderStyle* style = o->style();
> +    if (!style)
> +        return NoControlPart;
> +    ControlPart result = style->appearance();
> +    if (supportsFocus(result) && isFocused(o)) {
> +        option.state |= QStyle::State_HasFocus;
> +        option.state |= QStyle::State_KeyboardFocusChange;
> +    }
> +    if (style->direction() == WebCore::RTL)
>          option.direction = Qt::RightToLeft;

The above part could use a few new lines.

> +        if (backgroundColor != m_defaultTextBackgroundColor) {
> +            option.palette.setColor(QPalette::Button, backgroundColor);
> +            option.palette.setColor(QPalette::Window, backgroundColor);
> +            option.palette.setColor(QPalette::Base, backgroundColor);
> +            option.palette.setColor(QPalette::Text, style->color());
> +        }

Do you really need the Window? I dont like setting multiply ones, maybe this should go directly into the paint methods

[Carol Szabo]

Created attachment 41307 [details]
Proposed Patch - best effort fix

Addressed Kenneth's concerns.

[Carol Szabo]

Created attachment 41317 [details]
Proposed Patch - best effort

Fixed comment style added more info about limitations in change log.

[Kenneth Rohde Christiansen]

(In reply to comment #14)
> Created an attachment (id=41317) [details]
> Proposed Patch - best effort
> 
> Fixed comment style added more info about limitations in change log.

LGTM, just need an officiel reviewer to give it a r+

[Kenneth Rohde Christiansen]

According to David Hyatt it looks good, but he would like a Qt reviewer to give the final word.

[Tor Arne Vestbø]

Comment on attachment 41317 [details]
Proposed Patch - best effort

Clearing review flag for now, needs more discussion

[Tor Arne Vestbø]

Shouldn't we _set_ the default background color based on the QWebPageClient's
palette at some point (in adjust* or extraDefaultStyleSheet), and then always
set the option.palette to the backgroundColor when we paint?

That we don't have to store the default and check if it's changed.

I'm assuming we want the behavior of rendering google.com with a yellow
line-edit if the developer has modified the QPalette of either the application
or the widget. This is the behavior of FF on Linux at least. If the site wants
a background color (always white eg.) they set it, we pick it up from the CSS,
and set it on the option palette when rendering.

[Carol Szabo]

(In reply to comment #18)
> Shouldn't we _set_ the default background color based on the QWebPageClient's
> palette at some point (in adjust* or extraDefaultStyleSheet), and then always
> set the option.palette to the backgroundColor when we paint?
> 
> That we don't have to store the default and check if it's changed.
> 
> I'm assuming we want the behavior of rendering google.com with a yellow
> line-edit if the developer has modified the QPalette of either the application
> or the widget. This is the behavior of FF on Linux at least. If the site wants
> a background color (always white eg.) they set it, we pick it up from the CSS,
> and set it on the option palette when rendering.

For the adjustStyle* functions to work I need to save the defaults so the only purpose they can serve is to make the JS aware of the actual color used for rendering.
I will look into using extraDefaultStyleSheet but for now this brings the behavior closer to FF and IE.

[Carol Szabo]

Created attachment 43169 [details]
Proposed patch

This patch implements about 3 different related issues:
1. Rendering of Text input background does not follow the various shapes that text boxes can have under various Qt styles.
2. The default CSS style sheets do not reflect the default control backgrounds as set through various QStyles and QStyleSheets.
3. When the user uses CSS to change the background and foreground colors of controls, these changes are not accurately reflected in control rendering.
Causing weird things such as invisible text instead of reverse video for some controls such as Search.

My original goal was to just fix the 3rd issue, but after sever rounds of review and discussion it was determined that all three issues need to be addressed for the patch to be accepted.

[WebKit Review Bot]

Checking style for patch 43169 from bug 30173.
Patch failed to apply and check style
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/graphics/qt/GraphicsContextQt.cpp
patching file WebCore/platform/qt/QWebPageClient.h
patching file WebCore/platform/qt/QWebPopup.cpp
patching file WebCore/platform/qt/RenderThemeQt.cpp
patching file WebCore/platform/qt/RenderThemeQt.h
patching file WebCore/rendering/RenderTheme.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/qt/css1/box_properties/acid_test-expected.txt
Hunk #1 FAILED at 23.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/css1/box_properties/acid_test-expected.txt.rej
patch -p0 "LayoutTests/platform/qt/css1/box_properties/acid_test-expected.txt" returned 1.  Pass --force to ignore patch failures.

[Carol Szabo]

Created attachment 43303 [details]
Proposed Patch

Addresses mainly style issues and optimizations raised by kenneth regarding my previous patch.
This patch is dependent on landing my patch to 31527 first.
https://bugs.webkit.org/show_bug.cgi?id=31527

[Carol Szabo]

Created attachment 43310 [details]
Proposed Patch fixes conflict flagged by review bot.

[Simon Hausmann]

Comment on attachment 43310 [details]
Proposed Patch fixes conflict flagged by review bot.

As agreed on the phone last week we cannot use the style reliably to draw the background (including popular platforms such as Windows XP). Instead we should use the WebKit fallback if possible.

That would also result in a much smaller patch that is easier to review.

[Carol Szabo]

Created attachment 43879 [details]
Patch In Progress Still need to figure out why it breaks DumpRenderTree

Almost done, but need to figure out problems with DumpRenderTree.

[Carol Szabo]

Created attachment 44186 [details]
Patch without ChangeLog and LayoutTest changes. Used to figure out impact.

This is for Ossy to help me out with LayoutTest changes.

[Csaba Osztrogonác]

Created attachment 44188 [details]
Layout tests results (affected by patch) on QtBuildBot

[Carol Szabo]

Created attachment 44258 [details]
Proposed Patch

This patch implements the consensus reached in the Qt patch review meeting on 11/19/2009.

[WebKit Review Bot]

Attachment 44258 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/qt/RenderThemeQt.cpp:86:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/qt/RenderThemeQt.cpp:428:  Declaration has space between type name and * in QWidget *ownerWidget  [whitespace/declaration] [3]
Total errors found: 2

[Carol Szabo]

Created attachment 44260 [details]
Proposed Patch

Fixes minor style issues in the previous patch. Only whitespace changes.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44260 [details] without any errors.

[Csaba Osztrogonác]

(In reply to comment #30)
> Created an attachment (id=44260) [details]
> Proposed Patch
I tested your patch on QtBuildbot, it works fine without any additional regression.

[Simon Hausmann]

Comment on attachment 44260 [details]
Proposed Patch

Based on a face-to-face discussion we agreed that the patch is too big to be reviewed. It contains multiple features that should be individual patches. I'll help with finding a better way of retrieving the palette for widgets from the style without instantiating the widgets.

[Szilard Ledan]

Sorry for the inconvenience. I would like to know if this bug is still valid, or you plan to fix it. Thank you for your patience and attention in advance.

[Carol Szabo]

(In reply to comment #34)
> Sorry for the inconvenience. I would like to know if this bug is still valid, or you plan to fix it. Thank you for your patience and attention in advance.

I am no longer working on this bug. My patch has been disliked by Simon in fundamental ways and Kenneth provided some related fixes.
I do not know if all is fixed now or not.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.