Bug 74143 – RenderTheme should have a function for disabled text color adjustment

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

RenderTheme should have a function for disabled text color adjustment.

Created attachment 118743 [details]
Patch

[Kent Tamura]

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.

Created attachment 118746 [details]
Patch 2

[Kent Tamura]

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.

[WebKit Review Bot]

Comment on attachment 118746 [details]
Patch 2

Clearing flags on attachment: 118746

Committed r102655: <http://trac.webkit.org/changeset/102655>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

Reopening to attach new patch.

Created attachment 118941 [details]
Patch 3

[Kent Tamura]

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
> [phone]  Yosifumi Inoue  <[e-mail]>
> +
> +        [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.

Created attachment 118950 [details]
Patch 4

[Kent Tamura]

Comment on attachment 118950 [details]
Patch 4

Please post a patch which can be applied to the latest WebKit revision.

Created attachment 118954 [details]
Patch 5 - Rebase

[Kent Tamura]

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.

Created attachment 118956 [details]
Patch 6 - Rebase

[Kent Tamura]

Comment on attachment 118956 [details]
Patch 6 - Rebase

ok

[WebKit Review Bot]

Comment on attachment 118956 [details]
Patch 6 - Rebase

Clearing flags on attachment: 118956

Committed r102660: <http://trac.webkit.org/changeset/102660>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.