Bug 42767 – [Windows] Home hey doesn't work in first DIV inside a TABLE

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julie Parent]

Created attachment 62202 [details]
Test case

Repro steps:
1. Create a page with the following HTML (or use attached test case):
<TABLE>
  <TR>
    <TD>
      <DIV contenteditable="true">
        <DIV>Line 1 - home does not work</DIV>
        <DIV>Line 2 - home does work</DIV>
      </DIV>
    </TD>
  </TR>
</TABLE>
2. Click into the contenteditable div and place the cursor inside Line 1.
3. Press Home

Expected result:
The cursor moves to the beginning of the line.

Result:
The cursor doesn't move.

Other notes:
* Happens in both Chromium on Windows and Safari on Windows.
* Works as expected on Line 2.
* Effects Google Sites (first line of any Sites page exhibits this bug).

(Related Chromium bug - crbug.com/49780)

[Julie Parent]

Note that this is Windows only - Mac behavior is correct to not move the cursor when home key is used.

[Ryosuke Niwa]

I guess Home/End doesn't move the caret in the line so this bug doesn't reproduce on Mac.

[Ryosuke Niwa]

Created attachment 62211 [details]
demonstrates the bug on mac using modify(move, backward, line-boundary)

[Ryosuke Niwa]

The problem lies in http://trac.webkit.org/browser/trunk/WebCore/editing/visible_units.cpp#L376 which is called by logicalStartPositionForLine < logicalStartOfLine < SelectionController::modifyMovingBackward.  Here, we are avoiding the first position in the table even if the previous position wasn't editable.  One line fix to add isEditablePosition(previous.deepEquivalent()) will fix the issue.

[Ryosuke Niwa]

Created attachment 62262 [details]
fixes the bug

[Ojan Vafai]

Comment on attachment 62262 [details]
fixes the bug

It would be nice if, in the cases where there are multiple dump calls, if each one could be prefixed. Also, it would be good if dump took a second optional argument for a description string. For example, I'd like the test expectations for this test to be something like the following:

Tests whether home moves the caret to the beginning of line inside a content-editable in an uneditable table.

Dump of markup 1:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text>The caret is initially <selection-caret>here but should move to the beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>

Dump of markup 2:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text><selection-caret>The caret is initially here but should move to the beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>

Where "Dump of markup *" is the default value for the optional description string. If Markup.dump is only called once, without a description string, then it should not print a description before dumping. Finally, if you make this change, then rename Markup._description to Markup._test_description to avoid confusion.

> +++ LayoutTests/resources/dump-as-markup.js	(working copy)

Your indentation is off in many places in this file.

> +	var markup = "";
> +
> +	if (Markup._description && !Markup._container)
> +		markup = Markup._description + '\n';

This should be += (imagine if a line got inserted above this one modifying markup). 

> +        Markup._container.style.cssText = 'width:100%';

I know you're just modifying existing code, but this should probably be "Markup._container.style.width = '100%';". No reason to use cssText if we're not actually setting multiple properties.

[Ryosuke Niwa]

Created attachment 62320 [details]
fixed per Ojan's comment

[Ojan Vafai]

Comment on attachment 62320 [details]
fixed per Ojan's comment

Just a couple more small things.

> +++ LayoutTests/resources/dump-as-markup.js	(working copy)
> +    if (typeof opt_node == 'string') {
> +        // Allow to specify the node by id and to pass the description in the first argument
> +        if (document.getElementById(opt_node))
> +            opt_node = document.getElementById(opt_node);
> +        else {
> +            opt_node = null;
> +            opt_description = opt_node;
> +        }

Why do we have this else clause? If opt_node is a string and the node is not in the document, we should just error.

> +    if (Markup._test_description && !Markup._container)
> +        markup += Markup._test_description + '\n';
> +
> +    Markup._dumpCalls++;
> +
> +    // If dump is called not by notifyDone or dump has already been called then
> +    // print out optional description because this is a test with multiple calls of dump.
> +    if (!Markup._done || opt_description) {
> +        if (!opt_description)
> +            opt_description = "Dump of markup " + Markup._dumpCalls
> +        if (Markup._container)

Checking Markup._container here and above is a bit confusing to me. How about checking that Markup._dumpCalls > 1? That should have the same effect, right?

> +            markup += '\n';
> +        markup += '\n' + opt_description;

Nit: Can you add add a colon after the description? I think it makes the output a bit more readable.

> -    Markup.dump();
> +    Markup._done = true;
> +
> +    // If dump has already been called, don't bother to dump again
I like this!

> +    if (!Markup._dumpCalls)
> +        Markup.dump();

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 62320 [details])
> Just a couple more small things.
> 
> > +++ LayoutTests/resources/dump-as-markup.js	(working copy)
> > +    if (typeof opt_node == 'string') {
> > +        // Allow to specify the node by id and to pass the description in the first argument
> > +        if (document.getElementById(opt_node))
> > +            opt_node = document.getElementById(opt_node);
> > +        else {
> > +            opt_node = null;
> > +            opt_description = opt_node;
> > +        }
> 
> Why do we have this else clause? If opt_node is a string and the node is not in the document, we should just error.

What if I've already called setNodeToDump and just want to pass-in the description?  Then I have to specify the node again in order to dump a subtree.  So I added that else clause to allow the description being passed through opt_node.

> > +    if (Markup._test_description && !Markup._container)
> > +        markup += Markup._test_description + '\n';
> > +
> > +    Markup._dumpCalls++;
> > +
> > +    // If dump is called not by notifyDone or dump has already been called then
> > +    // print out optional description because this is a test with multiple calls of dump.
> > +    if (!Markup._done || opt_description) {
> > +        if (!opt_description)
> > +            opt_description = "Dump of markup " + Markup._dumpCalls
> > +        if (Markup._container)
> 
> Checking Markup._container here and above is a bit confusing to me. How about checking that Markup._dumpCalls > 1? That should have the same effect, right?

Will do.

> > +            markup += '\n';
> > +        markup += '\n' + opt_description;
> 
> Nit: Can you add add a colon after the description? I think it makes the output a bit more readable.

Will do.

[Ryosuke Niwa]

Created attachment 62339 [details]
fixed per ojan's comments

[Ojan Vafai]

> What if I've already called setNodeToDump and just want to pass-in the description?  Then I have to specify the node again in order to dump a subtree.  So I added that else clause to allow the description being passed through opt_node.

Yikes. Having the arguments swap places like that is too confusing to be worth it. You can just do Markup.dump(null, "foo").

[Ryosuke Niwa]

Created attachment 62345 [details]
fixed per ojan's comments

(In reply to comment #11)
> > What if I've already called setNodeToDump and just want to pass-in the description?  Then I have to specify the node again in order to dump a subtree.  So I added that else clause to allow the description being passed through opt_node.
> 
> Yikes. Having the arguments swap places like that is too confusing to be worth it. You can just do Markup.dump(null, "foo").

Ok, removed.

[Ojan Vafai]

Comment on attachment 62345 [details]
fixed per ojan's comments

You lost your ChangeLog modifications. Please put those back in and feel free to commit.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/63918.