Changed the default value for Empty Dir Attribute

Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can inherit the style from parent.

BUG=578262
Committed: https://crrev.com/27dd55c82868e21611193b5f611d5ddbad18d5aa
Cr-Commit-Position: refs/heads/master@{#378186}

[chakshu, 2016-02-02 06:37:04]

Description was changed from

==========
WIP

BUG=
==========

to

==========
Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can
inherit the style from parent.

BUG=578262
==========

[chakshu, 2016-02-02 06:37:05]

chakshu.a@samsung.com changed reviewers:
+ ramya.v@samsung.com, sanjoy.pal@samsung.com

[pals, 2016-02-02 06:46:52]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-02 06:47:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/20001

[commit-bot: I haz the power, 2016-02-02 06:58:42]

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

[commit-bot: I haz the power, 2016-02-02 06:58:43]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[RaviKasibhatla, 2016-02-08 14:00:07]

The CQ bit was checked by kphanee@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 14:00:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/40001

[commit-bot: I haz the power, 2016-02-08 15:08:34]

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

[commit-bot: I haz the power, 2016-02-08 15:08:34]

Dry run: This issue passed the CQ dry run.

[pals, 2016-02-09 10:45:01]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[pals, 2016-02-09 10:45:34]

On 2016/02/08 15:08:34, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Looks good.

[commit-bot: I haz the power, 2016-02-09 10:45:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/60001

[commit-bot: I haz the power, 2016-02-09 12:29:21]

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

[commit-bot: I haz the power, 2016-02-09 12:29:21]

Dry run: This issue passed the CQ dry run.

[chakshu, 2016-02-09 12:31:25]

chakshu.a@samsung.com changed reviewers:
+ kojii@chromium.org

[chakshu, 2016-02-11 05:55:37]

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:237: } else {
@kojii Can you look at this fix for default value for invalid dir attribute, as
per the discussion we had?

[kojii, 2016-02-11 13:51:17]

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:239: if
(hasTagName(bodyTag))
Probably isHTMLBodyElement() is better? I'm not certain how practically it
differs though.

Also, minor, but this code adds CSSPropertyDirection twice if body. Better to
add only once.

[chakshu, 2016-02-12 06:04:10]

Changed as per review comments

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:239: if
(hasTagName(bodyTag))
On 2016/02/11 13:51:16, kojii wrote:
> Probably isHTMLBodyElement() is better? I'm not certain how practically it
> differs though.
> 
> Also, minor, but this code adds CSSPropertyDirection twice if body. Better to
> add only once.

isHTMLBodyElement() is basically the wrapper function for hasTagName(bodyTag) so
yeah, it is better to use it but practically it does not differ though. Fixed
the changes asked for.

[kojii, 2016-02-12 07:13:06]

kojii@chromium.org changed reviewers:
+ leviw@chromium.org

[kojii, 2016-02-12 07:13:07]

+leviw@

non-owner LGTM with nit.

https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:238: if
(isHTMLBodyElement(*this))
nit: better to use "else if"; i.e.,

if (...)
  ...
else if (...)
  ...
else
  ...

[chakshu, 2016-02-15 05:48:47]

On 2016/02/12 07:13:07, kojii wrote:
> +leviw@
> 
> non-owner LGTM with nit.
> 
>
https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> 
>
https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLElement.cpp:238: if
> (isHTMLBodyElement(*this))
> nit: better to use "else if"; i.e.,
> 
> if (...)
>   ...
> else if (...)
>   ...
> else
>   ...

@leviw Can you please take a look at this?

[chakshu, 2016-02-15 05:49:25]

Changed as per the review comments

https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:238: if
(isHTMLBodyElement(*this))
On 2016/02/12 07:13:07, kojii wrote:
> nit: better to use "else if"; i.e.,
> 
> if (...)
>   ...
> else if (...)
>   ...
> else
>   ...

Done.

[kojii, 2016-02-16 07:39:01]

https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.cpp:240:
addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "");
I should have noticed before, sorry, but is this line necessary? Adding an empty
string means it's not a valid style, so this line should be no-op. Can you try
if it still works without this?

[chakshu, 2016-02-18 07:43:42]

The CQ bit was checked by chakshu.a@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 07:44:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/120001

[commit-bot: I haz the power, 2016-02-18 08:57:58]

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

[commit-bot: I haz the power, 2016-02-18 08:57:58]

Dry run: This issue passed the CQ dry run.

[chakshu, 2016-02-18 09:00:58]

Changed as per review comments

https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.cpp:240:
addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "");
On 2016/02/16 07:39:01, kojii wrote:
> I should have noticed before, sorry, but is this line necessary? Adding an
empty
> string means it's not a valid style, so this line should be no-op. Can you try
> if it still works without this?

Oh yes, even I didn't realize that.
Made the change now. It works !

[kojii, 2016-02-18 10:16:25]

