1059163 - Clearing innerHTML of contenteditable with keyboard focus does not reset keyboard state

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

Does not happen on Flame 1.4, but does happen on Dolphin 1.4.

Have some HTML:

  <div id="text" contenteditable style="width: 50vw; font-size: 1.3rem; height: 40vw; border: solid 1px black"></div>
  <div contenteditable id="say">Click me</button>

Have some JS:

document.querySelector('#say').onclick = function(e) {
  e.preventDefault();

  document.querySelector('#text').innerHTML = '';
  document.querySelector('#text').focus();
}

Type in the contenteditable: "Fhqv" and press 'Click me'.

Expected:
- Keyboard suggest panel is empty

Actual:
- Keyboard suggestions are there
- Keyboard state thinks it's never cleared
- Keyboard goes into caps lock state and can't easily recover

Have to blur & focus to regain this.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Demo app that shows the behavior

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: This resolves the problem Gaia side

So after clearing the contenteditable, the state in the inputcontext is incorrect, as it keeps selectionstart & selectionend at the old values. This is a gecko bug.

Anyway, don't have time to rush a gecko bug out, so here's a quick workaround that at least solves the problem in ConnectA2 for Dolphin 1.4. We'll take it in our own build.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

:yxl, any clue what happens on gecko side that this doesn't happen on Flame?

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

I don't know why and cannot find a dolphin device to test this bug today. May find a device tomorrow.

I can reproduce this bug with v2.0 and v2.1 as well.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Mochitest

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

For master I found a bug in the selection events... See bug 1060579.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8479822 [details] [review]
This resolves the problem Gaia side

I don't have a dolphin either, and will ask around to borrow one.
Before that, could you please help explain why this workaround is needed?

If I understand correctly, the activate() method would be triggered after you refocus on the input field and this has nothing to do with selectionchange.
So, we should get correct selectionStart in activate(), no need to override its value.

If the selectionStart would be incorrect in activate, then this is a issue that we should fix at Gecko side.

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

BTW, I could not repro this issue with 2.0 and master (2.1), did I miss anything?

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #7)
> Comment on attachment 8479822 [details] [review]
> This resolves the problem Gaia side
> 
> I don't have a dolphin either, and will ask around to borrow one.
> Before that, could you please help explain why this workaround is needed?
> 
> If I understand correctly, the activate() method would be triggered after
> you refocus on the input field and this has nothing to do with
> selectionchange.
> So, we should get correct selectionStart in activate(), no need to override
> its value.
> 
> If the selectionStart would be incorrect in activate, then this is a issue
> that we should fix at Gecko side.

