Bug 57007 - [Qt] Re-draw the <input> fields for Qt Mobile Theme to do not override elements already styled
Summary: [Qt] Re-draw the <input> fields for Qt Mobile Theme to do not override elemen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Diego Gonzalez
URL:
Keywords: EasyFix, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-03-24 05:45 PDT by Diego Gonzalez
Modified: 2011-04-14 06:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.50 KB, patch)
2011-03-27 22:26 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Inputs with mobile theme (76.39 KB, image/png)
2011-03-28 11:38 PDT, Diego Gonzalez
no flags Details
Gmail mobile with no overridden inputs (31.70 KB, image/png)
2011-03-28 11:39 PDT, Diego Gonzalez
no flags Details
Patch 2 (14.09 KB, patch)
2011-03-28 11:41 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch 3 (16.59 KB, patch)
2011-03-29 14:19 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Current screenshot (79.77 KB, image/png)
2011-03-30 04:54 PDT, Diego Gonzalez
no flags Details
Combo as buttons screenshot (22.52 KB, image/png)
2011-03-30 06:05 PDT, Diego Gonzalez
no flags Details
Patch drawing combo as buttons (16.57 KB, patch)
2011-03-30 06:06 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
More screenshots (88.45 KB, image/png)
2011-03-30 06:29 PDT, Diego Gonzalez
no flags Details
Wrongly drawn radio buttons (65.86 KB, image/png)
2011-03-30 11:43 PDT, Kenneth Rohde Christiansen
no flags Details
Current theme screenshot after patch (117.61 KB, image/png)
2011-04-02 11:57 PDT, Diego Gonzalez
no flags Details
Patch (17.40 KB, patch)
2011-04-02 12:03 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 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.
Comment 1 Diego Gonzalez 2011-03-27 22:26:54 PDT
Created attachment 87099 [details]
Patch

First patch to see what you guys think and test
Comment 2 WebKit Review Bot 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.
Comment 3 Kenneth Rohde Christiansen 2011-03-28 00:48:29 PDT
Can you post a few screenshots?
Comment 4 Diego Gonzalez 2011-03-28 11:38:31 PDT
Created attachment 87172 [details]
Inputs with mobile theme
Comment 5 Diego Gonzalez 2011-03-28 11:39:22 PDT
Created attachment 87173 [details]
Gmail mobile with no overridden inputs
Comment 6 Diego Gonzalez 2011-03-28 11:41:57 PDT
Created attachment 87174 [details]
Patch 2

Some more adjustments
Comment 7 WebKit Review Bot 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.
Comment 8 Kenneth Rohde Christiansen 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?
Comment 9 Diego Gonzalez 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
Comment 10 Diego Gonzalez 2011-03-29 14:19:26 PDT
Created attachment 87409 [details]
Patch 3

Added buttons padding
Comment 11 WebKit Review Bot 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.
Comment 12 Kenneth Rohde Christiansen 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.
Comment 13 Kenneth Rohde Christiansen 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)
Comment 14 Diego Gonzalez 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
Comment 15 Kenneth Rohde Christiansen 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.
Comment 16 Kenneth Rohde Christiansen 2011-03-30 05:19:21 PDT
Looks better, but comboboxes are still wrong, ie they do not look like buttons.
Comment 17 Kenneth Rohde Christiansen 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.
Comment 18 Diego Gonzalez 2011-03-30 06:05:29 PDT
Created attachment 87513 [details]
Combo as buttons screenshot
Comment 19 Diego Gonzalez 2011-03-30 06:06:59 PDT
Created attachment 87515 [details]
Patch drawing combo as buttons
Comment 20 WebKit Review Bot 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.
Comment 21 Kenneth Rohde Christiansen 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?
Comment 22 Diego Gonzalez 2011-03-30 06:29:08 PDT
Created attachment 87521 [details]
More screenshots
Comment 23 Kenneth Rohde Christiansen 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).
Comment 24 Kenneth Rohde Christiansen 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.
Comment 25 Kenneth Rohde Christiansen 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.
Comment 26 Kenneth Rohde Christiansen 2011-03-30 11:43:05 PDT
Created attachment 87584 [details]
Wrongly drawn radio buttons
Comment 27 Diego Gonzalez 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 :)
Comment 28 Diego Gonzalez 2011-04-02 11:57:22 PDT
Created attachment 87981 [details]
Current theme screenshot after patch
Comment 29 Diego Gonzalez 2011-04-02 12:03:02 PDT
Created attachment 87982 [details]
Patch
Comment 30 Kenneth Rohde Christiansen 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.
Comment 31 Kenneth Rohde Christiansen 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.
Comment 32 Diego Gonzalez 2011-04-06 08:44:29 PDT
Fixed at r83051