Bug 85603 – [EFL][DRT] Input Attribute Placeholder RefTests failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dominik Röttsches (drott)]

EFL doesn't render the placeholder attribute as gray, needs more investigation.

These tests are failing:
fast/forms/placeholder-with-positioned-element.html
fast/forms/isindex-placeholder.html
fast/forms/textarea-placeholder.html
fast/forms/input-placeholder.html

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

Created attachment 140940 [details]
Less style overriding.

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

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.

[Ryuan Choi]

Looks good to me.

[Raphael Kubo da Costa (:rakuco)]

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?

[Ryuan Choi]

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

[Kangil Han]

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

[Gyuyoung Kim]

Comment on attachment 140940 [details]
Less style overriding.

Looks fine.

[Dominik Röttsches (drott)]

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.

[Gyuyoung Kim]

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 ?

[Dominik Röttsches (drott)]

(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.

[Gyuyoung Kim]

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.

[Dominik Röttsches (drott)]

(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.

[Gyuyoung Kim]

(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.

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

(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?

[Dominik Röttsches (drott)]

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

[WebKit Review Bot]

Comment on attachment 141386 [details]
Re-upped, ChangeLogs updated

Clearing flags on attachment: 141386

Committed r116761: <http://trac.webkit.org/changeset/116761>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

Reopening to attach new patch.

[Dominik Röttsches (drott)]

Created attachment 141414 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[WebKit Review Bot]

Comment on attachment 141414 [details]
Patch

Clearing flags on attachment: 141414

Committed r116774: <http://trac.webkit.org/changeset/116774>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.