Bug 57007 – [Qt] Re-draw the <input> fields for Qt Mobile Theme to do not override elements already styled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Diego Gonzalez]

Check if the element is styled and Draw the inputs for mobile theme instead of use
the css theme.

[Diego Gonzalez]

Created attachment 87099 [details]
Patch

First patch to see what you guys think and test

[WebKit Review Bot]

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]

Can you post a few screenshots?

[Diego Gonzalez]

Created attachment 87172 [details]
Inputs with mobile theme

[Diego Gonzalez]

Created attachment 87173 [details]
Gmail mobile with no overridden inputs

[Diego Gonzalez]

Created attachment 87174 [details]
Patch 2

Some more adjustments

[WebKit Review Bot]

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]

The input fields look quite nice, but there is something wrong with the buttons. Are you looking into that?

[Diego Gonzalez]

(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]

Created attachment 87409 [details]
Patch 3

Added buttons padding

[WebKit Review Bot]

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]

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]

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]

Created attachment 87503 [details]
Current screenshot

PS: There is no styled defined for scrollbars in current mobile theme

[Kenneth Rohde Christiansen]

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]

Looks better, but comboboxes are still wrong, ie they do not look like buttons.

[Kenneth Rohde Christiansen]

You screenshots shows the elements zoomed in, but not scaled up. Try applying a scale on the QGraphicsWebView.

[Diego Gonzalez]

Created attachment 87513 [details]
Combo as buttons screenshot

[Diego Gonzalez]

Created attachment 87515 [details]
Patch drawing combo as buttons

[WebKit Review Bot]

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]

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]

Created attachment 87521 [details]
More screenshots

[Kenneth Rohde Christiansen]

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 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]

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]

Created attachment 87584 [details]
Wrongly drawn radio buttons

[Diego Gonzalez]

(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]

Created attachment 87981 [details]
Current theme screenshot after patch

[Diego Gonzalez]

Created attachment 87982 [details]
Patch

[Kenneth Rohde Christiansen]

I still think that the comboboxes needs a bit more left padding, but it is a very good improvement.

[Kenneth Rohde Christiansen]

What is missing now.:

Better padding at least in text area
Combobox left padding
Show that you are never painting scrollbars.

[Diego Gonzalez]

Fixed at r83051