On 2016/02/18 at 09:00:58, chakshu.a wrote:
> Changed as per review comments
> 
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> 
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLElement.cpp:240:
addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "");
> On 2016/02/16 07:39:01, kojii wrote:
> > I should have noticed before, sorry, but is this line necessary? Adding an
empty
> > string means it's not a valid style, so this line should be no-op. Can you
try
> > if it still works without this?
> 
> Oh yes, even I didn't realize that.
> Made the change now. It works !


Thank you for working through several reviews, non-owner LGTM again.

leviw@, can quickly look at this?

[chakshu, 2016-02-27 12:18:58]

On 2016/02/18 10:16:25, kojii wrote:
> On 2016/02/18 at 09:00:58, chakshu.a wrote:
> > Changed as per review comments
> > 
> >
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLElement.cpp:240:
> addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "");
> > On 2016/02/16 07:39:01, kojii wrote:
> > > I should have noticed before, sorry, but is this line necessary? Adding an
> empty
> > > string means it's not a valid style, so this line should be no-op. Can you
> try
> > > if it still works without this?
> > 
> > Oh yes, even I didn't realize that.
> > Made the change now. It works !
> 
> 
> Thank you for working through several reviews, non-owner LGTM again.
> 
> leviw@, can quickly look at this?

Friendly ping again.

[chakshu, 2016-02-27 12:19:07]

On 2016/02/18 10:16:25, kojii wrote:
> On 2016/02/18 at 09:00:58, chakshu.a wrote:
> > Changed as per review comments
> > 
> >
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1653313002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLElement.cpp:240:
> addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "");
> > On 2016/02/16 07:39:01, kojii wrote:
> > > I should have noticed before, sorry, but is this line necessary? Adding an
> empty
> > > string means it's not a valid style, so this line should be no-op. Can you
> try
> > > if it still works without this?
> > 
> > Oh yes, even I didn't realize that.
> > Made the change now. It works !
> 
> 
> Thank you for working through several reviews, non-owner LGTM again.
> 
> leviw@, can quickly look at this?

Friendly ping again.

[kojii, 2016-02-29 02:15:01]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2016-02-29 02:19:42]

+eae@ can you have a look? The original fix to this line was by leviw in
crbug.com/84428, but is this simple enough not to need to bother him?

[eae, 2016-02-29 05:21:09]

LGTM

[eae, 2016-02-29 05:21:36]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-02-29 05:21:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/120001

[commit-bot: I haz the power, 2016-02-29 07:38:31]

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

[commit-bot: I haz the power, 2016-02-29 07:38:32]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chakshu, 2016-02-29 07:57:00]

The CQ bit was checked by chakshu.a@samsung.com

