Closed Bug 1279802 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
x86
Windows 10
defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
mozilla51
Iteration:
50.4 - Aug 1
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- verified

People

(Reporter: alice0775, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
s/disapear/disappear/
Summary: Highlight all disapears when I click middle mouse button to start autoscroll → Highlight all disappears when I click middle mouse button to start autoscroll
Feature has been backed out so not tracking these bugs for 50.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Points: --- → 1
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.
Attachment #8775610 - Flags: review?(jaws) → review-
(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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
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/
Attachment #8775610 - Flags: review- → review?(jaws)
Blocks: 1291278
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.
Attachment #8775610 - Flags: review?(jaws) → review+
(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.
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe5d0a5fa8fe
do not hide the modal highlight on middle click. r=jaws
https://hg.mozilla.org/mozilla-central/rev/fe5d0a5fa8fe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1279803
Depends on: 1295539
Depends on: 1295540
Can we uplift to Aurora given the dupe (1279803) was found in 50?
Flags: needinfo?(mdeboer)
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.
Flags: needinfo?(mdeboer)
Ah, sorry :)
Verified as fixed on Windows 10 and Windows 7 on the latest Nightly 51.0a1 Build ID:20160914030200
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.