Bug 42887 – [Qt] Search input field doesn't have cancel button

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

This bug revealed by fast/forms/search-cancel-button-events.html introduced 
in http://trac.webkit.org/changeset/63929 (https://bugs.webkit.org/show_bug.cgi?id=34393)

[Csaba Osztrogonác]

fast/forms/search-cancel-button-events.html is skipped by http://trac.webkit.org/changeset/63960 (until fix)

[Robert Hogan]

Created attachment 72211 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 72211 [details]
Patch

I would like to see a screenshot of this, and also when using the Qt mobile theme. (there is a compile flag for using the qt mobile theme)

[Robert Hogan]

Created attachment 72220 [details]
Screenshot

[Robert Hogan]

(In reply to comment #3)
> (From update of attachment 72211 [details])
> I would like to see a screenshot of this, and also when using the Qt mobile theme. (there is a compile flag for using the qt mobile theme)

I compile with CONFIG += use_qt_mobile_theme and the screenshot was the same.

[Andreas Kling]

Comment on attachment 72211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72211&action=review

r=me

> WebKit/qt/Api/qwebsettings.cpp:106
> +        hash->insert(QWebSettings::SearchCancelButtonGraphic, QApplication::style()->standardPixmap(QStyle::SP_DialogCloseButton));
> +        hash->insert(QWebSettings::SearchCancelButtonPressedGraphic, QApplication::style()->standardPixmap(QStyle::SP_DialogCloseButton));

Not a huge fan of the standardPixmap()s, but it's not a big deal, we can put some custom ones in later.

> WebKit/qt/Api/qwebsettings.cpp:380
> +    \value SearchCancelButtonGraphic The graphic shown for clearing the text in a search field.
> +    \value SearchCancelButtonPressedGraphic The graphic shown when SearchCancelButtonGraphic is pressed.

FYI: These additions will be subject to API review (scheduled for next week.)

[Kenneth Rohde Christiansen]

We might want to override these with our Mobile Theme.

[WebKit Commit Bot]

Comment on attachment 72211 [details]
Patch

Rejecting patch 72211 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
ers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSCanvasPattern.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/JSCanvasPattern.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSConsoleCustom.o /Projects/CommitQueue/WebCore/bindings/js/JSConsoleCustom.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(56 failures)


Full output: http://queues.webkit.org/results/6782106

[WebKit Commit Bot]

Comment on attachment 72211 [details]
Patch

Clearing flags on attachment: 72211

Committed r73635: <http://trac.webkit.org/changeset/73635>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

Was it rolled out? Should be REOPENED?

[Csaba Osztrogonác]

(In reply to comment #11)
> Was it rolled out? Should be REOPENED?

It was rolled out by http://trac.webkit.org/changeset/73642

[Robert Hogan]

Created attachment 78431 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 78431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78431&action=review

Is it styled in the mobile theme?

> WebCore/platform/graphics/qt/ImageQt.cpp:74
>          pixmap = QWebSettings::webGraphic(QWebSettings::DeleteButtonGraphic);
>      else if (!qstrcmp(name, "inputSpeech"))
>          pixmap = QWebSettings::webGraphic(QWebSettings::InputSpeechButtonGraphic);
> +    else if (!qstrcmp(name, "searchCancelButton"))
> +        pixmap = QWebSettings::webGraphic(QWebSettings::SearchCancelButtonGraphic);
> +    else if (!qstrcmp(name, "searchCancelButtonPressed"))
> +        pixmap = QWebSettings::webGraphic(QWebSettings::SearchCancelButtonPressedGraphic);

So sad that none of this will work for WebKit2.

> WebCore/platform/qt/RenderThemeQt.cpp:199
> +    // FIXME: Need to add SearchFieldPart if it should be style-able

Use real sentences... add a dot at the end.

> WebCore/platform/qt/RenderThemeQt.cpp:932
> +    // Scale the button size based on the font size

Use real sentences.

[Kenneth Rohde Christiansen]

Christian, can you get John to make some graphics for the mobile theme?

[Robert Hogan]

Created attachment 78436 [details]
Patch

[Robert Hogan]

Created attachment 78438 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 78438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78438&action=review

It contains new API, but it looks fine with me from a webkit 1 perspective.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:96
> +// These values all match Safari/Win/Chromium

Add dot before landing. Would be good with a "why" they must match :-) because then I do not see why we all need to define them.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:216
> +// Remove this when SearchFieldPart is style-able in RenderTheme::isControlStyled

Add "()."

> Source/WebCore/platform/qt/RenderThemeQt.cpp:224
> +        return (style->border() != border
> +                || *style->backgroundLayers() != fill
> +                || style->visitedDependentColor(CSSPropertyBackgroundColor) != backgroundColor);

So this part is covered with the layout tests? seems like something that should be common code

> Source/WebCore/platform/qt/RenderThemeQt.cpp:930
> +    // Taken from RenderThemeChromium.cpp.

Here the comment is inside the method

> Source/WebCore/platform/qt/RenderThemeQt.cpp:939
> +// Taken from RenderThemeChromium.cpp

Here it is outside? on purpose?

> Source/WebCore/platform/qt/RenderThemeQt.cpp:955
> +    // Adapted from RenderThemeChromium.cpp.

the adaption means using qMax? or did you change logic? you should make that clear

[Robert Hogan]

(In reply to comment #14)
> (From update of attachment 78431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78431&action=review
> 
> Is it styled in the mobile theme?
> 
(In reply to comment #14)
> (From update of attachment 78431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78431&action=review
> 
> Is it styled in the mobile theme?
> 

I don't know to be honest.

Styling the search field widget will result in a couple of differences from the base expected results in:

fast/css/pseudo-cache-stale.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/search-cancel-button-style-sharing.html

These differences look like:

-        RenderTextControl {INPUT} at (2,2) size 168x25 [bgcolor=#FFFFFF] border: (2px inset #000000)]
+        RenderTextControl {INPUT} at (2,2) size 166x26

This happens because RenderTheme won't apply CSS styles to controls that are already styled by the UA. The fact that we also do :

    style->setBackgroundColor(Color::transparent);
    style->resetBorder();
    style->resetPadding();

for both adjustTextFieldStyle() and searchFieldStyle() also contributes to this difference in layout tests. I've seen it elsewhere in layout test differences on Qt, so worth noting that this is why it happens.

Supporting styling is required to pass fast/css/pseudo-required-optional-006.html.

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> (In reply to comment #14)
> > (From update of attachment 78431 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=78431&action=review
> > 
> > Is it styled in the mobile theme?
> > 
> (In reply to comment #14)
> > (From update of attachment 78431 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=78431&action=review
> > 
> > Is it styled in the mobile theme?
> > 
> 
> I don't know to be honest.
> 
> Styling the search field widget will result in a couple of differences from the base expected results in:
> 
> fast/css/pseudo-cache-stale.html
> fast/css/text-input-with-webkit-border-radius.html
> fast/forms/search-cancel-button-style-sharing.html
> 
> These differences look like:
> 
> -        RenderTextControl {INPUT} at (2,2) size 168x25 [bgcolor=#FFFFFF] border: (2px inset #000000)]
> +        RenderTextControl {INPUT} at (2,2) size 166x26
> 
> This happens because RenderTheme won't apply CSS styles to controls that are already styled by the UA. The fact that we also do :
> 
>     style->setBackgroundColor(Color::transparent);
>     style->resetBorder();
>     style->resetPadding();
> 
> for both adjustTextFieldStyle() and searchFieldStyle() also contributes to this difference in layout tests. I've seen it elsewhere in layout test differences on Qt, so worth noting that this is why it happens.
> 
> Supporting styling is required to pass fast/css/pseudo-required-optional-006.html.

I think it would be good to add such comments in the code, noone will know about that.

[Robert Hogan]

(In reply to comment #18)
> (From update of attachment 78438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78438&action=review
> 
> It contains new API, but it looks fine with me from a webkit 1 perspective.
> 

Implies this also lives in WebKit2?

> > Source/WebCore/platform/qt/RenderThemeQt.cpp:96
> > +// These values all match Safari/Win/Chromium
> 
> Add dot before landing. Would be good with a "why" they must match :-) because then I do not see why we all need to define them.

Maybe a separate patch?

> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:216
> > +// Remove this when SearchFieldPart is style-able in RenderTheme::isControlStyled
> 
> Add "()."

OK

> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:224
> > +        return (style->border() != border
> > +                || *style->backgroundLayers() != fill
> > +                || style->visitedDependentColor(CSSPropertyBackgroundColor) != backgroundColor);
> 
> So this part is covered with the layout tests? seems like something that should be common code

Yes, copied from RenderTheme:isControlStyled(). I'm just shimming a check for SearchFieldPart rather than uncommenting it in RenderTheme. We can remove this per the comment once Jocelyn confirms it's safe to do so I hope.

> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:930
> > +    // Taken from RenderThemeChromium.cpp.
> 
> Here the comment is inside the method
> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:939
> > +// Taken from RenderThemeChromium.cpp
> 
> Here it is outside? on purpose?

OK.

> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:955
> > +    // Adapted from RenderThemeChromium.cpp.
> 
> the adaption means using qMax? or did you change logic? you should make that clear

Will do - just qMax really.

[Kenneth Rohde Christiansen]

> Implies this also lives in WebKit2?

Yes, WebKit2/Qt is using our RenderThemeQt.
 
> Maybe a separate patch?

Fine

[Robert Hogan]

Jocelyn, could you respond to the last part of https://bugs.webkit.org/show_bug.cgi?id=42887#c21 ?

[Robert Hogan]

Created attachment 78562 [details]
Patch

[Robert Hogan]

Created attachment 78564 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 78564 [details]:

http/tests/xmlhttprequest/cross-origin-no-credential-prompt.html bug 52249 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Jocelyn Turcotte]

(In reply to comment #23)
> Jocelyn, could you respond to the last part of https://bugs.webkit.org/show_bug.cgi?id=42887#c21 ?

Hei Robert, I couldn't catch you on IRC about the comment in RenderTheme.cpp, and never actually touched that file, so maybe something has messed up you git blame?

Though please slap me if my git log is broken and I'm suffering amnesia.

[WebKit Commit Bot]

Comment on attachment 78564 [details]
Patch

Clearing flags on attachment: 78564

Committed r75632: <http://trac.webkit.org/changeset/75632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Committed r75634: <http://trac.webkit.org/changeset/75634>

[WebKit Review Bot]

http://trac.webkit.org/changeset/75632 might have broken Qt Linux Release
The following tests are not passing:
fast/css/pseudo-cache-stale.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/search-cancel-button-style-sharing.html
fast/forms/search-vertical-alignment.html

[Robert Hogan]

Committed r75636: <http://trac.webkit.org/changeset/75636>