Stop inserting placeholders for TextArea and Input elements.

Stop inserting placeholders for TextArea and Input elements.

Input and TextArea elements do not require to insert placeholders
in their shadow trees. It was done before due to lagacy constrains
that are no longer the case.

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

[aurimas (slooooooooow), 2013-04-04 17:03:47]

Hello Ojan,

This is my try to fix the issue with <br> being inserted when the textarea or
input elements become empty. Please take a look.

Aurimas

[eseidel, 2013-04-04 17:06:36]

Can we test this?

[aurimas (slooooooooow), 2013-04-04 17:10:10]

On 2013/04/04 17:06:36, Eric Seidel (Google) wrote:
> Can we test this?

Sorry, I forgot to add to the description that this also reverts
https://bugs.webkit.org/show_bug.cgi?id=112275 fix which according to Ojan was
incorrect (offline email thread). The tests added in 112275 test the <br>
insertion for input elements. And I need to add an additional test for textarea
elements.

Thanks for the catch.

[ojan, 2013-04-08 19:23:36]

This patch needs tests, but it's roughly what I had in mind when I was
suggesting that we should stop inserting BRs.

For examples of tests, you can look at some of the existing tests in
LayoutTests/editing, e.g. LayoutTests/editing/deleting/delete-ws-fixup-002.html.

https://codereview.chromium.org/13532011/diff/1/Source/WebCore/editing/Delete...
File Source/WebCore/editing/DeleteSelectionCommand.cpp (right):

https://codereview.chromium.org/13532011/diff/1/Source/WebCore/editing/Delete...
Source/WebCore/editing/DeleteSelectionCommand.cpp:808: &&
!isHTMLTextAreaElement(textControl) && !textControl->toInputElement();
If you're willing to try it out, I had a more general solution in mind that
would work for both text controls and more general use of contentEditable. 

Specifically, if you change the last line to:
&& !m_endingPosition.containerNode().isRootEditableElement();

That way, if you have <div contentEditable>a</div> and you delete the 'a', we
won't insert a <br> there either.

[aurimas (slooooooooow), 2013-04-09 21:54:21]

https://chromiumcodereview.appspot.com/13532011/diff/1/Source/WebCore/editing...
File Source/WebCore/editing/DeleteSelectionCommand.cpp (right):

https://chromiumcodereview.appspot.com/13532011/diff/1/Source/WebCore/editing...
Source/WebCore/editing/DeleteSelectionCommand.cpp:808: &&
!isHTMLTextAreaElement(textControl) && !textControl->toInputElement();
On 2013/04/08 19:23:36, ojan wrote:
> If you're willing to try it out, I had a more general solution in mind that
> would work for both text controls and more general use of contentEditable. 
> 
> Specifically, if you change the last line to:
> && !m_endingPosition.containerNode().isRootEditableElement();
> 
> That way, if you have <div contentEditable>a</div> and you delete the 'a', we
> won't insert a <br> there either.

I tried adding !m_endingPosition.containerNode()->isRootEditableElement();
however it crashes when it tries to delete the last element in textarea. The
crash happens in WebKit/Source/WebCore/dom/Node.h:467

I also tried
enclosingTextFormControl(m_selectionToDelete.start())->isRootEditableElement();.
This did not crash in TextArea, but it crashed in ContentEditable when deleting
a node inside the ContentEditable. It crashed on the same line as the one above.

Any ideas how to get around it?

[ojan, 2013-04-09 23:44:29]

I'm not really sure what's going on with the crashes. Levi actually knows this
code much better than I do. Levi, do you have a suggestion on what the generic
solution here would be? I was telling Aurimas that we should get rid of putting
<br> in empty contentEditables entirely since we no longer need that for them
not to collapse. We will still need it for blocks inside the contentEditable,
but that's a different issue.

[aurimas (slooooooooow), 2013-04-09 23:54:13]

1. Is there a way to differentiate between contentEditables and blocks inside
content editable?

2. What does enclosingTextFromControl(m_selectionToDelete.start()) represent?

3. What is m_endingPosition.containerNode()? How is it different from (2)

4. What is a safe way to check if a node isRootEditableElement()?


Here is the crash stack i'm getting:
#0  0x0000000001eae871 in WebCore::Node::document (this=0x0) at
../../third_party/WebKit/Source/WebCore/dom/Node.h:467
#1  0x0000000002ef3546 in WebCore::Node::rendererIsEditable (this=0x0,
editableLevel=WebCore::Node::Editable, 
    treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
../../third_party/WebKit/Source/WebCore/dom/Node.cpp:681
#2  0x00000000022904c0 in WebCore::Node::rendererIsEditable (this=0x0,
editableType=WebCore::ContentIsEditable, 
    treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
../../third_party/WebKit/Source/WebCore/dom/Node.h:426
#3  0x0000000002ef53b8 in WebCore::Node::isRootEditableElement (this=0x0) at
../../third_party/WebKit/Source/WebCore/dom/Node.cpp:1300

[leviw_travelin_and_unemployed, 2013-04-09 23:54:29]

