Bug 78547 - Add a themeChromiumAndroid.css file for android-specific default styles
Summary: Add a themeChromiumAndroid.css file for android-specific default styles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-02-13 16:03 PST by Eric Seidel (no email)
Modified: 2012-02-16 16:47 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.39 KB, patch)
2012-02-13 16:08 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (6.57 KB, patch)
2012-02-16 10:47 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-02-13 16:03:47 PST
Add a themeChromiumAndroid.css file for android-specific default styles
Comment 1 Eric Seidel (no email) 2012-02-13 16:08:40 PST
Created attachment 126856 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-13 16:09:58 PST
Tkent might be curious to see the styling of the datetime inputs, but I think this can just be a rubber-stamp review, it's very simple.
Comment 3 Satish Sampath 2012-02-13 16:15:12 PST
Comment on attachment 126856 [details]
Patch

> static const RGBA32 defaultTapHighlightColor = 0x6633b5e5;
Should this be under "#if ENABLE(TOUCH_EVENTS)" as well?
Comment 4 Adam Barth 2012-02-13 16:15:23 PST
Comment on attachment 126856 [details]
Patch

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

Ok.

> Source/WebCore/rendering/RenderThemeChromiumAndroid.h:50
>  private:

Missing a blank line above this line.
Comment 5 Peter Beverloo 2012-02-13 16:24:06 PST
Comment on attachment 126856 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:825
>                '../css/themeChromiumSkia.css',  # Chromium only.

nit: it seems like someone tried to align them, may be good as a drive-by to remove the extra space here.

> Source/WebCore/css/themeChromiumAndroid.css:35
> +select[size][multiple] {

Is this last selector necessary? It seems to me that any elements having both @size and @multiple also match one of the earlier two selectors.
Comment 6 Kent Tamura 2012-02-13 16:46:20 PST
Comment on attachment 126856 [details]
Patch

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

> Source/WebCore/css/themeChromiumAndroid.css:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2012?

> Source/WebCore/css/themeChromiumAndroid.css:45
> +input[type="date"], input[type="datetime"], input[type="datetime-local"], input[type="time"], input[type="month"] {
> +    -webkit-appearance: menulist-button;
> +}

I understand how they are implemented in Android and it's ok to commit this as is for now.
These types inherit a text field type in WebCore/html/*InputType. We should make a better way to switch form control UI.

> Source/WebCore/rendering/RenderThemeChromiumAndroid.h:45
> +    virtual String extraDefaultStyleSheet();
>  
>      virtual Color systemColor(int cssValidId) const;
>  
>      virtual void adjustInnerSpinButtonStyle(CSSStyleSelector*, RenderStyle*, Element*) const;
>  
> +    virtual bool delegatesMenuListRendering() const { return true; }
> +
> +#if ENABLE(TOUCH_EVENTS)
> +    virtual Color platformTapHighlightColor() const

Should append OVERRIDE.
Comment 7 Eric Seidel (no email) 2012-02-13 16:48:02 PST
Wow!  Thanks for all the reviews!  Will upload a new patch.
Comment 8 Eric Seidel (no email) 2012-02-16 10:45:02 PST
(In reply to comment #5)
> (From update of attachment 126856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126856&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:825
> >                '../css/themeChromiumSkia.css',  # Chromium only.
> 
> nit: it seems like someone tried to align them, may be good as a drive-by to remove the extra space here.

I don't really see any alignment pattern.  I can remove the extra space from the chromiumskia one, but I think I'll just leave it:

              '../css/themeChromium.css', # Chromium only.
              '../css/themeChromiumAndroid.css', # Chromium only.
              '../css/themeChromiumLinux.css', # Chromium only.
              '../css/themeChromiumSkia.css',  # Chromium only.

> > Source/WebCore/css/themeChromiumAndroid.css:35
> > +select[size][multiple] {
> 
> Is this last selector necessary? It seems to me that any elements having both @size and @multiple also match one of the earlier two selectors.

No clue.
Comment 9 Eric Seidel (no email) 2012-02-16 10:47:54 PST
Created attachment 127408 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-02-16 16:47:11 PST
Comment on attachment 127408 [details]
Patch for landing

Clearing flags on attachment: 127408

Committed r107998: <http://trac.webkit.org/changeset/107998>
Comment 11 WebKit Review Bot 2012-02-16 16:47:17 PST
All reviewed patches have been landed.  Closing bug.