Closed Bug 902942 Opened 8 years ago Closed 8 years ago

[InputContext] forms.js element deletion doesn't update focusedElement

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: janjongboom, Assigned: janjongboom)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Weird issue:

<div id="container"></div>

var container = document.getElementById('workspace');
container.innerHTML = '<textarea id="test2">Hi!</textarea>';
document.getElementById('test2').focus();
container.innerHTML = '';
container.innerHTML = '<div id="test3" contenteditable="true">Hi</div>';
document.querySelector('#test3').focus();

After this code a focus event comes into forms.js, it has a ref to the right element etc. But now we redirect this to `getSelectionInfo()` and there we use `let element = this.focusedElement;`

However, this is not correct because this returns the old (deleted!) element. Thus returning the wrong information.

I believe this is because in `showKeyboard` we check:
```
    if (this.isKeyboardOpened)
      return;
```
But if this condition is true then we won't update focusedElement, which is incorrect.
Attached patch 902942.diff (obsolete) — Splinter Review
Assignee: nobody → janjongboom
Attachment #787599 - Flags: review?(xyuan)
I actually think this patch might be wrong. At the moment the keyboard doesn't disappear when deleting an element with focus, so we should have a MutationObserver on the focused element and treat a deletion as a blur. Then everything would be fine.
Attached patch 902942_v2.diff (obsolete) — Splinter Review
This patch uses DOM Observers instead. Think that's nicer.
Attachment #787599 - Attachment is obsolete: true
Attachment #787599 - Flags: review?(xyuan)
Attachment #787640 - Flags: review?(xyuan)
It is surprising that removing a focused element doesn't trigger a blur. I guess it is a bug with Firefox.

@Mounir, you're an export on DOM and can you help to identify the issue?

I tested the following code in Firefox and Chromium. 

For Firefox, the output is strange that I got four `focus` event:
  focus
  focus 
  focus
  focus

For Chromium, I got expected result:
  focus
  blur
  focus

------------------------------------------------------------
<html>
<body>
<div id="container"></div>
<script>
addEventListener('blur', function(evt) {
  console.log('blur');
}, true);
addEventListener('focus', function(evt) {
  console.log('focus');
}, true);

var container = document.getElementById('container');
container.innerHTML = '<textarea id="test2">Hi!</textarea>';
document.getElementById('test2').focus();
container.innerHTML = '';
container.innerHTML = '<div id="test3" contenteditable="true">Hi</div>';
document.querySelector('#test3').focus();
</script>
</body>
</html>
Flags: needinfo?(mounir)
Comment on attachment 787640 [details] [diff] [review]
902942_v2.diff

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

The patch doesn't work with the test code of comment 4. 

The expected event sequence is:
 focus of #test2
 blur of #test2
 focus of #test3

but the patched sequence is:
 focus of #test2
 focus of #test3
 blur of #test2

The faked blur event of the first removed element comes after the focus event of the second element. This makes keyboard confusing that the last event is blur, but actually these is an element being focused.

To resolve the issue, we should fire a faked blur event if the focused element is changed without receiving the blur event.