[commit-bot: I haz the power, 2016-02-29 07:57:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653313002/120001

[commit-bot: I haz the power, 2016-02-29 09:27:41]

Description was changed from

==========
Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can
inherit the style from parent.

BUG=578262
==========

to

==========
Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can
inherit the style from parent.

BUG=578262
==========

[commit-bot: I haz the power, 2016-02-29 09:27:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-29 09:28:57]

Description was changed from

==========
Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can
inherit the style from parent.

BUG=578262
==========

to

==========
Changed the default value for Empty Dir Attribute.

Empty dir attribute was set to "ltr" and now it is changed to "" so that it can
inherit the style from parent.

BUG=578262

Committed: https://crrev.com/27dd55c82868e21611193b5f611d5ddbad18d5aa
Cr-Commit-Position: refs/heads/master@{#378186}
==========

[commit-bot: I haz the power, 2016-02-29 09:28:58]

Patchset 7 (id:??) landed as
https://crrev.com/27dd55c82868e21611193b5f611d5ddbad18d5aa
Cr-Commit-Position: refs/heads/master@{#378186}

[tkent, 2016-02-29 23:36:38]

Please do not add a test to LayoutTests/imported manually.

Does this work well if nested <body> elements?

[kojii, 2016-03-01 02:21:57]

On 2016/02/29 at 23:36:38, tkent wrote:
> Please do not add a test to LayoutTests/imported manually.

Right, my bad to miss that.

chakshu.a@, can you move the test files to somewhere else? Probably
LayoutTests/fast/dom/Attr/ (where the test file for crrev.com/31083009 is) is
good.

> Does this work well if nested <body> elements?

Probably not, but it's still an improvement, and solves a real issue in
wikipedia, so I think it's good to land.

[chakshu, 2016-03-02 09:56:03]

On 2016/03/01 02:21:57, kojii wrote:
> On 2016/02/29 at 23:36:38, tkent wrote:
> > Please do not add a test to LayoutTests/imported manually.
> 
> Right, my bad to miss that.
> 
> chakshu.a@, can you move the test files to somewhere else? Probably
> LayoutTests/fast/dom/Attr/ (where the test file for crrev.com/31083009 is) is
> good.
> 
Sorry about that. Have fixed it now and moved them to LayoutTests/fast/dom/Attr/
https://codereview.chromium.org/1760443002/

> > Does this work well if nested <body> elements?
> 
> Probably not, but it's still an improvement, and solves a real issue in
> wikipedia, so I think it's good to land.
Can you please explain what do you mean by nested <body> elements?
I will try putting a fix for that as well

[kojii, 2016-03-02 13:04:27]

On 2016/03/02 at 09:56:03, chakshu.a wrote:
> On 2016/03/01 02:21:57, kojii wrote:
> > On 2016/02/29 at 23:36:38, tkent wrote:
> > > Please do not add a test to LayoutTests/imported manually.
> > 
> > Right, my bad to miss that.
> > 
> > chakshu.a@, can you move the test files to somewhere else? Probably
> > LayoutTests/fast/dom/Attr/ (where the test file for crrev.com/31083009 is)
is
> > good.
> > 
> Sorry about that. Have fixed it now and moved them to
LayoutTests/fast/dom/Attr/
> https://codereview.chromium.org/1760443002/

Thanks, reviewed.

> > > Does this work well if nested <body> elements?
> > 
> > Probably not, but it's still an improvement, and solves a real issue in
> > wikipedia, so I think it's good to land.
> Can you please explain what do you mean by nested <body> elements?
> I will try putting a fix for that as well

Since we just check tagName in this fix, I suppose this fix would fail for a
test case like:

<div dir=rtl><body dir>text</body></div>

What we should check is the <body> which HTML spec handles special, but I don't
know if we have an easy method to determine that. Try searching in the code
base, there should be somewhere doing it. Hopefully reviewers for DOM (such as
tkent) can suggest during the review.

Also, if you're interested in, testing the root element might be also fun. If a
test fails for <body> element, it's quite possible to fail for <html> element as
well. So I think, assuming the dynamic change to <html> fails in the same way as
to <body>, the real condition would be:

if (is-the-root-html-element || is-the-real-body-element) ...

Fun? ;)

[chakshu, 2016-03-03 14:50:09]

On 2016/03/02 13:04:27, kojii wrote:
> On 2016/03/02 at 09:56:03, chakshu.a wrote:
> > On 2016/03/01 02:21:57, kojii wrote:
> > > On 2016/02/29 at 23:36:38, tkent wrote:
> > > > Please do not add a test to LayoutTests/imported manually.
> > > 
> > > Right, my bad to miss that.
> > > 
> > > chakshu.a@, can you move the test files to somewhere else? Probably
> > > LayoutTests/fast/dom/Attr/ (where the test file for crrev.com/31083009 is)
> is
> > > good.
> > > 
> > Sorry about that. Have fixed it now and moved them to
> LayoutTests/fast/dom/Attr/
> > https://codereview.chromium.org/1760443002/
> 
> Thanks, reviewed.
> 
> > > > Does this work well if nested <body> elements?
> > > 
> > > Probably not, but it's still an improvement, and solves a real issue in
> > > wikipedia, so I think it's good to land.
> > Can you please explain what do you mean by nested <body> elements?
> > I will try putting a fix for that as well
> 
> Since we just check tagName in this fix, I suppose this fix would fail for a
> test case like:
> 
> <div dir=rtl><body dir>text</body></div>
> 
> What we should check is the <body> which HTML spec handles special, but I
don't
> know if we have an easy method to determine that. Try searching in the code
> base, there should be somewhere doing it. Hopefully reviewers for DOM (such as
> tkent) can suggest during the review.
> 
> Also, if you're interested in, testing the root element might be also fun. If
a
> test fails for <body> element, it's quite possible to fail for <html> element
as
> well. So I think, assuming the dynamic change to <html> fails in the same way
as
> to <body>, the real condition would be:
> 
> if (is-the-root-html-element || is-the-real-body-element) ...
> 
> Fun? ;)

Thanks for explaining. The particular test case mentioned is anyway handled
while
DOM formation. It gets rendered as

<body dir><div dir=rtl>text</div></body> 

Though it fails probably in case
<html dir=rtl><body dir>text</body></html>

Is that what you meant to be fixed by checking if it is the root element?

Yeah, I guess it will be fun playing around with that :)

[kojii, 2016-03-04 04:45:13]

> Thanks for explaining. The particular test case mentioned is anyway handled
while
> DOM formation. It gets rendered as
> <body dir><div dir=rtl>text</div></body> 

Ah, so the nested body is stripped by the parser, didn't know that. Interesting.
What about if you document.createElement("body"), set dir, and appendChild()
somewhere?

See sample: http://jsbin.com/yexosu/edit?html,output

> Though it fails probably in case
> <html dir=rtl><body dir>text</body></html>
> 
> Is that what you meant to be fixed by checking if it is the root element?
> 
> Yeah, I guess it will be fun playing around with that :)

Yeah, right, that way, you can avoid body being stripped.

Another thing I thought was, when crbug.com/84428 mentions "change body.dir
fails", it's likely that "change document.documentElement.dir" may fail as well.
I meant, when testing special behavior for body, it's also nice to test
document.documentElement as well.