Closed Bug 894713 Opened 6 years ago Closed 6 years ago

Defect - Monocles being left behind when OSK dismissed

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: kjozwiak, Assigned: jimm)

References

Details

(Whiteboard: feature=defect u=metro_firefox_user c=content_features p=1)

Attachments

(3 files, 1 obsolete file)

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
Blocks: 831909
Priority: -- → P2
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: nobody → jmathies
Attachment #780934 - Flags: review?(rsilveira)
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Whiteboard: feature=defect u=metro_firefox_user c=content_features p=0 → feature=defect u=metro_firefox_user c=content_features p=1
I should probably add a test for this as well.
Attached patch fix v.2Splinter Review
added a test. confirmed that if we don't register the blur handler, this test fails.
Attachment #780934 - Attachment is obsolete: true
Attachment #780934 - Flags: review?(rsilveira)
Attachment #780943 - Flags: review?(rsilveira)
Status: NEW → ASSIGNED
QA Contact: jbecerra
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?
Attachment #780943 - Flags: review?(rsilveira) → review+
(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.
Attached patch followupSplinter Review
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.
https://hg.mozilla.org/mozilla-central/rev/3e669a5c499e
https://hg.mozilla.org/mozilla-central/rev/b5aa56bc292b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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.
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.
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.