::: b2g/chrome/content/forms.js
@@ +229,5 @@
>      this._focusedElement = val;
>    },
>  
>    setFocusedElement: function fa_setFocusedElement(element) {
> +    let self = this;

I suggest declare the variable inside the block where it is used.

@@ +238,5 @@
>      if (this.focusedElement) {
>        this.focusedElement.removeEventListener('mousedown', this);
>        this.focusedElement.removeEventListener('mouseup', this);
> +      if (this._observer) {
> +        this._observer.disconnect();

set this._observer to null to completely remove it as we don't need it any more.
Attachment #787640 - Flags: review?(xyuan) → review-
Neil is our resident focus expert still I believe.
Flags: needinfo?(enndeakin)
This is caused by bug 559561.
Flags: needinfo?(enndeakin)
Hi Jonas and Neil,
thanks for your quick response.

The cause is found.
Depends on: 559561
Flags: needinfo?(mounir)
Are we going to wait for bug 559561 to land? As this currently blocks running tests for InputMethod.
Flags: needinfo?(xyuan)
Let's wait for a while. Could you change your tests to avoid this issue?

For example:
--------------------------------------

<div id="container">
  <textarea id="test2">Hi!</textarea>
  '<div id="test3" contenteditable="true">Hi</div>
</div>

document.getElementById('test2').focus();

document.getElementById('test3').focus();
Flags: needinfo?(xyuan)
(In reply to Yuan Xulei [:yxl] from comment #5)
> The faked blur event of the first removed element comes after the focus
> event of the second element. This makes keyboard confusing that the last
> event is blur, but actually these is an element being focused.
> 
> To resolve the issue, we should fire a faked blur event if the focused
> element is changed without receiving the blur event.

I'm sorry I should be more elaborate about this. This doesn't fix behavior for normal blur events but just our internal code flow for forms.js, so making sure that `oninputcontextchange` is fired at all times when appropriate. Here is a larger test case:

```
<html>
<body>
<div id="container"></div>
<script>

var container = document.getElementById('container');

var _tests = [];
var test = function(title, fn) {
  _tests.push([title, fn]);
};

test('Event fires on textbox focus', function(next) {
  navigator.mozInputMethod.oninputcontextchange = function() {
    dump('icc1 ' + !!navigator.mozInputMethod.inputcontext + '\n');
    if (navigator.mozInputMethod.inputcontext) {
      dump('1tbc ' + navigator.mozInputMethod.inputcontext.textAfterCursor + '\n');
      next();
    }
  };
  container.innerHTML = '<input type="text" id="test" value="Normal textbox" />';
  document.querySelector('#test').focus();
});

test('Event fires on textarea focus', function(next) {
  navigator.mozInputMethod.oninputcontextchange = function() {
    dump('icc2 ' + !!navigator.mozInputMethod.inputcontext + '\n');
    if (navigator.mozInputMethod.inputcontext) {
      dump('2tbc ' + navigator.mozInputMethod.inputcontext.textAfterCursor + '\n');
      next();
    }
  };
  container.innerHTML = '<textarea id="test2">Hi!</textarea>';
  document.querySelector('#test2').focus();
});

test('Event fires on contenteditable focus', function(next) {
  navigator.mozInputMethod.oninputcontextchange = function() {
    dump('icc3 ' + !!navigator.mozInputMethod.inputcontext + '\n');
    if (navigator.mozInputMethod.inputcontext) {
      dump('3tbc ' + navigator.mozInputMethod.inputcontext.textAfterCursor + '\n');
      next();
    }
  };
  container.innerHTML = '<div id="test3" contenteditable="true">Hi</div>';
  document.querySelector('#test3').focus();
});

(function run() {
  function next() {
    var test = _tests.splice(0, 1)[0];
    if (!test)
      return;
    console.log('running test', test[0]);
    container.innerHTML = '';
    test[1](next);
  }
  next();
})();
</script>
</body>
</html>
```

With patch applied gives:

```
icc1 true
1tbc Normal textbox
icc2 false
icc2 true
2tbc Hi!
icc3 false
icc3 true
3tbc Hi
icc3 false
```

Without patch applied:

```
icc1 true
1tbc Normal textbox
icc2 false
icc2 true
2tbc Hi!
```

So execution just stops because we hit a code path where we think that the state is correct, but in reality it has changed.
I can work around it by focusing a different element outside of the container before clearing innerHTML, so that'll work :-)
:yxl, FYI, https://github.com/comoyo/gaia/commit/4888a218f2456eab215378a5be2b8b02059337ef

I think we can close this one. Should work once bug 559561 is patched.
Thanks for your code.
It is a potential bug for the keyboard. I think we'd leave this open until bug 559561 is fixed.
Bug 559561 hasn't seen any activity for the past 3 years. And it doesn't even contain a decision on what behavior we want. So I would not wait for that to get fixed.
Thinking of this, we are going to have serious trouble, because when a client application deletes an element with focus the keyboard will be f* up; and the user will blame it on the OS.
Flags: needinfo?(xyuan)
you can add a mutationobserver to notice when the focused element is removed.

or if you are using C++ to implement, you can use nsIMutationObserver
Yeah that's what I'm doing in this patch at the moment :-)
Attachment #787640 - Flags: review- → review?(xyuan)
Clearing ni? and setting r?
Flags: needinfo?(xyuan)
Comment on attachment 787640 [details] [diff] [review]
902942_v2.diff

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

Please fix these in comment 5 and below.

::: b2g/chrome/content/forms.js
@@ +244,3 @@
>        if (!element) {
>          this.focusedElement.blur();
>        }

Fake a blur event here if the focused element is changed before mutation observer gets notification.
Attachment #787640 - Flags: review?(xyuan) → review-
(In reply to Yuan Xulei [:yxl] from comment #20)
> Fake a blur event here if the focused element is changed before mutation
> observer gets notification.
I don't get this :p We blur() it here so an event would come through anyway?
(In reply to Jan Jongboom [:janjongboom] from comment #21)
> (In reply to Yuan Xulei [:yxl] from comment #20)
> > Fake a blur event here if the focused element is changed before mutation
> > observer gets notification.
> I don't get this :p We blur() it here so an event would come through anyway?
We can't blur it here, because it makes the test case of Comment 4 failed.
blur event should be sent before the new focus event. If we blur it here, the blur event won't be sent until we finish handling the focus event.
Can't fix that here, this is a workaround for bug 559561. We either don't fire a blur event at all, or it'll come after the focus unless we properly fix the other bug.
If so, please address comment 4 only.
Please don't change what events we are firing in the page. If we want to do so we should do that as part of bug 559561 so that it works consistently across all Gecko platforms.

I think the workaround here simply needs to act as if a blur event was fired. Without actually firing one.
Good. That's what we're doing here now.
Attachment #787640 - Flags: review- → review?(xyuan)
Comment on attachment 787640 [details] [diff] [review]
902942_v2.diff

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

Hi Jan, please fix comments below before checking in. :-)

::: b2g/chrome/content/forms.js
@@ +238,5 @@
>      if (this.focusedElement) {
>        this.focusedElement.removeEventListener('mousedown', this);
>        this.focusedElement.removeEventListener('mouseup', this);
> +      if (this._observer) {
> +        this._observer.disconnect();

Set this._observer to null to help free memory.

@@ +244,3 @@
>        if (!element) {
>          this.focusedElement.blur();
>        }

Fake a blur event if the previous focusedElement doesn't get blured. The code may like this:
else {
  this.handleEvent({ target: element, type: "blur" });
}

@@ +279,5 @@
> +            return n === element;
> +          });
> +        });
> +        if (del) {
> +          // item was deleted, fake a blur so all state gets set correctly

Make a check to ensure element == this.focusedElement when faking the blur event.
Attachment #787640 - Flags: review?(xyuan)
(In reply to Yuan Xulei [:yxl] from comment #27)
> @@ +244,3 @@
> >        if (!element) {
> >          this.focusedElement.blur();
> >        }
> 
> Fake a blur event if the previous focusedElement doesn't get blured. The
> code may like this:
> else {
>   this.handleEvent({ target: element, type: "blur" });
> }
No, this should be a real blur() call otherwise we'll trigger a recursion.

I've addressed the other comments.
(In reply to Yuan Xulei [:yxl] from comment #27)
> @@ +279,5 @@
> > +            return n === element;
> > +          });
> > +        });
> > +        if (del) {
> > +          // item was deleted, fake a blur so all state gets set correctly
> 
> Make a check to ensure element == this.focusedElement when faking the blur
> event.
I think this is redundant, because the observer is removed when focused element changes, so this will always be true here.
Attached patch 902942_v3.diff (obsolete) — Splinter Review
Attachment #787640 - Attachment is obsolete: true
Attachment #793407 - Flags: review?(xyuan)
Attached patch 902942_v3.diffSplinter Review
Attachment #793407 - Attachment is obsolete: true
Attachment #793407 - Flags: review?(xyuan)
Attachment #793409 - Flags: review?(xyuan)
Comment on attachment 793409 [details] [diff] [review]
902942_v3.diff

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

r=me. The workaround is not perfect, but it resolves most of the problems. Thanks for your work.
Attachment #793409 - Flags: review?(xyuan) → review+
Thanks Yuan.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b5c3f87f17ea
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reopening, we're testing against `this.focusedElement` which is undefined, because different scope, should be `self.focusedElement`...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 902942_2_v4.diffSplinter Review
Hi Yuan, when trying some new stuff I found out that this patch was errornous. We should be checking against self.focusedElement...
Attachment #796603 - Flags: review?(xyuan)
Attachment #796603 - Flags: review?(xyuan) → review+
checkin-needed for 902942_2_v4.diff
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/633ac217d31d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.