1279802 - Highlight all disappears when I click middle mouse button to start autoscroll

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

Steps to reproduce:
1. Open long page ( e.g., https://developer.mozilla.org/En/Code_snippets/Tabbed_browser )
2. Ctrl+F and input something (e.g., browser)
3. Click middle mouse button

Actual Results:
Highlight all disapears 

Expected Results:
Highlight all should not disapear

Comment 1

[Alice0775 White]

s/disapear/disappear/

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 3

[Marcia Knous [:marcia]]

Feature has been backed out so not tracking these bugs for 50.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279802 - do not hide the modal highlight on middle click.

Review commit: https://reviewboard.mozilla.org/r/67780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67780/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8775610 [details]
Bug 1279802 - do not hide the modal highlight on middle click.

https://reviewboard.mozilla.org/r/67780/#review65116

::: toolkit/modules/FinderHighlighter.jsm:244
(Diff revision 1)
> +    // Do not hide on middle-click.
> +    if (event && event.type == "mousedown" && event.button == 1)
> +      return;

Why are we hiding it on any mouse activity? Just curious, but it seems jarring to hide the overlay once I start trying to make a selection.

How it currently behaves:

mh = modal highlight
ha = highlight all
mh on / ha on: selection+overlay disappears
mh on / ha off: selection+overlay disappears
mh off / ha on: highlight stays
mh off / ha off: current find match unselects

Since the highlight background color and the current match background color are different than the normal :-moz-selection, we should just keep the selections visible during mouse events such as dragging.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Why are we hiding it on any mouse activity? Just curious, but it seems
> jarring to hide the overlay once I start trying to make a selection.
> 
> How it currently behaves:
> 
> mh = modal highlight
> ha = highlight all
> mh on / ha on: selection+overlay disappears
> mh on / ha off: selection+overlay disappears
> mh off / ha on: highlight stays
> mh off / ha off: current find match unselects
> 
> Since the highlight background color and the current match background color
> are different than the normal :-moz-selection, we should just keep the
> selections visible during mouse events such as dragging.

Would it makes sense then to listen for a real 'click' event instead of 'mousedown'? Perhaps in that case I should also just stop listening to 'touchstart', because it'd be equally jarring.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, we should make sure to match the behavior of the current find-in-page. Canceling the selection on 'click' matches that for me. The selection also clears when I touch on the screen though that is because after the touchstart I get a touchend event followed by a click event. Listening for only `click` will be sufficient, but make sure that is is only button=0 that clears the selection.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775610 [details]
Bug 1279802 - do not hide the modal highlight on middle click.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67780/diff/1-2/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8775610 [details]
Bug 1279802 - do not hide the modal highlight on middle click.

https://reviewboard.mozilla.org/r/67780/#review65730

::: toolkit/modules/FinderHighlighter.jsm:845
(Diff revision 2)
>      if (this._highlightListeners)
>        return;
>  
>      this._highlightListeners = [
>        this._scheduleRepaintOfMask.bind(this, window),
> -      this.hide.bind(this, window)
> +      this.hide.bind(this, window, null)

Do you need this extra argument? I think the default argument should cover this.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Do you need this extra argument? I think the default argument should cover
> this.

Yeah, I want the third argument to be the event object, which'll be passed in as the second if I don't explicitly pass `null` as the second instead.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe5d0a5fa8fe
do not hide the modal highlight on middle click. r=jaws

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Andrew Overholt [:overholt]]

Can we uplift to Aurora given the dupe (1279803) was found in 50?

Comment 15

[Mike de Boer [:mikedeboer]]

Well, the dupe was indeed found in 50, but the feature was disabled for that release. Therefore you won't be able to reproduce it in alpha2.
IOW: no need to uplift.

Comment 16

[Andrew Overholt [:overholt]]

Ah, sorry :)

Comment 17

[roxleitan]

Verified as fixed on Windows 10 and Windows 7 on the latest Nightly 51.0a1 Build ID:20160914030200