894713 - Defect - Monocles being left behind when OSK dismissed

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: monocles being left behind

When sliding in the "Charms" bar from the right side while the user has the monocles visible in the URL bar, the OSK will dismiss but the monocles will be left behind. (issue sometimes occurs when the user dismisses the OSK without the "Charms" bar)

- Attached the screenshot to illustrate the issue

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to wikipedia.org
3) Tap on the URL area and the OSK should appear
4) Tap on the text a few times in the URL bar and get two monocles displayed
5) Slide in the "Charms" bar from the right
6) Dismiss the "Charms" bar from the right by sliding it back in (notice that when you slide in the "Charms" bar, the OSK is dismissed and the monocles are left behind)

Current Behavior:

- Currently, when you slide in the "Charms" bar, the OSK will dismiss but the monocles will be left behind

Expected Behavior:

- When a user slides in the "Charm" bar, the OSK & monocles should be dismissed

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

This adds a couple safety catches for chrome selection. On the ui side we fire a selection update event when the keyboard state changes. On the chrome side we add a blur listener so we can shutdown selection any time the target element loses focus.

Comment 2

[Jim Mathies [:jimm]]

I should probably add a test for this as well.

Comment 3

[Jim Mathies [:jimm]]

Attachment: fix v.2

added a test. confirmed that if we don't register the blur handler, this test fails.

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 780943 [details] [diff] [review]
fix v.2

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

Code looks good an fixes the issue.

::: browser/metro/base/content/helperui/ChromeSelectionHandler.js
@@ +299,5 @@
> +    }
> +    let bindingParent = null;
> +    try {
> +      // if focusedElement is null, this will throw
> +      bindingParent = this._contentWindow.document.getBindingParent(document.commandDispatcher.focusedElement);

Can we test if focusedElement is null before calling getBindingParent to avoid the try catch?

@@ +321,5 @@
>      }
>    },
>  
> +  handleEvent: function handleEvent(aEvent) {
> +    if (aEvent.type == "blur" && !this._targetHasFocus()) {

Could it happen that this._targetHasFocus is true after _targetElement gets a blur?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #4)
> Can we test if focusedElement is null before calling getBindingParent to
> avoid the try catch?

Sure I can try that. I wasn't sure what other errors in that call might throw so I went with the trap.

> 
> @@ +321,5 @@
> >      }
> >    },
> >  
> > +  handleEvent: function handleEvent(aEvent) {
> > +    if (aEvent.type == "blur" && !this._targetHasFocus()) {
> 
> Could it happen that this._targetHasFocus is true after _targetElement gets
> a blur?

Apparently not from testing this patch.

Comment 6

[Jim Mathies [:jimm]]

Attachment: followup

I'm going to push a quick follow up to this. While running tests I noticed an occasional "this._targetElement is null" error showing up in the logs at ChromeSelectionHandler.js:804.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e669a5c499e
https://hg.mozilla.org/mozilla-central/rev/b5aa56bc292b

Comment 8

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130805030205

WFM
Tested on Windows 8 for iteration-11 using latest nightly built from http://hg.mozilla.org/mozilla-central/rev/482b9d04974a

I followed steps provided in comment0 and got expected result.
When I slided in the "Charm" bar, the OSK & monocles was dismissed.

Comment 9

[Samvedana (:Samvedana)]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #13 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-09-09-03-02-04-mozilla-central/

- Went through the original test case from comment #0 without any issues
- Ensured that dismissing the Navigation App Bar by tapping on the website didn't leave the monocles behind
- Ensured that dismissing the Navigation App Bar by sliding it to the bottom didn't leave the monocles behind
- Ensured that the above test cases also worked with filled view without any issues

Found several issues with the monocles not appearing but the entire URL was selected, will create new issues.