Make contenteditable="false" element as atomic node for selection motion

On IE and FireFox, they treats contenteditable="false" element as atomic element. When moving caret around it, cursor is placed at start/end of it.

This patch changes Blink(Chrome) behavior as IE and FireFox.

This patch updates tests for behavior change:
- editing/selection/4889598.html: Removed, because updated version of mixed-editability-3.html does same test, line motion around uneditable table element.
- editing/selection/mixed-editability-3.html: Changed to cover forward and backward line motion and verify by script rather than editing log and render tree for ease of future maintenance.
- editing/selection/move-by-line-004.html: Changed for new behavior. Caret can't go cross uneditable table cell as FireFox.

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

[yosin_UTC9, 2013-05-17 09:19:13]

Could you review this patch?
Thanks in advance.
-yosi
P.S.
Failure tests on Linux/Mac don't relate this patch. They may be flaky.

[yosin_UTC9, 2013-05-21 03:57:28]

ping...

[ojan, 2013-05-22 02:25:20]

Seems reasonable to me overall that contentEditable=false regions inside
contentEditable=true regions should be atomic.

I'm a bit uncertain about the actual code in question though. Maybe Levi or Eric
has a better sense.

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
File LayoutTests/editing/selection/mixed-editability-3.html (right):

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/mixed-editability-3.html:13:
testCaretMotion('both', 'line', 1, 'e1', 0, 'e2', 0);
These tests are kind of hard to read. Can you instead pass a single object to
testCaretMotion?

testCaretMotion({
    direction: 'both',
    granularity: 'line',
    repeatCount: 1,
    origin: 'e1',
    originOffset: 0,
    target: 'e2',
    targetOffset: 0,
});

And you could make repeatCount optional and default to 1.

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
File LayoutTests/editing/selection/resources/js-test-selection-shared.js
(right):

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/resources/js-test-selection-shared.js:7:
testCaretMotion('forward', granularity, repeatCount, origin, originOffset,
target, targetOffset);
Here and elsewhere, Blink style is 4-space indents.

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
renderer->isTableRow())
I don't really understand this check. What's special about tables and table
rows?

[yosin_UTC9, 2013-05-22 07:54:11]

Could you review this patch?
Thanks in advance.
-yosi

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
File LayoutTests/editing/selection/mixed-editability-3.html (right):

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/mixed-editability-3.html:13:
testCaretMotion('both', 'line', 1, 'e1', 0, 'e2', 0);
On 2013/05/22 02:25:20, ojan wrote:
> These tests are kind of hard to read. Can you instead pass a single object to
> testCaretMotion?
> 
> testCaretMotion({
>     direction: 'both',
>     granularity: 'line',
>     repeatCount: 1,
>     origin: 'e1',
>     originOffset: 0,
>     target: 'e2',
>     targetOffset: 0,
> });
> 
> And you could make repeatCount optional and default to 1.

Done.

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
File LayoutTests/editing/selection/resources/js-test-selection-shared.js
(right):

https://codereview.chromium.org/14794012/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/resources/js-test-selection-shared.js:7:
testCaretMotion('forward', granularity, repeatCount, origin, originOffset,
target, targetOffset);
On 2013/05/22 02:25:20, ojan wrote:
> Here and elsewhere, Blink style is 4-space indents.

Done.

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
renderer->isTableRow())
On 2013/05/22 02:25:20, ojan wrote:
> I don't really understand this check. What's special about tables and table
> rows?

To move position into table cell, we should not make table element and table row
atomic even if they are uneditable.

Checking table and table row suppose:
 <table contenteditable=flase>
   <tr contenteditable=false>
     <td contenteditable=true>
       We wanna reach here!
     </td>
   </tr>
</table.

[leviw_travelin_and_unemployed, 2013-05-22 21:02:47]

LGTM with the added nit.

https://codereview.chromium.org/14794012/diff/21001/Source/core/editing/htmle...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/14794012/diff/21001/Source/core/editing/htmle...
Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
renderer->isTableRow())
I'd add a comment describing why we handle tables differently so this is
clearer.

