Bug 42887

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

Created attachment 72211 [details] Patch

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)

Created attachment 72220 [details] Screenshot

(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 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.)

We might want to override these with our Mobile Theme.

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 on attachment 72211 [details] Patch Clearing flags on attachment: 72211 Committed r73635: <http://trac.webkit.org/changeset/73635>

All reviewed patches have been landed. Closing bug.

Was it rolled out? Should be REOPENED?

(In reply to comment #11) > Was it rolled out? Should be REOPENED? It was rolled out by http://trac.webkit.org/changeset/73642

Created attachment 78431 [details] Patch

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.

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

Created attachment 78436 [details] Patch

Created attachment 78438 [details] Patch

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

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

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

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

> Implies this also lives in WebKit2? Yes, WebKit2/Qt is using our RenderThemeQt. > Maybe a separate patch? Fine

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

Created attachment 78562 [details] Patch

Created attachment 78564 [details] Patch

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.

(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 on attachment 78564 [details] Patch Clearing flags on attachment: 78564 Committed r75632: <http://trac.webkit.org/changeset/75632>

All reviewed patches have been landed. Closing bug.

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

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

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