1315065 - Outlook On the Web (OWA) Backspace not working when indenting

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P4)

Description

[hector7869]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce:

Running firefox 4.9 or above. No Addons, plugins, etc. When editing the body of an email in outlook on the web (OWA) if you increase the indent and type some characters you are no longer able to use the backspace key to delete these characters. The cursor just sits at the last character. Works fine if you don't indent. Not reproducible in other browsers. 

Running Exchange 2016 CU3. 


Actual results:

If you increase the indent and type some characters you are no longer able to use the backspace key to delete these characters.


Expected results:

Backspace key should delete characters

Comment 1

[Loic]

Masayuki, are you able to reproduce the issue with OWA? If you don't have time, could you NI? someone else, please.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I cannot reproduce this bug only with the STR, however, I reproduced it after pressing "End" key. It seems that this is a bug of our editor.

STR:

1. Go to https://www.microsoft.com/en-US/outlook-com/ and sign in.
2. Click "New" button.
3. Set focus to the message body.
4. Type "abc".
5. Indent one time from the toolbar below the message body.
6. Press "End" key.
7. Press "Backspace".

Current my queue is already full, but I'll try to look this more later... (Feel free to steal this bug from me!)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://jsfiddle.net/d_toybox/868yw3bb/
Perhaps, when I type "End" key in <blockquote>'s <p>in OWA, selection is collapsed at the end of <blockquote>.

Comment 4

[Mike Taylor [:miketaylr]]

> I reproduced it after pressing "End" key. It seems that this is a bug of our editor.

Any reason this got moved into Tech Evangelism, Jenn?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> https://jsfiddle.net/d_toybox/868yw3bb/
> Perhaps, when I type "End" key in <blockquote>'s <p>in OWA, selection is
> collapsed at the end of <blockquote>.

Hmm, I was wrong. I found different bug at investigating the cause :-p

Correct testcase for this bug is: https://jsfiddle.net/d_toybox/868yw3bb/5/

When there are two or more empty text nodes at the end of a line, "End" key moves selection, to the rightmost empty text node.  Then, Backspace key does nothing in current our editor code because it doesn't assume that there can be empty text node.

I'll post patches soon.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d01839e02f

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Filed bug 1316302 for the bug of found in comment 3.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node

Currently, when selection is collapsed at an empty text node, the behavior of each major browser is different.

When you remove the last character of non-empty text node followed by empty text nodes, Chromium removes all following empty text nodes.  However, Edge never removes empty text nodes even when selection is collapsed at an empty text node.

With this patch, our behavior becomes same as Edge.  I think that we should take this for keeping backward compatibility since Gecko never removes empty text nodes.  So, in other words, this patch makes Backspace key press at an empty text node modify the preceding non-empty text node.

When you remove the first character of non-empty text node preceded with empty text nodes, Edge removes all preceding empty text nodes.  However, Chromium and Gecko keeps previous empty text nodes than caret position.  So, we should keep current behavior for backward compatibility.  In other words, this patch makes Delete key press at an empty text node modify the following non-empty text node and keep current behavior.


The fixing approach of this is, making WSRunObject::PriorVisibleNode() and WSRunObject::NextVisibleNode() ignore empty text node.  This should make sense because empty text node is not a visible node. (On the other hand, when the DOMPoint has a null character, it should treat as visible character.  That is visible with Unicode codepoint.)

Review commit: https://reviewboard.mozilla.org/r/91702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91702/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8809236 [details]
Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node

https://reviewboard.mozilla.org/r/91702/#review92112

::: editor/libeditor/HTMLEditRules.cpp:1959
(Diff revision 1)
>        *aHandled = true;
>        rv = mHTMLEditor->DeleteText(nodeAsText, std::min(so, eo),
>                                     DeprecatedAbs(eo - so));
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      // XXX When Backspace key is pressed, Chromium removes following empty

This is a bit scary to be compatible with Edge in one case but compatible with Chrome in another case.