https://chromiumcodereview.appspot.com/13532011/diff/1/Source/WebCore/editing...
File Source/WebCore/editing/DeleteSelectionCommand.cpp (right):

https://chromiumcodereview.appspot.com/13532011/diff/1/Source/WebCore/editing...
Source/WebCore/editing/DeleteSelectionCommand.cpp:808: &&
!isHTMLTextAreaElement(textControl) && !textControl->toInputElement();
Ojan, I think your change is correct, but simply violates one of many
assumptions elsewhere in editing code. More handy than the line this asserts on
would be a backtrace.

[leviw_travelin_and_unemployed, 2013-04-09 23:56:47]

On 2013/04/09 23:54:13, aurimas wrote:
> 1. Is there a way to differentiate between contentEditables and blocks inside
> content editable?
> 
> 2. What does enclosingTextFromControl(m_selectionToDelete.start()) represent?
> 
> 3. What is m_endingPosition.containerNode()? How is it different from (2)
> 
> 4. What is a safe way to check if a node isRootEditableElement()?
> 
> 
> Here is the crash stack i'm getting:
> #0  0x0000000001eae871 in WebCore::Node::document (this=0x0) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.h:467
> #1  0x0000000002ef3546 in WebCore::Node::rendererIsEditable (this=0x0,
> editableLevel=WebCore::Node::Editable, 
>     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:681
> #2  0x00000000022904c0 in WebCore::Node::rendererIsEditable (this=0x0,
> editableType=WebCore::ContentIsEditable, 
>     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.h:426
> #3  0x0000000002ef53b8 in WebCore::Node::isRootEditableElement (this=0x0) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:1300

Ahhh... without a placeholder to have a position inside of, we end up with a
position that isn't considered editable...

[leviw_travelin_and_unemployed, 2013-04-10 00:04:51]

On 2013/04/09 23:54:13, aurimas wrote:
> 1. Is there a way to differentiate between contentEditables and blocks inside
> content editable?
I'm not sure what the question here is... there's always a root editable element
that has contentEditable set. rootEditableElement or editableRootForPosition
will give you such a thing.

> 2. What does enclosingTextFromControl(m_selectionToDelete.start()) represent?
What it sounds like: the text form control that contains the selection (if there
is one).

> 3. What is m_endingPosition.containerNode()? How is it different from (2)
A position can represent an offset into a node (a container node or text node)
or the position directly before or after a node. endingPosition is a single
position that's where the selection will be after it's deleted.
selectionToDelete has a start and end position.

> 4. What is a safe way to check if a node isRootEditableElement()?

[aurimas (slooooooooow), 2013-04-10 00:07:58]

On 2013/04/10 00:04:51, Levi wrote:
> On 2013/04/09 23:54:13, aurimas wrote:
> > 1. Is there a way to differentiate between contentEditables and blocks
inside
> > content editable?
> I'm not sure what the question here is... there's always a root editable
element
> that has contentEditable set. rootEditableElement or editableRootForPosition
> will give you such a thing.
> 
> > 2. What does enclosingTextFromControl(m_selectionToDelete.start())
represent?
> What it sounds like: the text form control that contains the selection (if
there
> is one).
> 
> > 3. What is m_endingPosition.containerNode()? How is it different from (2)
> A position can represent an offset into a node (a container node or text node)
> or the position directly before or after a node. endingPosition is a single
> position that's where the selection will be after it's deleted.
> selectionToDelete has a start and end position.
> 
> > 4. What is a safe way to check if a node isRootEditableElement()?

I'm sorry I don't really know WebKit that much :/ What is the best way for me to
fix my bug?

[Xianzhu, 2013-04-10 20:16:37]

On 2013/04/09 23:54:13, aurimas wrote:
> 1. Is there a way to differentiate between contentEditables and blocks inside
> content editable?
> 
> 2. What does enclosingTextFromControl(m_selectionToDelete.start()) represent?
> 
> 3. What is m_endingPosition.containerNode()? How is it different from (2)
> 
> 4. What is a safe way to check if a node isRootEditableElement()?
> 
> 
> Here is the crash stack i'm getting:
> #0  0x0000000001eae871 in WebCore::Node::document (this=0x0) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.h:467
> #1  0x0000000002ef3546 in WebCore::Node::rendererIsEditable (this=0x0,
> editableLevel=WebCore::Node::Editable, 
>     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:681
> #2  0x00000000022904c0 in WebCore::Node::rendererIsEditable (this=0x0,
> editableType=WebCore::ContentIsEditable, 
>     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.h:426
> #3  0x0000000002ef53b8 in WebCore::Node::isRootEditableElement (this=0x0) at
> ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:1300

This this the stacktrace for
"m_endingPosition.containerNode()->isRootEditableElement()" or 
"enclosingTextFormControl(m_selectionToDelete.start())->isRootEditableElement()"?

[aurimas (slooooooooow), 2013-04-10 20:19:32]

