Closed Bug 1494274 Opened 3 years ago Closed 3 years ago

Port clickSelectsAll and doubleClickSelectsAll behavior from urlbarBindings.xml to UrlbarInput.jsm

Categories

(Firefox :: Address Bar, task, P2)

task

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Depends on: 1491255
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #9012586 - Flags: review?(mak77)
Attached patch patchSplinter Review
Attachment #9012586 - Attachment is obsolete: true
Attachment #9012586 - Flags: review?(mak77)
Attachment #9012809 - Flags: review?(mak77)
Comment on attachment 9012809 [details] [diff] [review]
patch

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

I'm a bit worried by the lack of tests, this rewrite is a good occasion to improve on coverage, I think we should spend some time on those, especially for optional behaviors that are usually uncovered. That said, it's possible this is covered by existing tests we'll convert in the future?

::: browser/components/urlbar/UrlbarInput.jsm
@@ +339,5 @@
>        return;
>      }
>  
> +    Cc["@mozilla.org/widget/clipboardhelper;1"]
> +      .getService(Ci.nsIClipboardHelper)

can we memoize this in a defineLazyServiceGetter in the module scope?

::: browser/components/urlbar/UrlbarPrefs.jsm
@@ +49,5 @@
>    // fetching results.  However, we ignore this for the very first result (the
>    // "heuristic" result).  We fetch it as fast as possible.
>    ["delay", 50],
>  
> +  ["doubleClickSelectsAll", false],

Please add comments for each prefs, we are using this module also to better document the urlbar prefs, that will be useful for future insight of people working on experiments.
Attachment #9012809 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #3)
> Please add comments for each prefs, we are using this module also to better
> document the urlbar prefs, that will be useful for future insight of people
> working on experiments.

I'm not sure what to document here. The pref name seems self-explanatory enough, and repeating "controls whether a (double) click selects all text" doesn't seem useful...
It might be worth documenting why (because of how window managers work), which also leads to the fact the defaults are different on mac/windows versus linux.
(In reply to Mark Banner (:standard8) from comment #5)
> It might be worth documenting why (because of how window managers work),

What do you mean by that?

> which also leads to the fact the defaults are different on mac/windows
> versus linux.

Afaik they're different because of the Primary selection on Linux. This would probably be better documented where we set the default for Linux, i.e. in firefox.js.
Flags: needinfo?(standard8)
(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Mark Banner (:standard8) from comment #5)
> > It might be worth documenting why (because of how window managers work),
> 
> What do you mean by that?

Not just documenting what it does, but why it does it or why it is present. As you indicated, you can easily see that it controls the click selection. However, the last time I came across this (fairly recently), I had to do a reasonable amount of digging to find out that it was to do with pasting on linux versus other platforms.

I guess I was thinking of something like "Controls how click selection works on the urlbar. This is used to handle different ways of selecting text. For example, Linux uses a single middle-click to paste, and as a result turns this off, as otherwise the paste buffer is lost."

(I'm not 100% sure the example is correct, that's from what I remember).

> > which also leads to the fact the defaults are different on mac/windows
> > versus linux.
> 
> Afaik they're different because of the Primary selection on Linux. This
> would probably be better documented where we set the default for Linux, i.e.
> in firefox.js.

I'm torn here. firefox.js is in a different location from the code that implements the prefs, so you have to dig to find it if you don't know about it. On the flip side, firefox.js is the place that a lot of people will look, though you could say the same about all.js as well...
Flags: needinfo?(standard8)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d895e3296e
Port clickSelectsAll and doubleClickSelectsAll behavior from urlbarBindings.xml to UrlbarInput.jsm. r=mak
https://hg.mozilla.org/mozilla-central/rev/91d895e3296e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Depends on: 1554542
Type: enhancement → task
Blocks: 1554864
Blocks: 1528614
No longer blocks: 1528614
You need to log in before you can comment on or make changes to this bug.