Closed Bug 972574 Opened 6 years ago Closed 6 years ago

monocles not matching what is being selected after using double tap under URL text field


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

Windows 8.1


(Not tracked)

Firefox 30


(Reporter: kjozwiak, Assigned: capella)



(Whiteboard: [release28] p=0 s=it-30c-29a-28b.3 r=ff28)


(2 files, 2 obsolete files)

When you double tap on the URL string under the URL text field, the entire string will be highlighted but the two monocles will not represent what is currently being highlighted.

Video of the issue:

Steps to reproduce the issue:

1) Open fxmetro
2) Once you have the browser opened, type in "Wikipedia" and search via any search engine (Google/Bing will work)
3) Once the page loads with your search term, tap on the URL text field and the OSK should slide in from the bottom. At this point, the entire URL is selected and the monocles correctly represent what is being highlighted.
4) Double tap the string the second time around. This time you'll notice that the entire URL string is still highlighted but the monocles are not matching what is being highlighted

Current Behavior:

- Selection grippers are not matching what is being selected when you double tap on the URL string under the URL text field

Expected Behavior:

- When a user double taps on the URL string, everything between the two grippers should be highlighted and everything outside the grippers should not be highlighted

Found the issue using the following builds:
Whiteboard: [triage] [defect] p=0 → [release28] p=0 r=ff28
Assignee: nobody → markcapella
Priority: -- → P1
Attached patch bug972574.diff (v0) (obsolete) — Splinter Review
I borrowed some familiar infrastructure from Androids SelectionHandler for this solution  ;-)
Attachment #8382947 - Flags: review?(mbrubeck)
Comment on attachment 8382947 [details] [diff] [review]
bug972574.diff (v0)

Review of attachment 8382947 [details] [diff] [review]:

Passing this to azasypkin who has looked at selection code much more recently than I.  (Oleg, feel free to pass this on further to jimm if you want additional eyes on it.)
Attachment #8382947 - Flags: review?(mbrubeck) → review?(azasypkin)
QA Contact: kamiljoz
Whiteboard: [release28] p=0 r=ff28 → [release28] p=0 s=it-30c-29a-28b.3 r=ff28
Comment on attachment 8382947 [details] [diff] [review]
bug972574.diff (v0)

Review of attachment 8382947 [details] [diff] [review]:

Overall it looks and works good for me! But for some reason the majority of urlbar mochitests are failing on my local machine. Can you check that?

Also it would be great if you add appropriate mochitest that should help us to avoid future regressions.

Passing further to jimm, master of our selection code, as I'm still new to it :)

::: browser/metro/base/content/library/SelectionPrototype.js
@@ +6,5 @@
>  let Ci = Components.interfaces;
>  let Cc = Components.classes;
> +const kCaretMode = 1;
> +const kSelectionMode = 2;

I see that we define the same constants at ChromeSelectionHandler that looks like spreading the same thing between different files. Can we improve that somehow?

The first thing that comes in mind is to define SelectionPrototype.isInCaretMode and .isInSelectionMode or even define "enum" at SelectionPrototype.Mode = {...: 1, ...:2}. Though I haven't found common approach on defining such "enums" in our codebase, maybe there are some drawbacks of such definitions known?
Attachment #8382947 - Flags: review?(jmathies)
Attachment #8382947 - Flags: review?(azasypkin)
Attachment #8382947 - Flags: review+
Comment on attachment 8382947 [details] [diff] [review]
bug972574.diff (v0)

Ok, let me go ahead and do a try push ... not sure what the affected tests are, as this should be neutral in all situations other than UI handle/monocle (terminology?) positioning.

I also tried to do something global with the duplicate CONST's ... I see this code being eventually being put into SelectionHandler.js ... there's a quirk I noticed working on bug 865369 that I've corrected by cloning this approach, and I now have (3) sets of the CONST's in my patch queue which is sub-optimal.

I'll re-nom for review? with jimm once I've addressed these things ... Thanks ! :-D
Attachment #8382947 - Flags: review?(jmathies)
Attached patch bug972574.diff (v1) (obsolete) — Splinter Review
This addresses the nits, and passes all Try tests :)

Build push:
Test push:
Attachment #8384442 - Flags: review?(jmathies)
Comment on attachment 8384442 [details] [diff] [review]
bug972574.diff (v1)

Review of attachment 8384442 [details] [diff] [review]:

This looks like it could use a test. Mind putting one together before this lands?

