Bug 74143 - RenderTheme should have a function for disabled text color adjustment
Summary: RenderTheme should have a function for disabled text color adjustment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 54643
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 17:05 PST by Kent Tamura
Modified: 2011-12-13 00:17 PST (History)
2 users (show)

See Also:


Attachments
Patch (8.39 KB, patch)
2011-12-11 23:43 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (8.08 KB, patch)
2011-12-12 00:13 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (8.79 KB, patch)
2011-12-12 19:49 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (8.12 KB, patch)
2011-12-12 21:32 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 5 - Rebase (3.48 KB, patch)
2011-12-12 22:09 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 6 - Rebase (3.06 KB, patch)
2011-12-12 22:54 PST, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-12-08 17:05:37 PST
RenderTheme should have a function for disabled text color adjustment.
Comment 1 yosin 2011-12-11 23:43:40 PST
Created attachment 118743 [details]
Patch
Comment 2 Kent Tamura 2011-12-11 23:53:39 PST
Comment on attachment 118743 [details]
Patch

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

> Source/WebCore/ChangeLog:12337
>          * css/html.css:
> + 
>          (bdi, output): bdi and output should both use -webkit-isolate as the default value for unicode-bidi.

Do not modify unrelated ChangeLog entry.

> Source/WebCore/rendering/RenderTheme.h:149
> +    // Disabled text color for disabled text controls.

The comment is not helpful.  Please remove it.

> Source/WebCore/rendering/RenderThemeChromiumMac.h:62
> +    virtual Color disabledTextColor(const Color& textColor, const Color&) const OVERRIDE { return textColor; }
> +

No reason to make this protected.

> Source/WebCore/rendering/RenderThemeChromiumSkia.h:155
> +    virtual Color disabledTextColor(const Color& textColor, const Color&) const OVERRIDE { return textColor; }
> +

ditto.
Comment 3 yosin 2011-12-12 00:13:03 PST
Created attachment 118746 [details]
Patch 2
Comment 4 Kent Tamura 2011-12-12 18:25:23 PST
Comment on attachment 118746 [details]
Patch 2

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

> Source/WebCore/rendering/RenderThemeChromiumMac.h:35
> +    virtual Color disabledTextColor(const Color& textColor, const Color&) const OVERRIDE { return textColor; }

nit: This function can be private.

> Source/WebCore/rendering/RenderThemeChromiumSkia.h:61
> +        virtual Color disabledTextColor(const Color& textColor, const Color&) const OVERRIDE { return textColor; }

dito.
Comment 5 WebKit Review Bot 2011-12-12 19:49:10 PST
Comment on attachment 118746 [details]
Patch 2

Clearing flags on attachment: 118746

Committed r102655: <http://trac.webkit.org/changeset/102655>
Comment 6 WebKit Review Bot 2011-12-12 19:49:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 yosin 2011-12-12 19:49:30 PST
Reopening to attach new patch.
Comment 8 yosin 2011-12-12 19:49:33 PST
Created attachment 118941 [details]
Patch 3
Comment 9 Kent Tamura 2011-12-12 20:37:33 PST
Comment on attachment 118941 [details]
Patch 3

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

Your "Patch 2" was already landed. This patch doesn't work.

> LayoutTests/ChangeLog:10
> +2011-12-13  Yosifumi Inoue  <yosin@chromium.org>
> +
> +        [Forms] Disabled option selected in form
> +        https://bugs.webkit.org/show_bug.cgi?id=74270
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/forms/select/menulist-disabled-option-expected.html: Added.
> +        * fast/forms/select/menulist-disabled-option.html: Added.
> +

Do not add unrelated ChangeLog.
Comment 10 yosin 2011-12-12 21:32:41 PST
Created attachment 118950 [details]
Patch 4
Comment 11 Kent Tamura 2011-12-12 21:47:40 PST
Comment on attachment 118950 [details]
Patch 4

Please post a patch which can be applied to the latest WebKit revision.
Comment 12 yosin 2011-12-12 22:09:27 PST
Created attachment 118954 [details]
Patch 5 - Rebase
Comment 13 Kent Tamura 2011-12-12 22:11:23 PST
Comment on attachment 118954 [details]
Patch 5 - Rebase

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

> Source/WebCore/ChangeLog:16
> +        * rendering/RenderTextControl.cpp:
> +        (WebCore::RenderTextControl::adjustInnerTextStyle): Use RenderTheme::disabledTextColor instead of PLATFORM wraped static function. 
> +        * rendering/RenderTheme.cpp:
> +        (WebCore::RenderTheme::disabledTextColor): Moved from RenderTextControl.cpp. This method implements for non-Chromium color.
> +        * rendering/RenderTheme.h: Add new virtual method disabledTextColor.
> +        * rendering/RenderThemeChromiumMac.h: Implementation of RenderTheme::disabledTextColor for Chrimium Mac.
> +        * rendering/RenderThemeChromiumSkia.h: Implementation of RenderTheme::disabledTextColor for Chrimium.

Please update the file list.
Comment 14 yosin 2011-12-12 22:54:54 PST
Created attachment 118956 [details]
Patch 6 - Rebase
Comment 15 Kent Tamura 2011-12-12 23:02:49 PST
Comment on attachment 118956 [details]
Patch 6 - Rebase

ok
Comment 16 WebKit Review Bot 2011-12-13 00:17:46 PST
Comment on attachment 118956 [details]
Patch 6 - Rebase

Clearing flags on attachment: 118956

Committed r102660: <http://trac.webkit.org/changeset/102660>
Comment 17 WebKit Review Bot 2011-12-13 00:17:52 PST
All reviewed patches have been landed.  Closing bug.