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.
Created attachment 40797 [details] First try
Comment on attachment 40797 [details] First try You can't add a raw member variable to RenderStyle like that.
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.
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.
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.
Created attachment 40816 [details] WebCore: Add a palette() method that is needed by our RenderTheme.
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>
All reviewed patches have been landed. Closing bug.
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.
bot accidentaly marked it as FIXED. reopening
Created attachment 41114 [details] Proposed patch - limited Fixed some errors in reporting changes (ChangeLog) added one more test that is affected by this change.
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
Created attachment 41307 [details] Proposed Patch - best effort fix Addressed Kenneth's concerns.
Created attachment 41317 [details] Proposed Patch - best effort Fixed comment style added more info about limitations in change log.
(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+
According to David Hyatt it looks good, but he would like a Qt reviewer to give the final word.
Comment on attachment 41317 [details] Proposed Patch - best effort Clearing review flag for now, needs more discussion
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.
(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.
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.
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.
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
Created attachment 43310 [details] Proposed Patch fixes conflict flagged by review bot.
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.
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.
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.
Created attachment 44188 [details] Layout tests results (affected by patch) on QtBuildBot
Created attachment 44258 [details] Proposed Patch This patch implements the consensus reached in the Qt patch review meeting on 11/19/2009.
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
Created attachment 44260 [details] Proposed Patch Fixes minor style issues in the previous patch. Only whitespace changes.
style-queue ran check-webkit-style on attachment 44260 [details] without any errors.
(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.
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.
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.
(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.
=== 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.