Summary: | Qt Port does not use style colour as configured for input text and menu lists | ||
---|---|---|---|
Product: | WebKit | Reporter: | Mike Fenton <mifenton> |
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | eric, hausmann, kenneth, manyoso, vestbo |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: |
Description
Mike Fenton
2009-08-06 11:01:19 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.
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.
Seems right to me. Simon do you have any objections to these patches? 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 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
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? 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). 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. 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 on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.
LGTM.
Comment on attachment 34219 [details]
Patch to update RenderThemeQt.cpp style() color setting and associated tests.
Landed in 46896
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
Created attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win
Reopned as not all ports have expected test results. Created attachment 34312 [details]
Skip menulist-style-color.html for mac/win until they can be looked at.
Comment on attachment 34303 [details]
Add test results for text-style-color.html for Mac/Win
Landed in 46904
Created attachment 34315 [details]
Replace Patch that won't apply cleanly.
Comment on attachment 34315 [details]
Replace Patch that won't apply cleanly.
Landed in 46909
Since all patches have been landed. Closing bug. Please use a new bug for more patches. |