Autocomplete confused about direction if inline style and inherited dir attribute are mixed

Autocomplete confused about direction if inline style and inherited dir attribute are mixed.

Both text box and autocomplete should have same direction i.e when the input box has a direction LTR, then autocomplete popup should also have the same direction
and vice-versa.

The present logic goes for finding dir attribute on the element and its parent which is the reason for autocomplete's incorrect direction when inline style and
inherited dir attribute are mixed. Now, we use renderStyle associated with a particular element to find out the direction (this takes into account both the
effect of dir attribute as well as the inline style).

BUG=397831
TBR=tkent@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179716

[Sunil Ratnu, 2014-08-06 12:05:50]

PTAL.
The browser tests form the same are here:
https://codereview.chromium.org/446843002/

[ebraminio, 2014-08-06 15:14:53]

On 2014/08/06 12:05:50, Sunil Ratnu wrote:
> PTAL.
> The browser tests form the same are here:
> https://codereview.chromium.org/446843002/

Looks great! Just a question, is it possible to do it without the `for (the
element and each of its parents)`? I think that would be very nice also.

[Sunil Ratnu, 2014-08-06 15:27:58]

On 2014/08/06 15:14:53, ebraminiogmail wrote:
> On 2014/08/06 12:05:50, Sunil Ratnu wrote:
> > PTAL.
> > The browser tests form the same are here:
> > https://codereview.chromium.org/446843002/
> 
> Looks great! Just a question, is it possible to do it without the `for (the
> element and each of its parents)`? I think that would be very nice also.

Thanks! I think yes its possible. I tried doing the same without the loop and
that also passes the try servers. I will once again look for try server results
for that approach tomorrow morning.

[Ilya Sherman, 2014-08-06 20:47:36]

+tkent as reviewer

Thanks!  I'm not very familiar with this code, but this change looks reasonable
to me.  Please also implement a layout test for this CL.

[Ilya Sherman, 2014-08-06 20:49:18]

On 2014/08/06 20:47:36, Ilya Sherman wrote:
> Please also implement a layout test for this CL.

To clarify: I did see your browsertest in the Chromium repository, which is
great.  However, since this is a Blink change, it should have test coverage
within the Blink repository.

[Inactive, 2014-08-06 20:49:37]

Why cannot this be layout tested? It would be much better than relying on a
browser test.

[Inactive, 2014-08-06 20:51:44]

https://codereview.chromium.org/419023007/diff/60001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/60001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:661: if (style) {
I cannot say if the change is good or not. However, style-wise, this could be
written as:
if (RenderStyle* style = element->renderStyle())
    return style->isLeftToRightDirection() ? "ltr" : "rtl";

[tkent, 2014-08-07 01:26:56]

