RESOLVED FIXED 57007
[Qt] Re-draw the <input> fields for Qt Mobile Theme to do not override elements already styled
https://bugs.webkit.org/show_bug.cgi?id=57007
Summary [Qt] Re-draw the <input> fields for Qt Mobile Theme to do not override elemen...
Diego Gonzalez
Reported 2011-03-24 05:45:08 PDT
Check if the element is styled and Draw the inputs for mobile theme instead of use the css theme.
Attachments
Patch (13.50 KB, patch)
2011-03-27 22:26 PDT, Diego Gonzalez
no flags
Inputs with mobile theme (76.39 KB, image/png)
2011-03-28 11:38 PDT, Diego Gonzalez
no flags
Gmail mobile with no overridden inputs (31.70 KB, image/png)
2011-03-28 11:39 PDT, Diego Gonzalez
no flags
Patch 2 (14.09 KB, patch)
2011-03-28 11:41 PDT, Diego Gonzalez
no flags
Patch 3 (16.59 KB, patch)
2011-03-29 14:19 PDT, Diego Gonzalez
no flags
Current screenshot (79.77 KB, image/png)
2011-03-30 04:54 PDT, Diego Gonzalez
no flags
Combo as buttons screenshot (22.52 KB, image/png)
2011-03-30 06:05 PDT, Diego Gonzalez
no flags
Patch drawing combo as buttons (16.57 KB, patch)
2011-03-30 06:06 PDT, Diego Gonzalez
no flags
More screenshots (88.45 KB, image/png)
2011-03-30 06:29 PDT, Diego Gonzalez
no flags
Wrongly drawn radio buttons (65.86 KB, image/png)
2011-03-30 11:43 PDT, Kenneth Rohde Christiansen
no flags
Current theme screenshot after patch (117.61 KB, image/png)
2011-04-02 11:57 PDT, Diego Gonzalez
no flags
Patch (17.40 KB, patch)
2011-04-02 12:03 PDT, Diego Gonzalez
no flags
Diego Gonzalez
Comment 1 2011-03-27 22:26:54 PDT
Created attachment 87099 [details] Patch First patch to see what you guys think and test
WebKit Review Bot
Comment 2 2011-03-27 22:30:23 PDT
Attachment 87099 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "option" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "painter" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "widget" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Rohde Christiansen
Comment 3 2011-03-28 00:48:29 PDT
Can you post a few screenshots?
Diego Gonzalez
Comment 4 2011-03-28 11:38:31 PDT
Created attachment 87172 [details] Inputs with mobile theme
Diego Gonzalez
Comment 5 2011-03-28 11:39:22 PDT
Created attachment 87173 [details] Gmail mobile with no overridden inputs
Diego Gonzalez
Comment 6 2011-03-28 11:41:57 PDT
Created attachment 87174 [details] Patch 2 Some more adjustments
WebKit Review Bot
Comment 7 2011-03-28 11:44:16 PDT
Attachment 87174 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "option" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "painter" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "widget" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Rohde Christiansen
Comment 8 2011-03-28 14:08:29 PDT
The input fields look quite nice, but there is something wrong with the buttons. Are you looking into that?
Diego Gonzalez
Comment 9 2011-03-28 20:32:38 PDT
(In reply to comment #8) > The input fields look quite nice, but there is something wrong with the buttons. Are you looking into that? Hummmm will check it
Diego Gonzalez
Comment 10 2011-03-29 14:19:26 PDT
Created attachment 87409 [details] Patch 3 Added buttons padding
WebKit Review Bot
Comment 11 2011-03-29 14:21:49 PDT
Attachment 87409 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "option" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "painter" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "widget" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Rohde Christiansen
Comment 12 2011-03-29 14:36:05 PDT
New screenshots? Also, can you make sure that we paint scrollbars as 0 pixel wide scrollbars? :-) ie, not paint them and not making them take up space at all.
Kenneth Rohde Christiansen
Comment 13 2011-03-29 14:39:04 PDT
Also post a screenshot of the current mobile theme for comparison, including at least radio button (different states, sizes) checkboxes (different states, sizes) input fields text areas scrollbars select/comboboxes (singles and multiple)
Diego Gonzalez
Comment 14 2011-03-30 04:54:39 PDT
Created attachment 87503 [details] Current screenshot PS: There is no styled defined for scrollbars in current mobile theme
Kenneth Rohde Christiansen
Comment 15 2011-03-30 05:18:18 PDT
IT (In reply to comment #14) > Created an attachment (id=87503) [details] > Current screenshot > > PS: There is no styled defined for scrollbars in current mobile theme That might be the issue because it shown the windows like scrollbar.
Kenneth Rohde Christiansen
Comment 16 2011-03-30 05:19:21 PDT
Looks better, but comboboxes are still wrong, ie they do not look like buttons.
Kenneth Rohde Christiansen
Comment 17 2011-03-30 05:21:48 PDT
You screenshots shows the elements zoomed in, but not scaled up. Try applying a scale on the QGraphicsWebView.
Diego Gonzalez
Comment 18 2011-03-30 06:05:29 PDT
Created attachment 87513 [details] Combo as buttons screenshot
Diego Gonzalez
Comment 19 2011-03-30 06:06:59 PDT
Created attachment 87515 [details] Patch drawing combo as buttons
WebKit Review Bot
Comment 20 2011-03-30 06:10:37 PDT
Attachment 87515 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "option" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "painter" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/qt/QtMobileWebStyle.h:32: The parameter name "widget" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Rohde Christiansen
Comment 21 2011-03-30 06:28:34 PDT
I would say that the combos should have the same size of the buttons, currently the text placement looks quite bad in them. Apart from that it looks quite nice. Did you test the scrollbars yet?
Diego Gonzalez
Comment 22 2011-03-30 06:29:08 PDT
Created attachment 87521 [details] More screenshots
Kenneth Rohde Christiansen
Comment 23 2011-03-30 06:30:50 PDT
Oh the text placement also seems an issue for the text area. Please write some text in the input fields next time around. Actually, I don't think the buttons need to be so wide vertically (high).
Kenneth Rohde Christiansen
Comment 24 2011-03-30 11:38:51 PDT
Comment on attachment 87515 [details] Patch drawing combo as buttons View in context: https://bugs.webkit.org/attachment.cgi?id=87515&action=review > Source/WebCore/platform/qt/QtMobileWebStyle.cpp:192 > default: > QWindowsStyle::drawControl(element, option, painter, widget); > } It can be this part of the code that makes the scrollbar drawn as a Windows control > Source/WebCore/platform/qt/QtMobileWebStyle.cpp:326 > + QLinearGradient linearGradient(rect.bottomLeft(), QPoint(rect.bottomLeft().x(), rect.bottomLeft().y() - 20)); Add a comment explaining the 20, or use a variable instead with a good descriptive name > Source/WebCore/platform/qt/QtMobileWebStyle.h:33 > > void drawControl(ControlElement element, const QStyleOption* option, QPainter* painter, const QWidget* widget = 0) const; > void drawComplexControl(ComplexControl cc, const QStyleOptionComplex* option, QPainter* painter, const QWidget* widget = 0) const; > + void drawPrimitive(PrimitiveElement element, const QStyleOption* option, QPainter* painter, const QWidget* widget = 0) const; > You should clean up the style in another patch, or it would be good doing so. I can easily review that.
Kenneth Rohde Christiansen
Comment 25 2011-03-30 11:42:32 PDT
Hi Diego, I found one more issue that you need to fix. Radio buttons are shown wrong on http://www.amw.com/missing_persons/search.cfm Going to upload screenshot.
Kenneth Rohde Christiansen
Comment 26 2011-03-30 11:43:05 PDT
Created attachment 87584 [details] Wrongly drawn radio buttons
Diego Gonzalez
Comment 27 2011-03-30 14:03:44 PDT
(In reply to comment #26) > Created an attachment (id=87584) [details] > Wrongly drawn radio buttons Nice catch Kenneth! It's also reproducible in current mobile theme implementation. Investigating it :)
Diego Gonzalez
Comment 28 2011-04-02 11:57:22 PDT
Created attachment 87981 [details] Current theme screenshot after patch
Diego Gonzalez
Comment 29 2011-04-02 12:03:02 PDT
Kenneth Rohde Christiansen
Comment 30 2011-04-04 01:22:31 PDT
I still think that the comboboxes needs a bit more left padding, but it is a very good improvement.
Kenneth Rohde Christiansen
Comment 31 2011-04-04 01:25:50 PDT
What is missing now.: Better padding at least in text area Combobox left padding Show that you are never painting scrollbars.
Diego Gonzalez
Comment 32 2011-04-06 08:44:29 PDT
Fixed at r83051
Note You need to log in before you can comment on or make changes to this bug.