914447 - Defect - Monocles not always appearing around selected URL in Navigation App Bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: Monocles missing from URL

When tapping on the Navigation App Bar to select the URL, sometimes the monocles will not appear at both ends of the selected URL.

- Attached a screenshot to illustrate the issue (notice the monocles missing)

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to wikipedia.org
3) Slide in the Navigation App Bar from the bottom and tap on the URL (entire URL should be selected and a monocle on both ends should appear)
4)Slide in the Windows Charm from the right side of the screen (the monocles should disappear)
5) Tap on the website to dismiss the Windows Charm and then re-tap the URL in the Navigation App Bar (monocles should re-appear)
6) Go through Step #4 & #5 a few times (sometimes it takes 10-15 times) and you'll notice that sometimes the monocles will not appear

Current Behavior:

- Monocles not always appearing when tapping on the URL under the Navigation App Bar

Expected Behavior:

- Monocles should be appearing every single time when tapping on the URL under the Navigation App Bar

Comment 1

[Jim Mathies [:jimm]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #0)
> 1) Open Firefox Metro
> 2) Go to wikipedia.org
> 3) Slide in the Navigation App Bar from the bottom and tap on the URL
> (entire URL should be selected and a monocle on both ends should appear)
> 4)Slide in the Windows Charm from the right side of the screen (the monocles
> should disappear)
> 5) Tap on the website to dismiss the Windows Charm and then re-tap the URL
> in the Navigation App Bar (monocles should re-appear)
> 6) Go through Step #4 & #5 a few times (sometimes it takes 10-15 times) and
> you'll notice that sometimes the monocles will not appear

Excellent str, thanks for this.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

This was caused by clicks that fell within the bounds of the urlbar but outside the bounds of internal text input. When we sent this over, the selection handler didn't find an editable at the event coordinates.

Note the 10 px offset feels a little cheesy, maybe there's a way to dig down and get the actual text input box object? Regardless this still works reliably.

Also added a little cleanup and commenting that seemed lacking when I first started debugging this.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8343089 [details] [diff] [review]
patch v.1

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

::: browser/metro/base/content/bindings/bindings.xml
@@ +199,4 @@
>              SelectionHelperUI.attachEditSession(ChromeSelectionHandler,
>                                                  event.clientX, event.clientY);
>            } else {
> +            dump("click: sending observer\n");

removed these dumps from my local patch.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8343089 [details] [diff] [review]
patch v.1

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

r=mbrubeck with some minor changes:

::: browser/metro/base/content/browser.xul
@@ +246,5 @@
>                       autocompletepopup="urlbar-autocomplete"
>                       completeselectedindex="true"
>                       placeholder="&urlbar.emptytext;"
>                       tabscrolling="true"
> +                     onclick="SelectionHelperUI.urlbarTextboxClick(this);"/>

Try passing (this.inputField) instead of (this) -- that should be a reference to the internal <html:input> element, which will maybe eliminate the need for the 10px offset?

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +532,5 @@
> +    }
> +
> +    // Attempt to enable selection if there's text in the control.
> +    this.attachEditSession(ChromeSelectionHandler,
> +      aEdit.boxObject.screenX + 10, aEdit.boxObject.screenY + 10);

If we keep the 10px offset, this needs a better comment explaining it.

Comment 5

[Jim Mathies [:jimm]]

> Try passing (this.inputField) instead of (this) -- that should be a

That's what I was looking for, thanks. There wasn't a boxObject on this element so I used data in getBoundingClientRect(), which worked.

Comment 6

[Jim Mathies [:jimm]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9d3b9da94bd