HTMLTextFormControlElement::directionForFormData is for |dirname| attribute
processing, and the behavior is defined by the standard.
 
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-dire...
(Looks the current implementation doesn't follow it. We should fix it.)

I think autocomplete UI should not use this function.

[Sunil Ratnu, 2014-08-07 03:21:42]

Thank you all for the review and suggestions. I will upload a new patch with
Layout tests.

On 2014/08/07 01:26:56, tkent wrote:
> HTMLTextFormControlElement::directionForFormData is for |dirname| attribute
> processing, and the behavior is defined by the standard.
> 
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-dire...
> (Looks the current implementation doesn't follow it. We should fix it.)
Thanks tkent for pointing that out, I will try to fix that in a separate patch.
> 
> I think autocomplete UI should not use this function.
Since autocomplete UI should not use this function, we can write a new function
like directionForAutocomplete() in blink side where we can implement the
approach of this patch and then make chromium side call (the only call present
in src/components/autofill/content/renderer/form_autofill_util.cc) this new
fucntion. Please let me know if that sounds fine to you guys?

[Ilya Sherman, 2014-08-07 03:26:10]

On 2014/08/07 03:21:42, Sunil Ratnu wrote:
> Thank you all for the review and suggestions. I will upload a new patch with
> Layout tests.
> 
> On 2014/08/07 01:26:56, tkent wrote:
> > HTMLTextFormControlElement::directionForFormData is for |dirname| attribute
> > processing, and the behavior is defined by the standard.
> > 
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-dire...
> > (Looks the current implementation doesn't follow it. We should fix it.)
> Thanks tkent for pointing that out, I will try to fix that in a separate
patch.
> > 
> > I think autocomplete UI should not use this function.
> Since autocomplete UI should not use this function, we can write a new
function
> like directionForAutocomplete() in blink side where we can implement the
> approach of this patch and then make chromium side call (the only call present
> in src/components/autofill/content/renderer/form_autofill_util.cc) this new
> fucntion. Please let me know if that sounds fine to you guys?

Thanks.  That sounds reasonable to me.  In fact, if we're adding a new function
anyway, perhaps we can just change the implementation of the
WebFormControlElement method, and leave the core classes as they are?  That
would also mean that you can skip the layout test, and just keep the browser
test -- at least, that's how I've seen other WebFoo class functionality tested
historically.  tkent@ is much more familiar with the Blink code than I am, so I
apologize if all of my suggestions here are misguided.

[tkent, 2014-08-07 03:28:52]

On 2014/08/07 03:26:10, Ilya Sherman wrote:
> On 2014/08/07 03:21:42, Sunil Ratnu wrote:
> > Thank you all for the review and suggestions. I will upload a new patch with
> > Layout tests.
> > 
> > On 2014/08/07 01:26:56, tkent wrote:
> > > HTMLTextFormControlElement::directionForFormData is for |dirname|
attribute
> > > processing, and the behavior is defined by the standard.
> > > 
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-dire...
> > > (Looks the current implementation doesn't follow it. We should fix it.)
> > Thanks tkent for pointing that out, I will try to fix that in a separate
> patch.
> > > 
> > > I think autocomplete UI should not use this function.
> > Since autocomplete UI should not use this function, we can write a new
> function
> > like directionForAutocomplete() in blink side where we can implement the
> > approach of this patch and then make chromium side call (the only call
present
> > in src/components/autofill/content/renderer/form_autofill_util.cc) this new
> > fucntion. Please let me know if that sounds fine to you guys?
> 
> Thanks.  That sounds reasonable to me.  In fact, if we're adding a new
function
> anyway, perhaps we can just change the implementation of the
> WebFormControlElement method, and leave the core classes as they are?  That
> would also mean that you can skip the layout test, and just keep the browser
> test -- at least, that's how I've seen other WebFoo class functionality tested
> historically.  tkent@ is much more familiar with the Blink code than I am, so
I
> apologize if all of my suggestions here are misguided.

It sounds reasonable.

[Sunil Ratnu, 2014-08-07 05:20:56]

Thanks! I've incorporated review comments. Please take a look.
In this patch, I've skipped the for loop and just checking the style for that
particular element because while resolving the style, the effect of parent
element is already taken care. Please let me know if I've missed anything here.

https://codereview.chromium.org/419023007/diff/60001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/60001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:661: if (style) {
On 2014/08/06 20:51:43, Chris Dumez wrote:
> I cannot say if the change is good or not. However, style-wise, this could be
> written as:
> if (RenderStyle* style = element->renderStyle())
>     return style->isLeftToRightDirection() ? "ltr" : "rtl";

Done.

[tkent, 2014-08-07 05:25:02]

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:734: String
HTMLTextFormControlElement::directionForAutocomplete() const
You don't need to add new function.
Please replace the content of WebFormControlElement::directionForFormData with
this code.

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:736: if (const HTMLElement*
element = this) {
|this| must not be null.

[Sunil Ratnu, 2014-08-07 07:06:18]

Incorporated review comments. PTAL. Thanks!

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:734: String
HTMLTextFormControlElement::directionForAutocomplete() const
On 2014/08/07 05:25:02, tkent wrote:
> You don't need to add new function.
> Please replace the content of WebFormControlElement::directionForFormData with
> this code.

Done.

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:736: if (const HTMLElement*
element = this) {
On 2014/08/07 05:25:02, tkent wrote:
> |this| must not be null.
|this| can not be NULL here. What i understood it that you wanted to point that
the check here might not be necessary?

https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
File Source/web/WebFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:175: if (isHTMLInputElement(*m_private)) {
Can we have it like this as well?

if (RenderStyle* style =
constUnwrap<HTMLTextFormControlElement>()->renderStyle())
    return style->isLeftToRightDirection() ? "ltr" : "rtl";
return "ltr";

[tkent, 2014-08-07 07:13:46]

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextFormControlElement.cpp:736: if (const HTMLElement*
element = this) {
On 2014/08/07 07:06:18, Sunil Ratnu wrote:
> On 2014/08/07 05:25:02, tkent wrote:
> > |this| must not be null.
> |this| can not be NULL here. What i understood it that you wanted to point
that
> the check here might not be necessary?
> 

Right. The check is unnecessary.

https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
File Source/web/WebFormControlElement.cpp (right):

https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:175: if (isHTMLInputElement(*m_private)) {
On 2014/08/07 07:06:18, Sunil Ratnu wrote:
> Can we have it like this as well?
> 
> if (RenderStyle* style =
> constUnwrap<HTMLTextFormControlElement>()->renderStyle())
>     return style->isLeftToRightDirection() ? "ltr" : "rtl";
> return "ltr";

No.  It doesn't work if m_private is not HTMLTextFormControlElement.

Instead, constUnwrap<HTMLFormControlElement>()->renderStyle() will work.  It's
ok to return the direction of form controls other than INPUT/TEXTAREA.

[Sunil Ratnu, 2014-08-07 07:29:32]

On 2014/08/07 07:13:46, tkent wrote:
>
https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
> File Source/core/html/HTMLTextFormControlElement.cpp (right):
> 
>
https://codereview.chromium.org/419023007/diff/80001/Source/core/html/HTMLTex...
> Source/core/html/HTMLTextFormControlElement.cpp:736: if (const HTMLElement*
> element = this) {
> On 2014/08/07 07:06:18, Sunil Ratnu wrote:
> > On 2014/08/07 05:25:02, tkent wrote:
> > > |this| must not be null.
> > |this| can not be NULL here. What i understood it that you wanted to point
> that
> > the check here might not be necessary?
> > 
> 
> Right. The check is unnecessary.
> 
>
https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
> File Source/web/WebFormControlElement.cpp (right):
> 
>
https://codereview.chromium.org/419023007/diff/100001/Source/web/WebFormContr...
> Source/web/WebFormControlElement.cpp:175: if (isHTMLInputElement(*m_private))
{
> On 2014/08/07 07:06:18, Sunil Ratnu wrote:
> > Can we have it like this as well?
> > 
> > if (RenderStyle* style =
> > constUnwrap<HTMLTextFormControlElement>()->renderStyle())
> >     return style->isLeftToRightDirection() ? "ltr" : "rtl";
> > return "ltr";
> 
> No.  It doesn't work if m_private is not HTMLTextFormControlElement.
> 
> Instead, constUnwrap<HTMLFormControlElement>()->renderStyle() will work.  It's
> ok to return the direction of form controls other than INPUT/TEXTAREA.

Thanks! New patchset now returns the direction of form controls other than
INPUT/TEXTAREA. PTAL.

[tkent, 2014-08-07 07:31:46]

lgtm

[commit-bot: I haz the power, 2014-08-07 07:32:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunil.ratnu@samsung.com/419023007/120001

[Sunil Ratnu, 2014-08-07 07:46:11]

The CQ bit was unchecked by sunil.ratnu@samsung.com

[Sunil Ratnu, 2014-08-07 13:01:04]

On 2014/08/07 07:46:11, Sunil Ratnu wrote:
> The CQ bit was unchecked by mailto:sunil.ratnu@samsung.com

Hi tkent, I've TBRed you on this as I had made a minor change after you gave L G
T M. PTAL.

[Sunil Ratnu, 2014-08-07 13:01:09]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-08-07 13:02:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunil.ratnu@samsung.com/419023007/140001

[commit-bot: I haz the power, 2014-08-07 14:24:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-07 14:31:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 14:31:29]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[Sunil Ratnu, 2014-08-07 16:07:22]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-08-07 16:08:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunil.ratnu@samsung.com/419023007/140001

[commit-bot: I haz the power, 2014-08-07 16:09:15]

Change committed as 179716