[ojan, 2013-05-22 21:05:45]

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
renderer->isTableRow())
On 2013/05/22 07:54:12, Yoshifumi Inoue wrote:
> On 2013/05/22 02:25:20, ojan wrote:
> > I don't really understand this check. What's special about tables and table
> > rows?
> 
> To move position into table cell, we should not make table element and table
row
> atomic even if they are uneditable.
> 
> Checking table and table row suppose:
>  <table contenteditable=flase>
>    <tr contenteditable=false>
>      <td contenteditable=true>
>        We wanna reach here!
>      </td>
>    </tr>
> </table. 

How is this different from the following?

<div contentEditable=false>
    foo
    <div contentEditable=true>we want to reach here</div>
    bar
</div>

[yosin_UTC9, 2013-05-23 09:58:44]

On 2013/05/22 21:05:45, ojan wrote:
>
https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
> File Source/core/editing/htmlediting.cpp (right):
> 
>
https://codereview.chromium.org/14794012/diff/12001/Source/core/editing/htmle...
> Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
> renderer->isTableRow())
> On 2013/05/22 07:54:12, Yoshifumi Inoue wrote:
> > On 2013/05/22 02:25:20, ojan wrote:
> > > I don't really understand this check. What's special about tables and
table
> > > rows?
> > 
> > To move position into table cell, we should not make table element and table
> row
> > atomic even if they are uneditable.
> > 
> > Checking table and table row suppose:
> >  <table contenteditable=flase>
> >    <tr contenteditable=false>
> >      <td contenteditable=true>
> >        We wanna reach here!
> >      </td>
> >    </tr>
> > </table. 
> 
> How is this different from the following?
> 
> <div contentEditable=false>
>     foo
>     <div contentEditable=true>we want to reach here</div>
>     bar
> </div>
Sorry, I can't answer your question. I don't know why IE treats TABLE different
from DIV, maybe their experience.

[yosin_UTC9, 2013-05-27 05:01:45]

Thanks for reviewing!
Committing...

https://codereview.chromium.org/14794012/diff/21001/Source/core/editing/htmle...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/14794012/diff/21001/Source/core/editing/htmle...
Source/core/editing/htmlediting.cpp:76: if (renderer->isTable() ||
renderer->isTableRow())
On 2013/05/22 21:02:47, Levi wrote:
> I'd add a comment describing why we handle tables differently so this is
> clearer.

Done.

[commit-bot: I haz the power, 2013-05-27 05:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yosin@chromium.org/14794012/35001

[ojan, 2013-05-27 06:51:54]

https://chromiumcodereview.appspot.com/14794012/diff/35001/Source/core/editin...
File Source/core/editing/htmlediting.cpp (right):

https://chromiumcodereview.appspot.com/14794012/diff/35001/Source/core/editin...
Source/core/editing/htmlediting.cpp:78: if (renderer->isTable() ||
renderer->isTableRow())
I don't think this bit is right. Even if we were to special-case tables, this
leaves out table sections. What does IE do if you make the tbody uneditable?

Still, I don't think we want to special-case tables unless we have some evidence
(other than layout tests) that web content depends on this. Browser
compatibility is important, but we should try to get browsers to converge on a
behavior that we think is good.

[commit-bot: I haz the power, 2013-05-27 07:10:00]

Change committed as 151179

[yosin_UTC9, 2013-05-27 09:10:13]

https://chromiumcodereview.appspot.com/14794012/diff/35001/Source/core/editin...
File Source/core/editing/htmlediting.cpp (right):

https://chromiumcodereview.appspot.com/14794012/diff/35001/Source/core/editin...
Source/core/editing/htmlediting.cpp:78: if (renderer->isTable() ||
renderer->isTableRow())
On 2013/05/27 06:51:54, ojan wrote:
> I don't think this bit is right. Even if we were to special-case tables, this
> leaves out table sections. What does IE do if you make the tbody uneditable?
> 
> Still, I don't think we want to special-case tables unless we have some
evidence
> (other than layout tests) that web content depends on this. Browser
> compatibility is important, but we should try to get browsers to converge on a
> behavior that we think is good.

I don't think there are web contents depend on this. Because, all browsers do
different.
Anyway, I'm reverting this patch: https://codereview.chromium.org/15732019/