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.
Created attachment 109005 [details] proposed patch Another solution to this could be tweaking minColorContrastValue this value but this could be too general.
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.
(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.
(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.
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.
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
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.
Created attachment 109601 [details] updated patch Please review the modified patch. I think other ports needs to rebaseline to pass test case.
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
Created attachment 110131 [details] updated patch commit queue failed as LayoutTests/platform/chromium/test_expectations.txt got modified. Attaching new patch for the same.
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.
(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.
Comment on attachment 110131 [details] updated patch Clearing flags on attachment: 110131 Committed r96958: <http://trac.webkit.org/changeset/96958>
All reviewed patches have been landed. Closing bug.
Mac rebaseline done in http://trac.webkit.org/changeset/97179.
Chromium rebaseline: http://trac.webkit.org/changeset/97247