Yeah, but selectionStart is wrong in the activate call due to bug 1060579. We needed a quick fix for release without changing Gecko. I think in retrospect we shouldn't land this in upstream.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #9)
> (In reply to Rudy Lu [:rudyl] from comment #7)
> > Comment on attachment 8479822 [details] [review]
> > This resolves the problem Gaia side
> > 
> > I don't have a dolphin either, and will ask around to borrow one.
> > Before that, could you please help explain why this workaround is needed?
> > 
> > If I understand correctly, the activate() method would be triggered after
> > you refocus on the input field and this has nothing to do with
> > selectionchange.
> > So, we should get correct selectionStart in activate(), no need to override
> > its value.
> > 
> > If the selectionStart would be incorrect in activate, then this is a issue
> > that we should fix at Gecko side.
> 
> Yeah, but selectionStart is wrong in the activate call due to bug 1060579.
> We needed a quick fix for release without changing Gecko. I think in
> retrospect we shouldn't land this in upstream.
> 
> *** This bug has been marked as a duplicate of bug 1060579 ***
I believe it is not the same with bug 1060579.  We haven't landed selection listener related code on v1.4 and v2.0. So fixing selection listener should not fix this totally.

In forms.js, we have an editor observer to monitor the content changes:
https://github.com/mozilla/gecko-dev/blob/2fbbb04a62fbcbbaed5291510f45cb4f327c1800/dom/inputmethod/forms.js#L330

When clearing innerHTML, we should get a notification and update input state for the keyboard app.
But for some unknown reason, we fail to get correct selectionStart value when updating input state.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #10)
> (In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #9)
> > (In reply to Rudy Lu [:rudyl] from comment #7)
> > > Comment on attachment 8479822 [details] [review]
> > > This resolves the problem Gaia side
> > > 
> > > I don't have a dolphin either, and will ask around to borrow one.
> > > Before that, could you please help explain why this workaround is needed?
> > > 
> > > If I understand correctly, the activate() method would be triggered after
> > > you refocus on the input field and this has nothing to do with
> > > selectionchange.
> > > So, we should get correct selectionStart in activate(), no need to override
> > > its value.
> > > 
> > > If the selectionStart would be incorrect in activate, then this is a issue
> > > that we should fix at Gecko side.
> > 
> > Yeah, but selectionStart is wrong in the activate call due to bug 1060579.
> > We needed a quick fix for release without changing Gecko. I think in
> > retrospect we shouldn't land this in upstream.
> > 
> > *** This bug has been marked as a duplicate of bug 1060579 ***
> I believe it is not the same with bug 1060579.  We haven't landed selection
> listener related code on v1.4 and v2.0. So fixing selection listener should
> not fix this totally.
> 
> In forms.js, we have an editor observer to monitor the content changes:
> https://github.com/mozilla/gecko-dev/blob/
> 2fbbb04a62fbcbbaed5291510f45cb4f327c1800/dom/inputmethod/forms.js#L330
> 
> When clearing innerHTML, we should get a notification and update input state
> for the keyboard app.
> But for some unknown reason, we fail to get correct selectionStart value
> when updating input state.

Good point. I still think it is somewhat related, because how we get the selection is also updated through the private selection API, and that doesn't emit an event, thus not updating that state.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #11)
...
> 
> Good point. I still think it is somewhat related, because how we get the
> selection is also updated through the private selection API, and that
> doesn't emit an event, thus not updating that state.
That may be the root cause.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1059163.patch

Nice and clean

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Only downside I have is that the mutation observer also triggers on things like inserting newlines (because <br>), but we have dirty checking before we send it back so no perf impact I think.

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8493699 [details] [diff] [review]
bug1059163.patch

Review of attachment 8493699 [details] [diff] [review]:
-----------------------------------------------------------------

Why not use EditAction to get content mutation notification?
https://github.com/mozilla/gecko-dev/blob/27b927d770836cdd6f2579df7b40f7edc4f6990a/dom/inputmethod/forms.js#L332

When the text of a content editable is changed, EditAction() will be called. So we may not need _focusContentObserver to do the same job.

::: dom/inputmethod/mochitest/test_bug1059163.html
@@ +32,5 @@
> +
> +function runTest() {
> +  let im = navigator.mozInputMethod;
> +
> +  im.oninputcontextchange = function() {

This oninputcontextchange handler is useless. You get non null gContext within the handler, but never use it elsewhere.

@@ +55,5 @@
> +  document.body.appendChild(app);
> +  app.addEventListener('mozbrowserloadend', function() {
> +    let mm = SpecialPowers.getBrowserFrameMessageManager(app);
> +
> +    let timeout = setTimeout(function() {

Remove this setTimeout. Let the mochitest test framework to handle the test timeout issue.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Seems this is no longer a Dolphin/1.4 only feature?

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #15)
> Comment on attachment 8493699 [details] [diff] [review]
> bug1059163.patch
> 
> Review of attachment 8493699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not use EditAction to get content mutation notification?
> https://github.com/mozilla/gecko-dev/blob/
> 27b927d770836cdd6f2579df7b40f7edc4f6990a/dom/inputmethod/forms.js#L332
> 
> When the text of a content editable is changed, EditAction() will be called.
> So we may not need _focusContentObserver to do the same job.

No. EditAction does not fire either when clearing through innerHTML.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1059163_v2.patch

Try: https://tbpl.mozilla.org/?tree=Try&rev=bc392e62206b

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8495988 [details] [diff] [review]
bug1059163_v2.patch

Redirecting as :yxl is on PTO.

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8495988 [details] [diff] [review]
bug1059163_v2.patch

Let's do the re-assign dance again

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/46c1cce018de

Comment 22

[Carsten Book [:Tomcat]]

sorry had to back this out again since the new test caused frequent test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2881587&repo=mozilla-inbound

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1059163_v3.patch

Added some safeguards in the tests, let's see what happens. Try @ https://tbpl.mozilla.org/?tree=Try&rev=803449c7ed5c

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1059163_v3.patch

Uploaded wrong patch. Try @ https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=587f6b16f455

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

So previous try run doesn't have the error anymore. Let's do another one to be sure. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b8559fc78b37

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

Didn't see the test failures anymore, so let's give it another shot at landing!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcaa2902761

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbcaa2902761