Kamil Jozwiak [:kjozwiak] Reporter
	Description • 10 years ago

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:
- http://www.youtube.com/watch?v=tPmG1G9R9kg

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:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-13-03-02-01-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-13-00-40-02-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/

	Mark Capella [:capella] Assignee
	Comment 1 • 10 years ago

I borrowed some familiar infrastructure from Androids SelectionHandler for this solution  ;-)

	Matt Brubeck (:mbrubeck)
	Comment 2 • 10 years ago

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.)

	Oleg Zasypkin [:azasypkin]
	Comment 3 • 10 years ago

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?

	Mark Capella [:capella] Assignee
	Comment 4 • 10 years ago

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

	Mark Capella [:capella] Assignee
	Comment 5 • 10 years ago

This addresses the nits, and passes all Try tests :)

Build push: https://tbpl.mozilla.org/?tree=Try&rev=04a95f3d54e8
Test push:  https://tbpl.mozilla.org/?tree=Try&rev=64bd61dd9b7b

	Jim Mathies [:jimm]
	Comment 6 • 10 years ago

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).

nice.

@@ +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".

	Jim Mathies [:jimm]
	Comment 7 • 10 years ago

(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.

	Oleg Zasypkin [:azasypkin]
	Comment 8 • 10 years ago

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:
https://code.google.com/p/closure-library/source/browse/closure/goog/events/eventtype.js

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 :)

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

What do you guys think about it?

	Mark Capella [:capella] Assignee
	Comment 9 • 10 years ago

Nits addressed ...

(Still looking through Oleg's materials ...)

	Mark Capella [:capella] Assignee
	Comment 10 • 10 years ago

A nice little test :-)

	Mark Capella [:capella] Assignee
	Comment 11 • 10 years ago

Test pushed to TRY before new changes, note expected failures
   https://tbpl.mozilla.org/?tree=Try&rev=a84786952f26

Test pushed after building w/new code, note nice and green
   https://tbpl.mozilla.org/?tree=Try&rev=e4a48492ea34

	Jim Mathies [:jimm]
	Comment 12 • 10 years ago

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.

	Jim Mathies [:jimm]
	Comment 13 • 10 years ago

Comment on attachment 8385282 [details] [diff] [review]
test972574.diff (v0)

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

Thanks!

	Mark Capella [:capella] Assignee
	Comment 14 • 10 years ago

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

	Oleg Zasypkin [:azasypkin]
	Comment 15 • 10 years ago

(In reply to Mark Capella [:capella] from comment #14)
> https://hg.mozilla.org/integration/fx-team/rev/0faa1c37a027

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.

	Mark Capella [:capella] Assignee
	Comment 16 • 10 years ago

   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()|.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0faa1c37a027

	Marco Mucci [:MarcoM]
	Comment 18 • 10 years ago

For testing and verification.  Reopen if any defects found.

	Kamil Jozwiak [:kjozwiak] Reporter
	Comment 19 • 10 years ago

Went through the verification process using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-05-03-02-01-mozilla-central/

- 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.

	Mark Capella [:capella] Assignee
	Comment 20 • 10 years ago

Fantastic! Please cc: me on the new issues if you identify the STR?

	Kamil Jozwiak [:kjozwiak] Reporter
	Comment 21 • 10 years ago

Will do Mark! Great job with the patch, works very well!