::: browser/metro/base/content/helperui/ChromeSelectionHandler.js
@@ +291,5 @@
> +
> +  /*
> +   * _deactivate
> +   *
> +   * Clears ChromeSelectionHandler-specific object vars, previously

comment nits -

"Clears ChromeSelectionHandler-specific object vars" sounds a little confusing, how about something more straight forward like "Resets ChromeSelectionHandler state"? Also "Initialized" shouldn't be capitalized.

::: browser/metro/base/content/library/SelectionPrototype.js
@@ +53,5 @@
>  var SelectionPrototype = function() { }
>  SelectionPrototype.prototype = {
> +  _CARET_MODE: 1, // Single monocle mode (Collapsed caret).
> +  _SELECTION_MODE: 2, // Double monocle mode (Selection w/Text).


@@ +173,5 @@
>    },
> +  /*
> +   * Selection Changed notification listener. This allows us to respond to selection changes
> +   * Introduced programmatically by Gecko events, target-page support code, etc.

nit - no cap on "Introduced".
(In reply to Oleg Zasypkin [:azasypkin] from comment #3)
> The first thing that comes in mind is to define
> SelectionPrototype.isInCaretMode and .isInSelectionMode or even define
> "enum" at SelectionPrototype.Mode = {...: 1, ...:2}. Though I haven't found
> common approach on defining such "enums" in our codebase, maybe there are
> some drawbacks of such definitions known?

I think we usually just do all cap vars, from what I remember you can't do const in an object. I like what we have here thus far.
Comment on attachment 8384442 [details] [diff] [review]
bug972574.diff (v1)

Review of attachment 8384442 [details] [diff] [review]:

::: browser/metro/base/content/helperui/ChromeSelectionHandler.js
@@ +11,2 @@
>  var ChromeSelectionHandler = {
> +  _mode: this._SELECTION_MODE,

Well, looks like "this" here isn't what we're really expecting. That is why I like the approach that we used at Google, like "EventType" enum here:

Above EventType declaration has the following properties that look beneficial for me:

1. It's defined at main object (aka "class") itself (not inside prototype) that made it kinda static and we can use it without real instances(thus removing "this" problem above). Well, in this particular case it's defined at "namespace" but it's easiest example :)

2. We group constants logically that allows us to detect related constants quickly, define common description for enum, and even pass it in the bundle if it's required.

3. I still hope that someday we can annotate our code in some way and automatically generate documentation or enable pre-build static analysis based on these annotations :)

4. Not really a benefit, but it's a bit less verbose as we can use "Mode" string only inside the name of the enum and can omit for enum members: ....Mode.CARET and ...Mode.SELECTION instead of CARET_MODE and SELECTION_MODE

This approach has its own drawbacks for sure, the main that I see is that you need to mention SelectionPrototype everywhere where you use this enum.

Also agree with jimm on "const" problem, don't see elegant solution right now. One of the possible ways will be analyzing this annotations(that we can put inside enum description) on pre-build step and expanding enum to smth different like google closure compiler does. Smth like:

SelectionPrototype.Mode = {CARET: 1} becomes 
Object.defineProperty(SelectionPrototype.Mode, 'CARET', {
   value: 1,
   writable: false,
   enumerable: false,
   configurable: false

Please, see a bit more info on how they define enums below. Of course it's Google's style, but it's comprehensive enough that we can adopt it (at least structure of possible cases) and modify for our needs :)

What do you guys think about it?
Nits addressed ...

(Still looking through Oleg's materials ...)
Attachment #8382947 - Attachment is obsolete: true
Attachment #8384442 - Attachment is obsolete: true
Attachment #8384442 - Flags: review?(jmathies)
Attachment #8385280 - Flags: review?(jmathies)
A nice little test :-)
Attachment #8385282 - Flags: review?(jmathies)
Test pushed to TRY before new changes, note expected failures

Test pushed after building w/new code, note nice and green
Comment on attachment 8385280 [details] [diff] [review]
bug972574.diff (v2)

Review of attachment 8385280 [details] [diff] [review]:

::: browser/metro/base/content/helperui/ChromeSelectionHandler.js
@@ +29,5 @@
>    /*
>     * General selection start method for both caret and selection mode.
>     */
>    _onSelectionAttach: function _onSelectionAttach(aJson) {
> +    // Clear previous ChromeSelectionHandler object vars.

nit - please update this comment as well.

@@ +32,5 @@
>    _onSelectionAttach: function _onSelectionAttach(aJson) {
> +    // Clear previous ChromeSelectionHandler object vars.
> +    this._deactivate();
> +
> +    // Initialize ChromeSelectionHandler object vars.

nit - here too.
Attachment #8385280 - Flags: review?(jmathies) → review+
Comment on attachment 8385282 [details] [diff] [review]
test972574.diff (v0)

Review of attachment 8385282 [details] [diff] [review]:

Attachment #8385282 - Flags: review?(jmathies) → review+
(In reply to Mark Capella [:capella] from comment #14)

Hmm, looks like patch still contains invalid piece of code that I mentioned earlier or am I missing something? :)

 var ChromeSelectionHandler = {
    _mode: this._SELECTION_MODE

Default value of _mode is now "undefined" instead of "2" (_SELECTION_MODE). "this" here is probably global context and we don't have "strict" directive in the file to warn us about it.
   This may be true, though the line is mostly irrelevant and should be removed, or more accurately set to null.

   Valid states are set during module entry-point |_onSelectionAttach()| and maintained properly until being cleared / set to null, during module exit-point |_deactive()|.
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
Went through the verification process using the following Nightly build:

- Went through the original test case from comment #0
- Ensured that initially taping on the URL will slide in the OSK and have the entire URL selected with the two monocles on each end of the string
- Ensured that single taping the URL displayed a single monocle
- Ensured that you can use the single monocle to select the URL string from different positions (beginning, middle and end)
- Ensured that single taping produced a single monocle and double taping produced two monocles at each end of the highlighted string
- Ensured that taping on the empty URL bar under about:start didn't create any monocles but did place a blinking cursor
- Went through the above test cases using different variations of snapped view

There's still some issues with the three monocles appearing at times, Bug #973925 addresses this problem. I've also noticed that sometimes only a single monocle will appear while everything is highlighted rather than the two monocles. I'll try to get some STR and create a new issues as it happens around 1/50 times or so. Marking as verified as the original issue has been fixed.
Flags: needinfo?(kamiljoz)
Fantastic! Please cc: me on the new issues if you identify the STR?
Will do Mark! Great job with the patch, works very well!
You need to log in before you can comment on or make changes to this bug.