If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
Editor
P4
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: hector7869, Assigned: masayuki, NeedInfo)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(platform-rel +, firefox52 fixed)

Details

(Whiteboard: [platform-rel-Microsoft][platform-rel-Outlook], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
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
(Reporter)

Updated

11 months ago
OS: Unspecified → All
Priority: -- → P4
Hardware: Unspecified → All

Comment 1

11 months ago
Masayuki, are you able to reproduce the issue with OWA? If you don't have time, could you NI? someone else, please.
Component: Untriaged → Editor
Flags: needinfo?(masayuki)
Product: Firefox → Core
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!)
Assignee: nobody → masayuki
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(masayuki)
platform-rel: --- → ?
Flags: needinfo?(davem)
Whiteboard: [platform-rel-Microsoft][platform-rel-Outlook]

Updated

11 months ago
platform-rel: ? → +
Component: Editor → Desktop
Product: Core → Tech Evangelism
Target Milestone: --- → Future
Version: 49 Branch → unspecified
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>.
> 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?
Flags: needinfo?(jchaulk)
(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.
Status: NEW → ASSIGNED
Component: Desktop → Editor
Product: Tech Evangelism → Core
Target Milestone: Future → ---
Version: unspecified → Trunk
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d01839e02f
Filed bug 1316302 for the bug of found in comment 3.
Comment hidden (mozreview-request)

Comment 9

10 months ago
mozreview-review
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
Attachment #8809236 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

10 months ago
mozreview-review
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.
(Assignee)

Comment 11

10 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 13

10 months ago
Pushed by masayuki@d-toybox.com:
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e166b2418ca
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

8 months ago
Flags: needinfo?(jchaulk)
You need to log in before you can comment on or make changes to this bug.