Closed Bug 902942 Opened 11 years ago Closed 11 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

Attachments

(2 files, 3 obsolete files)

902942_v3.diff
2.71 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
902942_2_v4.diff
1.04 KB, patch
xyuan
: review+
Details | Diff | Splinter Review 
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.diff
        — 
          — Splinter 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: 11 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.diff
        — 
          — Splinter 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: 11 years ago → 11 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: