957646 - Selection monocles sometimes don't display when tapping text ion the nav bar

Status: VERIFIED FIXED

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

Description

[Jim Mathies [:jimm]]

str:

1) load a tab
2) bring up the nav bar and tap url text

result: 1 out of every 10 taps doesn't get selection monocles.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: wip.diff

Here is general idea suggested by mbrubeck: we can use click\target directly to attach selection instead of detecting it via client coordinates(that may result into errors due to fluffing). Currently I'm passing both client coordinates and appropriate targets at the same time as selection processing code is reused by both chrome and content. 

Also as component intercommunication is based on message manager(sendAsyncMessage) that accepts only serializable parameters, target elements can't be passed this way. So, currently it works only for components that pass messages via SelectionPrototype's descendants (ex. Chrome inputs\textboxes). For the rest of cases coordinates are used. Probably passing target node references via CPOWs once browser.tabs.remote is turned on would work.

Specifically to urlbar: we still recreate moncles twice, the first time via urlbar click handler and the second time via window.click handler.

What are your thoughts on that, Jim?

Comment 2

[Oleg Zasypkin [:azasypkin]]

Attachment: urlbar v2.diff

Some updates after deeper testing to unify selection attachment + removed workaround code for bug 925457 - can't really reproduce it with the latest changes. Current tests are passing.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8373607 [details] [diff] [review]
urlbar v2.diff

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

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +496,5 @@
>          this._onSelectionStart(json);
>          break;
>  
>        case "Browser:SelectionAttach":
> +        this._onSelectionAttach(json.xPos, json.yPos, json.target);

is json.target valid here?

Comment 4

[Oleg Zasypkin [:azasypkin]]

Yeah, almost every change (except one related to workaround for bug 925457) under contenthandlers folder should be reverted if we don't have way to pass "content" target reference via sendAsyncMessage. I found only one way via CPOWs, but it depends on e10s, just wanted to get your feedback first in case you know one more way to pass "content" target reference.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #4)
> Yeah, almost every change (except one related to workaround for bug 925457)
> under contenthandlers folder should be reverted if we don't have way to pass
> "content" target reference via sendAsyncMessage. I found only one way via
> CPOWs, but it depends on e10s, just wanted to get your feedback first in
> case you know one more way to pass "content" target reference.

I don't think we would ever need to. Once we switch to e10s, front end won't have references to content elements to send. Everything will have to happen over in the content process. Since we share some code here with the chrome process via ChromeSelectionHandler those we may occasionally pass an element, which is fine. I just want to make sure the processing of those json.targets in the case where they aren't valid doesn't break anything.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Attachment: urlbar v3.diff

Ok, thanks, anyway we will have to replace elementFromPoint invocations with actual targets where it's possible to deal better with fluffing. I reverted unused changes from content related files to avoid confusion.

Also I was looking into patches related to bug 925457 workaround and didn't notice any tests for that, so there is a high risk of regression in case I remove that, so I reverted it back too. Now patch contains only chrome related changes.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Thanks for review! TRY tests have passed: https://tbpl.mozilla.org/?tree=Try&rev=1726b46a068b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/66fd6bc61c56

Just a friendly reminder that your commit message should be summarizing what the patch is doing, not just restating the problem it's trying to fix.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66fd6bc61c56

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process for Nightly without any issues, used the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-14-03-02-02-mozilla-central/

Reproduced the original issue before testing, used the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/01/2014-01-08-03-02-03-mozilla-central/

- Went through the original test case from comment #0 without any issues
- Used both long and short URLs and ensured that the monocles are appearing every single time the URL text is selected
- Ensured that the monocles are behaving correctly once they appear inside the URL text field
- Went through the above test cases in several different variations of snapped view without issues
- Taped the URL text field at least 200+ times and ensured that the monocles appeared every single time

Jim, should this patch be uplifted as this seems important? If it's not being uplifted, I will change the status to verified.

Comment 11

[Jim Mathies [:jimm]]

depends on how serious bug 972428 is I guess.

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Build ID: 20140219030203

Verified as fixed on latest Nightly.