Bug 28050 - Qt Port does not use style colour as configured for input text and menu lists
Summary: Qt Port does not use style colour as configured for input text and menu lists
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-06 11:01 PDT by Mike Fenton
Modified: 2009-08-07 14:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch to update RenderThemeQt.cpp style() color setting and associated tests. (12.86 KB, patch)
2009-08-06 12:56 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Coding style patch for RenderThemeQt.cpp to conform to webkit style standards as verified by cpp_style.py (4.87 KB, patch)
2009-08-06 12:59 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Add test results for text-style-color.html for Mac/Win (11.18 KB, patch)
2009-08-07 11:43 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Skip menulist-style-color.html for mac/win until they can be looked at. (1.73 KB, patch)
2009-08-07 12:05 PDT, Mike Fenton
manyoso: review+
Details | Formatted Diff | Diff
Replace Patch that won't apply cleanly. (1.76 KB, patch)
2009-08-07 12:52 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2009-08-06 11:01:19 PDT
Note:  This bug is related to bug #27814 https://bugs.webkit.org/show_bug.cgi?id=27814

Just as with the button rendering, adjustStyle calls for TextField, MenuList and MenuListButton all overwrite the style colour with the current palette text colour.

This was first noticed by kenneth in the test isindex-placeholder.html test.

The overwriting must be removed and new tests created to verify the style colour being set correctly.
Comment 1 Mike Fenton 2009-08-06 12:56:35 PDT
Created attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

This patch removes the override of the style colour and adds in two new tests to verify this functionality

New tests include

fast/forms/menulist-style-color.html
fast/forms/text-style-color.html

This patch also removes 4 tests from platform/qt/Skipped that now successfully pass using the primary result set.

NOTE:  Please review only, with the addition of new tests, result sets will be required for other platforms to build and test correctly and will need to be updated after the initial commit.
Comment 2 Mike Fenton 2009-08-06 12:59:54 PDT
Created attachment 34220 [details]
Coding style patch for RenderThemeQt.cpp to conform to webkit style standards as verified by cpp_style.py

This style patch has been separated from the code change as previously requested.  It is intended to be applied on top of the other patch, but could also be squashed together.
Comment 3 Kenneth Rohde Christiansen 2009-08-06 19:30:08 PDT
Seems right to me. Simon do you have any objections to these patches?
Comment 4 George Staikos 2009-08-06 20:54:36 PDT
Comment on attachment 34220 [details]
Coding style patch for RenderThemeQt.cpp to conform to webkit style standards as verified by cpp_style.py

This part is not controversial.
Comment 5 George Staikos 2009-08-06 20:55:32 PDT
Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

This looks correct but will wait for feedback from Simon
Comment 6 Simon Hausmann 2009-08-07 04:06:34 PDT
Hmm, the original setColor calls were introduced in r34468 / 3afe4dbf554b49e8e6564fd3913aa770952cbdcd by Tor Arne. Overwriting what's specified in CSS sounds indeed wrong, but shouldn't we still get the color from the palette if it's _not_ specified in CSS?
Comment 7 Mike Fenton 2009-08-07 05:17:52 PDT
I don't believe we want to set it at all when it's not provided.

It would be very difficult to have working tests as it's overriding the style for the rendering (resulting in the dump render tree output including the palette colour as though it was provided in CSS).
Comment 8 Kenneth Rohde Christiansen 2009-08-07 05:20:17 PDT
If we decide to use it in the case it is not provided, (if not, are we breaking the Qt styles?) we could disable it for the tests.
Comment 9 Tor Arne Vestbø 2009-08-07 05:45:37 PDT
I'm trying to remember why I added those lines, but I agree that they don't really make sense. Dunno what I was smoking.
Comment 10 Eric Seidel (no email) 2009-08-07 08:44:22 PDT
Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

LGTM.
Comment 11 Kenneth Rohde Christiansen 2009-08-07 10:47:06 PDT
Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

Landed in 46896
Comment 12 Kenneth Rohde Christiansen 2009-08-07 10:47:34 PDT
Comment on attachment 34220 [details]
Coding style patch for RenderThemeQt.cpp to conform to webkit style standards as verified by cpp_style.py

Landed in 46897
Comment 13 Mike Fenton 2009-08-07 11:43:21 PDT
Created attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win
Comment 14 Kenneth Rohde Christiansen 2009-08-07 11:46:28 PDT
Reopned as not all ports have expected test results.
Comment 15 Mike Fenton 2009-08-07 12:05:01 PDT
Created attachment 34312 [details]
Skip menulist-style-color.html for mac/win until they can be looked at.
Comment 16 Kenneth Rohde Christiansen 2009-08-07 12:43:05 PDT
Comment on attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win

Landed in 46904
Comment 17 Mike Fenton 2009-08-07 12:52:29 PDT
Created attachment 34315 [details]
Replace Patch that won't apply cleanly.
Comment 18 Kenneth Rohde Christiansen 2009-08-07 13:09:49 PDT
Comment on attachment 34315 [details]
Replace Patch that won't apply cleanly.

Landed in 46909
Comment 19 Eric Seidel (no email) 2009-08-07 14:27:50 PDT
Since all patches have been landed.  Closing bug.  Please use a new bug for more patches.