::: editor/libeditor/WSRunObject.cpp:530
(Diff revision 1)
>    for (; run; run = run->mRight) {
>      if (run->mType == WSType::normalWS) {
>        WSPoint point = GetCharAfter(aNode, aOffset);
> -      if (point.mTextNode) {
> +      // When it's a non-empty text node, return it.
> +      if (point.mTextNode && point.mTextNode->Length()) {
> +        if (!point.mChar) {

This could use a comment. How can mChar be 0?

::: editor/libeditor/tests/test_bug1315065.html:37
(Diff revision 1)
> +    var emptyTextNode2 = document.createTextNode("");
> +    range.insertNode(emptyTextNode2);
> +    var emptyTextNode1 = document.createTextNode("");
> +    range.insertNode(emptyTextNode1);
> +    is(p.childNodes.length, 5, "Failed to initialize the editor");
> +    is(p.childNodes.item(1), emptyTextNode1, "1st text node should be empytTextNode1");

s/empytTextNode1/emptyTextNode1/

Same also elsewhere

::: editor/libeditor/tests/test_bug1315065.html:67
(Diff revision 1)
> +    initForBackspace(i);
> +    synthesizeKey("KEY_Backspace", { code: "Backspace" });
> +    var p = document.getElementById("p");
> +    ok(p, kDescription + ": <p> element shouldn't be removed by Backspace key press");
> +    is(p.tagName.toLowerCase(), "p", kDescription + ": <p> element shouldn't be removed by Backspace key press");
> +    // When Backspace key is pressed even in empty text nodes, it should remove empty text nodes.

So, why you say "it should remove empty text nodes."
... and then later still "Edge doesn't... we should keep ...behavior".

What should actually happen?
Could you perhaps clarify the comment a bit

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8809236 [details]
Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node

https://reviewboard.mozilla.org/r/91702/#review92192

::: editor/libeditor/HTMLEditRules.cpp:1959
(Diff revision 1)
>        *aHandled = true;
>        rv = mHTMLEditor->DeleteText(nodeAsText, std::min(so, eo),
>                                     DeprecatedAbs(eo - so));
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      // XXX When Backspace key is pressed, Chromium removes following empty

Yeah, but changing behavior *might* break web apps which include some hacks for us... So, anyway, it's risky.

::: editor/libeditor/WSRunObject.cpp:530
(Diff revision 1)
>    for (; run; run = run->mRight) {
>      if (run->mType == WSType::normalWS) {
>        WSPoint point = GetCharAfter(aNode, aOffset);
> -      if (point.mTextNode) {
> +      // When it's a non-empty text node, return it.
> +      if (point.mTextNode && point.mTextNode->Length()) {
> +        if (!point.mChar) {

Although, for example, <p>#&x0000;</p>
https://jsfiddle.net/d_toybox/dt41nz6w/

::: editor/libeditor/tests/test_bug1315065.html:37
(Diff revision 1)
> +    var emptyTextNode2 = document.createTextNode("");
> +    range.insertNode(emptyTextNode2);
> +    var emptyTextNode1 = document.createTextNode("");
> +    range.insertNode(emptyTextNode1);
> +    is(p.childNodes.length, 5, "Failed to initialize the editor");
> +    is(p.childNodes.item(1), emptyTextNode1, "1st text node should be empytTextNode1");

Oh, thanks!

::: editor/libeditor/tests/test_bug1315065.html:67
(Diff revision 1)
> +    initForBackspace(i);
> +    synthesizeKey("KEY_Backspace", { code: "Backspace" });
> +    var p = document.getElementById("p");
> +    ok(p, kDescription + ": <p> element shouldn't be removed by Backspace key press");
> +    is(p.tagName.toLowerCase(), "p", kDescription + ": <p> element shouldn't be removed by Backspace key press");
> +    // When Backspace key is pressed even in empty text nodes, it should remove empty text nodes.

Okay, I changed the comment as:

> // When Backspace key is pressed even in empty text nodes, Gecko should not remove empty text nodes for now
> // because we should keep our traditional behavior (same as Edge) for backward compatibility as far as possible.
> // In this case, Chromium removes all empty text nodes, but Edge doesn't remove any empty text nodes.

I believe that *we* should keep backward compatibility of ourselves as far as possible. So, I meant that when we change behavior, one of following reason should be there:

* Current behavior causes inconvenience for users in major web service(s).
* Current behavior is really different from related spec.
* Current behavior is different from other browsers and they behaves similarly.
* Current behavior really doesn't make sense but one of other browsers behave nicer.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8809236 [details]
Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node

https://reviewboard.mozilla.org/r/91702/#review92204

::: editor/libeditor/WSRunObject.cpp:530
(Diff revision 1)
>    for (; run; run = run->mRight) {
>      if (run->mType == WSType::normalWS) {
>        WSPoint point = GetCharAfter(aNode, aOffset);
> -      if (point.mTextNode) {
> +      // When it's a non-empty text node, return it.
> +      if (point.mTextNode && point.mTextNode->Length()) {
> +        if (!point.mChar) {

Oops, this is my mistake. We shouldn't do continue in this case like PriorVisibleNode(). This remained old code which I was trying to write this patch.

So, I'll remove this check before landing. If you think this doesn't make sense, feel free to tell me.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8809236 [details]
Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91702/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e166b2418ca
When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node r=smaug

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e166b2418ca