Bug 28050 – Qt Port does not use style colour as configured for input text and menu lists

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Fenton]

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.

[Mike Fenton]

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.

[Mike Fenton]

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.

[Kenneth Rohde Christiansen]

Seems right to me. Simon do you have any objections to these patches?

[George Staikos]

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.

[George Staikos]

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

[Simon Hausmann]

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?

[Mike Fenton]

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

[Kenneth Rohde Christiansen]

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.

[Tor Arne Vestbø]

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.

[Eric Seidel (no email)]

Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.

Landed in 46896

[Kenneth Rohde Christiansen]

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

[Mike Fenton]

Created attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win

[Kenneth Rohde Christiansen]

Reopned as not all ports have expected test results.

[Mike Fenton]

Created attachment 34312 [details]
Skip menulist-style-color.html for mac/win until they can be looked at.

[Kenneth Rohde Christiansen]

Comment on attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win

Landed in 46904

[Mike Fenton]

Created attachment 34315 [details]
Replace Patch that won't apply cleanly.

[Kenneth Rohde Christiansen]

Comment on attachment 34315 [details]
Replace Patch that won't apply cleanly.

Landed in 46909

[Eric Seidel (no email)]

Since all patches have been landed.  Closing bug.  Please use a new bug for more patches.