Bug 30547 – (Chromium) searchbox not rendered properly due to the css property -webkit-border-radius

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jonathan Dixon]

Created attachment 41465 [details]
Screen shot of mis-rendered search box

(cross post from http://code.google.com/p/chromium/issues/detail?id=15665)

Chrome Version       : 3.0.182.3 (Offizieller Build 17055)
URLs (if applicable) : all Apple websites
Other browsers tested: yes
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 4:OK
  Firefox 3.x:OK
         IE 7:OK
         IE 8:OK

What steps will reproduce the problem?
1.visit any Apple website and start typing something into the search box in 
the upper right hand corner 


What is the expected result?
- Searchbox should be displayed properly and should suggest items as you type.


What happens instead?
- searchbox not displayed properly and no suggestions appear.

[Jonathan Dixon]

Created attachment 41467 [details]
Proposed fix - implement rounded border rendering on the text field painter

Tested by running in side a Chromium build.
The search box background color is still grey on www.apple.com, but this is in line with what the site's css is setting.

[Eric Seidel (no email)]

Comment on attachment 41467 [details]
Proposed fix - implement rounded border rendering on the text field painter

 8         No new tests: this is chromium only code.
Does not mean it doesn't need testing!

Eventually Chromium's results will be in webkit.org and thus we won't have this issue at all.  For now, if this is covered by existing tsts, please list those tests.  If it is not, then we need to add a new test and check in results for the other platforms.

[Jonathan Dixon]

Created attachment 41633 [details]
Prosed fix, with test

[Eric Seidel (no email)]

Comment on attachment 41633 [details]
Prosed fix, with test

Tabs in the ChangeLog will cause the commit to fail.

[Jonathan Dixon]

Created attachment 41671 [details]
Identical to previous patch, but with tabs removed from ChangeLog

Many thanks! Not even sure how the tabs got in there...

[Jonathan Dixon]

Created attachment 41672 [details]
Proposed fix (with tabs removed)

Agh didn't click the patch tick box, lets try that again...

[Eric Seidel (no email)]

Comment on attachment 41672 [details]
Proposed fix (with tabs removed)

Looks like you've added results only to platform/mac.  Either we need to have results in a shared location (right next to the test) or we'll need results for all the other platforms.  It looks like this is a rendering change, so it can't be dumpAsText, so we may end up needing results for all the other platforms. :(  Sadly we don't have an easy way to do that, although it is possible to check-in and ask nicely for others to add the results for you, or grab the new results off of the buildbots after your checkin.

[Jonathan Dixon]

Created attachment 41770 [details]
Proposed fix + test, with -expected files in global (not platform specific) location

Eric suggested moving the expected files to the global location, and adding other platforms in if we pickup differences on the try bots

[Eric Seidel (no email)]

Comment on attachment 41770 [details]
Proposed fix + test, with -expected files in global (not platform specific) location

Looks fine to me.  This shouldn't be landed by the queue though since it's expected to cause failures on other platforms.

[Jeremy Orlow]

Comment on attachment 41770 [details]
Proposed fix + test, with -expected files in global (not platform specific) location

Joth is going to watch it land.

[Eric Seidel (no email)]

Seems like a bad idea given how backed up the queue is right now: http://webkit-commit-queue.appspot.com/ (mostly due to the tree being red all day).  Also since this is expected to fail, a committer should be around to clean up the mess.  Does joth have commit bit?  And if so, why wouldnt' he just commit this himself?  I recommend cq-ing this.

[Eric Seidel (no email)]

The commit-queue is super-useful for set-and-forget patches, where they are expected to be commited w/o any issue, and letting the bot do it later makes perfect sense.  cq isn't very good for patches which require committer oversight. :(

[Jonathan Dixon]

It's not clear to me what webkit failures are expected? Dimitri thought that only safari-mac expected output is required in the webkit submit (which this patch has) so it should go through fine. There will be a follow on step to clean up the chromium tree after this lands, but that's beyond the scope of this commit.
Point taken about the webkit tree being red all day though :-(

[WebKit Commit Bot]

Comment on attachment 41770 [details]
Proposed fix + test, with -expected files in global (not platform specific) location

Rejecting patch 41770 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
Log
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/css/text-input-with-webkit-border-radius.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Jonathan Dixon]

Created attachment 42046 [details]
Fix previous patch which failed (tabs in the .html file)

Is there a pre-submit lint tool for patches?

[Eric Seidel (no email)]

check-webkit-style does a minor amount of linting.  It's derived from Google's cpp_lint.  It needs to be expanded to deal with things like mising ChangeLogs and tabs though. :(  You should feel encouraged to file bugs about us having better linting tools.

[WebKit Commit Bot]

Comment on attachment 42046 [details]
Fix previous patch which failed (tabs in the .html file)

Clearing flags on attachment: 42046

Committed r50237: <http://trac.webkit.org/changeset/50237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Dixon]

Created attachment 42080 [details]
Add tests to skip list for platforms that do not yet have expected output

[Eric Seidel (no email)]

Seems it would be easier to just get the results from the bots and check those in, rather than skipping. or?

[David Levin]

Comment on attachment 42080 [details]
Add tests to skip list for platforms that do not yet have expected output

r- for no changelog and Eric's comment above: https://bugs.webkit.org/show_bug.cgi?id=30547#c20

[Jonathan Dixon]

Created attachment 42160 [details]
Add expected output for GTK & QT (as created by build bots)

[Eric Seidel (no email)]

Comment on attachment 42160 [details]
Add expected output for GTK & QT (as created by build bots)

Needs a ChangeLog. :)  Also we'll need to re-open the bug if we want things like the commit-queue to notice the patches. :)  I'll make sure you have edit bug privs if you dont' already.

[Jonathan Dixon]

Re-opening in order to add patch to fixup GTK & QT tests.
Will add ChangleLog to the patch momentarily.
Thanks!

[Jonathan Dixon]

Created attachment 42164 [details]
Add expected output for GTK & QT (this time with ChangeLog)

I'll get the hang of this eventually! Thanks.

[Eric Seidel (no email)]

Comment on attachment 42164 [details]
Add expected output for GTK & QT (this time with ChangeLog)

Lovely!

[WebKit Commit Bot]

Comment on attachment 42164 [details]
Add expected output for GTK & QT (this time with ChangeLog)

Clearing flags on attachment: 42164

Committed r50314: <http://trac.webkit.org/changeset/50314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.