On 2013/04/10 20:16:37, Xianzhu wrote:
> On 2013/04/09 23:54:13, aurimas wrote:
> > 1. Is there a way to differentiate between contentEditables and blocks
inside
> > content editable?
> > 
> > 2. What does enclosingTextFromControl(m_selectionToDelete.start())
represent?
> > 
> > 3. What is m_endingPosition.containerNode()? How is it different from (2)
> > 
> > 4. What is a safe way to check if a node isRootEditableElement()?
> > 
> > 
> > Here is the crash stack i'm getting:
> > #0  0x0000000001eae871 in WebCore::Node::document (this=0x0) at
> > ../../third_party/WebKit/Source/WebCore/dom/Node.h:467
> > #1  0x0000000002ef3546 in WebCore::Node::rendererIsEditable (this=0x0,
> > editableLevel=WebCore::Node::Editable, 
> >     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> > ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:681
> > #2  0x00000000022904c0 in WebCore::Node::rendererIsEditable (this=0x0,
> > editableType=WebCore::ContentIsEditable, 
> >     treatment=WebCore::Node::UserSelectAllIsAlwaysNonEditable) at
> > ../../third_party/WebKit/Source/WebCore/dom/Node.h:426
> > #3  0x0000000002ef53b8 in WebCore::Node::isRootEditableElement (this=0x0) at
> > ../../third_party/WebKit/Source/WebCore/dom/Node.cpp:1300
> 
> This this the stacktrace for
> "m_endingPosition.containerNode()->isRootEditableElement()" or 
>
"enclosingTextFormControl(m_selectionToDelete.start())->isRootEditableElement()"?

This is for !m_endingPosition.containerNode().isRootEditableElement()

[Xianzhu, 2013-04-10 20:26:19]

On 2013/04/09 23:54:13, aurimas wrote:
> 4. What is a safe way to check if a node isRootEditableElement()?

The method itself should be safe. Your stacktrace shows that you called the
method against a NULL node.
enclosingTextFormControl(m_selectionToDelete.start()) will be NULL if the
selection is not in a text field but in a contentEditable.

[Xianzhu, 2013-04-10 20:52:32]

On 2013/04/10 20:19:32, aurimas wrote:
> On 2013/04/10 20:16:37, Xianzhu wrote:
> > This this the stacktrace for
> > "m_endingPosition.containerNode()->isRootEditableElement()" or 
> >
>
"enclosingTextFormControl(m_selectionToDelete.start())->isRootEditableElement()"?
> 
> This is for !m_endingPosition.containerNode().isRootEditableElement()

The crash seems because the m_endingPosition doesn't have a containerNode at the
time. Maybe this is the case when the last content of the editor is deleted? If
yes, I think you can try

(m_endingPosition.containerNode() &&
!m_endingPosition.containerNode()->isRootEditableElement())
or
(!m_endingPosition.containerNode() || ...)

depending on if we need the place holder when m_endingPosition doesn't have a
containerNode.

Levi and Ojan, if we use m_selectionToDelete.start().containerNode() instead in
the check, what's the difference?

[aurimas (slooooooooow), 2013-04-16 20:52:45]

leviw: do you have a solution in mind that might help me fix this bug?

[aurimas (slooooooooow), 2013-04-22 18:25:49]

Hey Levi and Ojan,

Since I do not know how to do it properly for all ContentEditable, I am keeping
the original fix. I also added a test.

Please take a look.
Aurimas

[leviw_travelin_and_unemployed, 2013-04-22 23:31:22]

Okay. lgtm after nits.

https://chromiumcodereview.appspot.com/13532011/diff/34001/LayoutTests/editin...
File LayoutTests/editing/deleting/delete-last-character.html (right):

https://chromiumcodereview.appspot.com/13532011/diff/34001/LayoutTests/editin...
LayoutTests/editing/deleting/delete-last-character.html:1: <html>
Add a doctype

[aurimas (slooooooooow), 2013-04-23 01:00:53]

Fixed the nit.

https://chromiumcodereview.appspot.com/13532011/diff/34001/LayoutTests/editin...
File LayoutTests/editing/deleting/delete-last-character.html (right):

https://chromiumcodereview.appspot.com/13532011/diff/34001/LayoutTests/editin...
LayoutTests/editing/deleting/delete-last-character.html:1: <html>
On 2013/04/22 23:31:22, Levi wrote:
> Add a doctype

Done.

[commit-bot: I haz the power, 2013-04-23 01:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-23 01:07:15]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[aurimas (slooooooooow), 2013-04-23 01:10:07]

Is it not enough to just check the CQ checkbox to add a patch to commit queue?
What else do I need to do?

Aurimas

[commit-bot: I haz the power, 2013-04-23 01:30:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-23 01:36:53]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[commit-bot: I haz the power, 2013-04-23 17:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-23 17:09:31]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[commit-bot: I haz the power, 2013-04-23 20:46:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-23 20:52:30]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[commit-bot: I haz the power, 2013-04-24 06:01:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-24 06:07:24]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[commit-bot: I haz the power, 2013-04-24 21:24:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/13532011/39001

[commit-bot: I haz the power, 2013-04-24 21:30:45]

Presubmit check for 13532011-39001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 8 file(s).

[Dirk Pranke, 2013-04-25 00:33:47]

Committed patchset #7 manually as r149062 (presubmit successful).