Closed Bug 1134209 Opened 7 years ago Closed 2 years ago

Text deletion copies text in contentEditable elements

Categories

(Firefox for Android Graveyard :: Keyboards and IME, defect)

35 Branch
ARM
Android
defect
Not set
major

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: esawin, Unassigned)

References

Details

STR
1. Open http://html5demos.com/contenteditable
2. Tap into the editable element
   a. Set the cursor inside of a word (e.g. on|ly)
   b. Set the cursor at the end of a word (e.g. only|)
3. Tap the delete key

Expected: The characters preceding the cursor position are deleted.

Actual: In case of a. nothing happens. In case of b. the word, excluding the last character, is copied and pasted at the cursor position (e.g. only| -> onlyonl|).
I have set up a simplified contentEditable test page [1], which does not show the same issue.
The difference in handling between the page from the STR [2] and [1] is that we select different roots for the editable content.

In case of [1], nsINode::GetSelectionRootContent returns the correct flattened content text for the text change events by selecting the correct root at [3].
In case of [2], nsINode::GetSelectionRootContent returns the <body> element for as the root content at [4], which results in wrong text content being passed through, which causes issues down the road.

As I understand, this is not the correct behavior for nsINode::GetSelectionRootContent.

[1] https://people.mozilla.org/~esawin/ce
[2] http://html5demos.com/contenteditable
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#334
[4] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#329
Flags: needinfo?(roc)
Flags: needinfo?(roc) → needinfo?(masayuki)
It looks like the simplified test page [1] works differently, because [2] sets document.designMode='on' on focus of the editable section, which explains why GetSelectionRootContent (comment 1) returns the <body> in this case. 

Our handling of IME input is broken in such a case. Should we ignore designMode when focused on a contentEditable element or what is the better approach here?

[1] https://people.mozilla.org/~esawin/ce
[2] http://html5demos.com/contenteditable
(In reply to Eugen Sawin [:esawin] from comment #2)
> It looks like the simplified test page [1] works differently, because [2]
> sets document.designMode='on' on focus of the editable section, which
> explains why GetSelectionRootContent (comment 1) returns the <body> in this
> case. 

Right.

> Our handling of IME input is broken in such a case. Should we ignore
> designMode when focused on a contentEditable element or what is the better
> approach here?

I don't think so. document.designMode="on" means <html contenteditable> or <body contenteditable>.

http://www.w3.org/TR/html5/editing.html#making-entire-documents-editable:-the-designmode-idl-attribute
> When the designMode changes from being disabled to being enabled, the user agent must synchronously reset
> the document's active range's start and end boundary points to be at the start of the Document and then
> run the focusing steps for the root element of the Document, if any.

I guess that we notify IME of text and/or offsets in contenteditable element rather than entire document?
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #3)
> > When the designMode changes from being disabled to being enabled, the user agent must synchronously reset
> > the document's active range's start and end boundary points to be at the start of the Document and then
> > run the focusing steps for the root element of the Document, if any.
> 
> I guess that we notify IME of text and/or offsets in contenteditable element
> rather than entire document?
nsINode::GetSelectionRootContent returns the <body> element and we pass IME the whole <body> content for onTextChange. The offsets seem to be relative to the focused contentEditable element, however, which breaks things when we try to query for the surrounding text when deleting characters.
> I guess that we notify IME of text and/or offsets in contenteditable element
> rather than entire document?

We get the entire document in case of [2], only the ce element in case of [1].

Here are some additional details about what happens on focus: in case of document [2], GetAnchorNode/GetAnchorOffset and GetFocusNode/GetFocusOffset return the same node/offset in [3], so we don't get the proper text on selection (set in [4]). With mIMEComposingText = "" and mIMEComposingStart not set to the selection start, we don't pass the check in [5] when composing/deleting text.

So the handling of document [1] and [2] start to differ on selection (besides the different selection roots as described in comment 1).

Is this something we can and should fix on the level of nsSelection?

[1] https://people.mozilla.org/~esawin/ce
[2] http://html5demos.com/contenteditable
[3] https://dxr.mozilla.org/mozilla-central/source/dom/events/ContentEventHandler.cpp#793
[4] https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1974
[5] https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1799
Flags: needinfo?(masayuki)
I wonder, the testcase (html5demos') doesn't work fine on any browsers. The test sets designMode when the contenteditable element gets focus. However, the focus event handler of the editor sets selection limit to the contenteditable element.
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#4596

I feel this is wrong, entire document should be editable (be able to move caret anywhere).

Anyway, that should be out of scope of this bug.

I think that nsINode::GetSelectionRoot() should return editor's active editing host (i.e., current selection limit of PresShell) which the editor *believes* as focused element. Then, offset confusion could be gone.

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#4596
Flags: needinfo?(masayuki)
Now, we unset focus from focused element when designMode is enabled (see bug 1128787, nobody should be focusable in designMode on Gecko). So, the testcase which enables designMode when the editor gets focus *and* disables designMode when the editor loses focus won't be editable.

I think that this should be marked as INVA.
I don't think this bug is invalid as this is something that could and will happen in the wild and we should handle it without breaking the experience.
A recently updated MDN page on designMode mentioned (before the update) that it used to be a workaround for IE to set the document to designMode=true to enable editing in contentEditable elements.

With bug 1128787, what happens is that on focus we still get the VKB and can type (although it's not inserted or displayed in the content) and will eventually crash when hitting delete.
(In reply to Eugen Sawin [:esawin] from comment #8)
> A recently updated MDN page on designMode mentioned (before the update) that
> it used to be a workaround for IE to set the document to designMode=true to
> enable editing in contentEditable elements.

I don't find such diff, could you tell me the URL?

And I think that if it's a hack for IE's bug, it should be performed only on IE, not other browsers.

I also checked current HTML5 spec, it says:

http://www.w3.org/TR/html5/editing.html#designMode
> When the designMode changes from being disabled to being enabled, the user agent must
> synchronously reset the document's active range's start and end boundary points to be at
> the start of the Document and then run the focusing steps for the root element of the Document,
> if any.

So, stealing focus from its descendant element is right behavior. 

> With bug 1128787, what happens is that on focus we still get the VKB and can
> type (although it's not inserted or displayed in the content) and will
> eventually crash when hitting delete.

Looks like that it's a new bug. Perhaps, Android's widget's. The VKB should be closed because the contenteditable element will lose focus immediately after it gets focus since designMode is enabled at focus event handler of the contenteditable element and it causes stealing focus from the contenteditable element.
Flags: needinfo?(nchen)
Hm we shouldn't depend on the VKB closing to avoid bugs. We should find out what the underlying issue is.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #10)
> Hm we shouldn't depend on the VKB closing to avoid bugs. We should find out
> what the underlying issue is.

On Windows, IME state is disabled as expected. So, I guess that Android widget doesn't close VKB even with disabled state...

The backup of the http://html5demos.com/contenteditable: https://bugzilla.mozilla.org/attachment.cgi?id=9146280

Deletion happens as expected on the latest Firefox Preview.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.