Bug 85603

Two more: fast/forms/input-placeholder-text-indent.html fast/forms/textarea-placeholder-wrapping.html

*** Bug 85497 has been marked as a duplicate of this bug. ***

Created attachment 140940 [details] Less style overriding.

*** Bug 85953 has been marked as a duplicate of this bug. ***

Comment on attachment 140940 [details] Less style overriding. Removing cq? because I got the test case modification for fast/forms/textarea-placeholder-wrapping.html landed, so I need to update the test_expectations modification in this patch. No code change.

Looks good to me.

This will make Edje theming less flexible, but the current behavior does seem wrong. You can get rid of these foreground/backgroundColor variables now, as they're only used in those context. You should also remove those entries from the theme, as they won't be used anymore. Are those setWhiteSpace calls correct, BTW?

*** Bug 85123 has been marked as a duplicate of this bug. ***

dominik : plus 'fast/forms/cursor-at-editable-content-boundary.html'

Comment on attachment 140940 [details] Less style overriding. Looks fine.

Created attachment 141116 [details] Updated patch 4 baselines added due to PRE change, all placeholder cases unskipped, cursor-at-editable-content-boundary unskipped, RenderThemeEFL and default theme cleaned of color classes.

Comment on attachment 141116 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=141116&action=review As kubo said, it looks foreground/background variables don't need to be kept. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-992 > - style->setWhiteSpace(PRE); Why do you remove setWhiteSpace() only here ?

(In reply to comment #12) > (From update of attachment 141116 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141116&action=review > > As kubo said, it looks foreground/background variables don't need to be kept. Yes, that's why I removed them for all cases except for selection foreground and background. These need to stay. Or did you find any leftovers? > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-992 > > - style->setWhiteSpace(PRE); > > Why do you remove setWhiteSpace() only here ? Because I don't know any issues/failing tests that would conflict with leaving this code there. So, in order to keep my patch minimal I am not touching these in this bug. If we come across failures caused by these PRE overrides, we can reconsider.

Comment on attachment 141116 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=141116&action=review >>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-992 >>> - style->setWhiteSpace(PRE); >> >> Why do you remove setWhiteSpace() only here ? > > Because I don't know any issues/failing tests that would conflict with leaving this code there. So, in order to keep my patch minimal I am not touching these in this bug. If we come across failures caused by these PRE overrides, we can reconsider. If you don't know any issues when setWhiteSpace(PRE) isn't removed, I think we need to keep it in order to be consistent with other function.

(In reply to comment #14) > If you don't know any issues when setWhiteSpace(PRE) isn't removed, I think we need to keep it in order to be consistent with other function. It does in fact solve cases that were previously failing when PRE override is removed for input type text, textareas and search field. My point was that right now I don't know about cases where it helps to remove such a PRE override for Buttons and MenuLists. So, let's keep the patch small and remove it there if needed later. Since the patch as it is now fixes a number of cases and doesn't cause new issues, I think it's ready for landing. As mentioned - if PRE issues come up with buttons or menus, let's handle them separately.

(In reply to comment #15) > (In reply to comment #14) > > If you don't know any issues when setWhiteSpace(PRE) isn't removed, I think we need to keep it in order to be consistent with other function. > > It does in fact solve cases that were previously failing when PRE override is removed for input type text, textareas and search field. My point was that right now I don't know about cases where it helps to remove such a PRE override for Buttons and MenuLists. So, let's keep the patch small and remove it there if needed later. > > Since the patch as it is now fixes a number of cases and doesn't cause new issues, I think it's ready for landing. As mentioned - if PRE issues come up with buttons or menus, let's handle them separately. I'm not sure whether this removing is correct. However, this patch lets failing test cases pass. rs+ on my side informally.

Comment on attachment 141116 [details] Updated patch Rejecting attachment 141116 [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: mium/third_party/v8-i18n --revision 32 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 47>At revision 32. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Total errors found: 0 in 0 files Full output: http://queues.webkit.org/results/12678025

(In reply to comment #17) > (From update of attachment 141116 [details]) > Rejecting attachment 141116 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 I am pretty certain this is a cq flaw wen committing to SVN: "RA layer request failed: Server sent unexpected return value (500 Internal Server Error) in response to CHECKOUT request for '/repository/webkit/!svn/ver/116748/trunk/LayoutTests/platform/efl/fast/css/text-transform-select-expected.txt' at /usr/lib/git-core/git-svn line 572" Can we try again?

Created attachment 141386 [details] Re-upped, ChangeLogs updated

Comment on attachment 141386 [details] Re-upped, ChangeLogs updated Clearing flags on attachment: 141386 Committed r116761: <http://trac.webkit.org/changeset/116761>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 141414 [details] Patch

Comment on attachment 141414 [details] Patch cq+'ing to fix the EFL build ASAP; in the future, please mention in the ChangeLog what revision caused the build to break, and send the patch in a new bug.

Comment on attachment 141414 [details] Patch Clearing flags on attachment: 141414 Committed r116774: <http://trac.webkit.org/changeset/116774>

All reviewed patches have been landed. Closing bug.