Bug 85603

Summary: [EFL][DRT] Input Attribute Placeholder RefTests failing
Product: WebKit Reporter: Dominik Röttsches (drott) <d-r>
Component: WebKit EFLAssignee: Dominik Röttsches (drott) <d-r>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, eric, gustavo, gyuyoung.kim, gyuyoung.kim, hausmann, kangil.han, kenneth, lucas.de.marchi, morrita, mrobinson, rakuco, ryuan.choi, tmpsantos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 85877    
Bug Blocks:    
Attachments:
Description Flags
Less style overriding.
none
Updated patch
none
Re-upped, ChangeLogs updated
none
Patch none

Description Dominik Röttsches (drott) 2012-05-04 04:32:05 PDT
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
Comment 1 Dominik Röttsches (drott) 2012-05-04 04:37:58 PDT
Two more:
  fast/forms/input-placeholder-text-indent.html
  fast/forms/textarea-placeholder-wrapping.html
Comment 2 Dominik Röttsches (drott) 2012-05-09 05:22:12 PDT
*** Bug 85497 has been marked as a duplicate of this bug. ***
Comment 3 Dominik Röttsches (drott) 2012-05-09 07:20:02 PDT
Created attachment 140940 [details]
Less style overriding.
Comment 4 Dominik Röttsches (drott) 2012-05-09 09:22:03 PDT
*** Bug 85953 has been marked as a duplicate of this bug. ***
Comment 5 Dominik Röttsches (drott) 2012-05-09 11:31:49 PDT
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.
Comment 6 Ryuan Choi 2012-05-09 16:06:10 PDT
Looks good to me.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-05-09 18:39:05 PDT
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?
Comment 8 Ryuan Choi 2012-05-09 18:48:14 PDT
*** Bug 85123 has been marked as a duplicate of this bug. ***
Comment 9 Kangil Han 2012-05-09 18:55:53 PDT
dominik : plus 'fast/forms/cursor-at-editable-content-boundary.html'
Comment 10 Gyuyoung Kim 2012-05-09 21:52:08 PDT
Comment on attachment 140940 [details]
Less style overriding.

Looks fine.
Comment 11 Dominik Röttsches (drott) 2012-05-10 02:36:23 PDT
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 12 Gyuyoung Kim 2012-05-10 03:32:05 PDT
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 ?
Comment 13 Dominik Röttsches (drott) 2012-05-10 03:43:13 PDT
(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 14 Gyuyoung Kim 2012-05-11 02:45:38 PDT
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.
Comment 15 Dominik Röttsches (drott) 2012-05-11 03:18:37 PDT
(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.
Comment 16 Gyuyoung Kim 2012-05-11 03:37:26 PDT
(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 17 WebKit Review Bot 2012-05-11 04:47:41 PDT
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
Comment 18 Dominik Röttsches (drott) 2012-05-11 04:55:54 PDT
(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?
Comment 19 Dominik Röttsches (drott) 2012-05-11 05:40:40 PDT
Created attachment 141386 [details]
Re-upped, ChangeLogs updated
Comment 20 WebKit Review Bot 2012-05-11 06:39:03 PDT
Comment on attachment 141386 [details]
Re-upped, ChangeLogs updated

Clearing flags on attachment: 141386

Committed r116761: <http://trac.webkit.org/changeset/116761>
Comment 21 WebKit Review Bot 2012-05-11 06:39:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Dominik Röttsches (drott) 2012-05-11 07:25:33 PDT
Reopening to attach new patch.
Comment 23 Dominik Röttsches (drott) 2012-05-11 07:25:40 PDT
Created attachment 141414 [details]
Patch
Comment 24 Raphael Kubo da Costa (:rakuco) 2012-05-11 07:27:43 PDT
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 25 WebKit Review Bot 2012-05-11 08:51:07 PDT
Comment on attachment 141414 [details]
Patch

Clearing flags on attachment: 141414

Committed r116774: <http://trac.webkit.org/changeset/116774>
Comment 26 WebKit Review Bot 2012-05-11 08:51:18 PDT
All reviewed patches have been landed.  Closing bug.