background color affects the font color in disabled textarea

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 102333 [details]
demo

font color of disabled textarea is affected by the background color.  Namely, textarea with background-color: white and background-color: transparent result in different alpha values for the font color.

[Vineet Chaudhary (vineetc)]

Created attachment 109005 [details]
proposed patch

Another solution to this could be tweaking minColorContrastValue this value but this could be too general.

[Darin Adler]

Comment on attachment 109005 [details]
proposed patch

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

Checking specifically for the Color::transparent value is not right. The right way to check that something is transparent is to check its alpha channel only, not to explicitly check for a particular RGB as well as alpha. We could have a minimum alpha value. It's a good insight, though, that difference from white is meaningless for a transparent or mostly transparent color.

To refine the rule, I’m not sure if simply using lightening unconditionally is the right way to handle a transparent or translucent background color. It seems we’d want to figure out the likely color from behind the text to have a good chance of getting the behavior right.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

Bug fixes need to come with test cases. That includes this one.

[Vineet Chaudhary (vineetc)]

(In reply to comment #2)
> (From update of attachment 109005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109005&action=review
> 
> Checking specifically for the Color::transparent value is not right. The right way to check that something is transparent is to check its alpha channel only, not to explicitly check for a particular RGB as well as alpha. We could have a minimum alpha value. It's a good insight, though, that difference from white is meaningless for a transparent or mostly transparent color.
> 
> To refine the rule, I’m not sure if simply using lightening unconditionally is the right way to handle a transparent or translucent background color. It seems we’d want to figure out the likely color from behind the text to have a good chance of getting the behavior right.

Yes we should check for backgroundColor.alpha() while setting the textColor, as below

backgroundColor.alpha() < 0.5  (0.5 or a value which we decide upon.)

Please provide your thought.

> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests. (OOPS!)
> 
> Bug fixes need to come with test cases. That includes this one.

[Darin Adler]

(In reply to comment #3)
> Yes we should check for backgroundColor.alpha() while setting the textColor, as below
> 
> backgroundColor.alpha() < 0.5  (0.5 or a value which we decide upon.)

Sounds OK. Not sure it will handle all cases well, but better than the current code or the current patch.

[Vineet Chaudhary (vineetc)]

Created attachment 109462 [details]
Updated Patch

(In reply to comment #4)
> (In reply to comment #3)
> > Yes we should check for backgroundColor.alpha() while setting the textColor, as below
> > 
> > backgroundColor.alpha() < 0.5  (0.5 or a value which we decide upon.)
> 
> Sounds OK. Not sure it will handle all cases well, but better than the current code or the current patch.

Attached updated patch as per review comments.
Also extended test case input-disabled-color.html to check this behavior.

[WebKit Review Bot]

Comment on attachment 109462 [details]
Updated Patch

Attachment 109462 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9928384

New failing tests:
fast/forms/input-disabled-color.html

[Darin Adler]

Comment on attachment 109462 [details]
Updated Patch

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

> LayoutTests/ChangeLog:12
> +        * platform/gtk/fast/forms/input-disabled-color-expected.png:
> +        * platform/gtk/fast/forms/input-disabled-color-expected.txt:

Patch will need output for platforms other than GTK. Landing this with GTK results only will make all the bots red.

[Vineet Chaudhary (vineetc)]

Created attachment 109601 [details]
updated patch

Please review the modified patch.
I think other ports needs to rebaseline to pass test case.

[WebKit Review Bot]

Comment on attachment 109601 [details]
updated patch

Rejecting attachment 109601 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/fast/forms/input-disabled-color-expected.txt
patching file LayoutTests/platform/mac/test_expectations.txt
patching file LayoutTests/platform/qt/test_expectations.txt
patching file LayoutTests/platform/win/test_expectations.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10009021

[Vineet Chaudhary (vineetc)]

Created attachment 110131 [details]
updated patch

commit queue failed as LayoutTests/platform/chromium/test_expectations.txt got modified.
Attaching new patch for the same.

[Darin Adler]

I don’t think this patch correctly skips the test on the Mac, since new-run-webkit-tests is not yet used on the Mac so test expectations are not used there.

[Ryosuke Niwa]

(In reply to comment #11)
> I don’t think this patch correctly skips the test on the Mac, since new-run-webkit-tests is not yet used on the Mac so test expectations are not used there.

As far as I know, Mac port currently uses nrwt. Mac WebKit2 port doesn't, however.

[WebKit Review Bot]

Comment on attachment 110131 [details]
updated patch

Clearing flags on attachment: 110131

Committed r96958: <http://trac.webkit.org/changeset/96958>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Mac rebaseline done in http://trac.webkit.org/changeset/97179.

[Kent Tamura]

Chromium rebaseline: http://trac.webkit.org/changeset/97247