Bug 42887 - [Qt] Search input field doesn't have cancel button
Summary: [Qt] Search input field doesn't have cancel button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt, QtTriaged
Depends on: 50778
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-23 04:15 PDT by Csaba Osztrogonác
Modified: 2011-01-12 13:12 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.89 KB, patch)
2010-10-28 11:34 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Screenshot (26.63 KB, image/png)
2010-10-28 12:48 PDT, Robert Hogan
no flags Details
Patch (16.45 KB, patch)
2011-01-10 12:46 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.26 KB, patch)
2011-01-10 13:04 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.27 KB, patch)
2011-01-10 13:07 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (18.32 KB, patch)
2011-01-11 11:27 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (18.35 KB, patch)
2011-01-11 11:31 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-07-23 04:15:28 PDT
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)
Comment 1 Csaba Osztrogonác 2010-07-23 04:25:45 PDT
fast/forms/search-cancel-button-events.html is skipped by http://trac.webkit.org/changeset/63960 (until fix)
Comment 2 Robert Hogan 2010-10-28 11:34:58 PDT
Created attachment 72211 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2010-10-28 11:40:00 PDT
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)
Comment 4 Robert Hogan 2010-10-28 12:48:29 PDT
Created attachment 72220 [details]
Screenshot
Comment 5 Robert Hogan 2010-10-28 12:58:18 PDT
(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.
Comment 6 Andreas Kling 2010-11-20 18:03:45 PST
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.)
Comment 7 Kenneth Rohde Christiansen 2010-11-22 00:37:31 PST
We might want to override these with our Mobile Theme.
Comment 8 WebKit Commit Bot 2010-12-08 13:38:51 PST
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
Comment 9 WebKit Commit Bot 2010-12-09 11:54:33 PST
Comment on attachment 72211 [details]
Patch

Clearing flags on attachment: 72211

Committed r73635: <http://trac.webkit.org/changeset/73635>
Comment 10 WebKit Commit Bot 2010-12-09 11:54:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Antonio Gomes 2010-12-12 12:22:34 PST
Was it rolled out? Should be REOPENED?
Comment 12 Csaba Osztrogonác 2010-12-13 02:09:09 PST
(In reply to comment #11)
> Was it rolled out? Should be REOPENED?

It was rolled out by http://trac.webkit.org/changeset/73642
Comment 13 Robert Hogan 2011-01-10 12:46:07 PST
Created attachment 78431 [details]
Patch
Comment 14 Kenneth Rohde Christiansen 2011-01-10 13:01:42 PST
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.
Comment 15 Kenneth Rohde Christiansen 2011-01-10 13:02:57 PST
Christian, can you get John to make some graphics for the mobile theme?
Comment 16 Robert Hogan 2011-01-10 13:04:14 PST
Created attachment 78436 [details]
Patch
Comment 17 Robert Hogan 2011-01-10 13:07:41 PST
Created attachment 78438 [details]
Patch
Comment 18 Kenneth Rohde Christiansen 2011-01-10 13:14:23 PST
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
Comment 19 Robert Hogan 2011-01-10 13:25:32 PST
(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.
Comment 20 Kenneth Rohde Christiansen 2011-01-10 13:28:24 PST
(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.
Comment 21 Robert Hogan 2011-01-10 13:29:54 PST
(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.
Comment 22 Kenneth Rohde Christiansen 2011-01-10 15:04:49 PST
> Implies this also lives in WebKit2?

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

Fine
Comment 23 Robert Hogan 2011-01-11 10:50:11 PST
Jocelyn, could you respond to the last part of https://bugs.webkit.org/show_bug.cgi?id=42887#c21 ?
Comment 24 Robert Hogan 2011-01-11 11:27:26 PST
Created attachment 78562 [details]
Patch
Comment 25 Robert Hogan 2011-01-11 11:31:45 PST
Created attachment 78564 [details]
Patch
Comment 26 WebKit Commit Bot 2011-01-11 14:06:25 PST
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: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 27 Jocelyn Turcotte 2011-01-12 02:42:28 PST
(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.
Comment 28 WebKit Commit Bot 2011-01-12 12:27:59 PST
Comment on attachment 78564 [details]
Patch

Clearing flags on attachment: 78564

Committed r75632: <http://trac.webkit.org/changeset/75632>
Comment 29 WebKit Commit Bot 2011-01-12 12:28:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Robert Hogan 2011-01-12 13:04:29 PST
Committed r75634: <http://trac.webkit.org/changeset/75634>
Comment 31 WebKit Review Bot 2011-01-12 13:08:28 PST
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
Comment 32 Robert Hogan 2011-01-12 13:12:59 PST
Committed r75636: <http://trac.webkit.org/changeset/75636>