1304073 - Dimmed highlight all should not disappear when you select the text with mouse drag

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Steps to reproduce:
1. Find for any term
2. Attempt to select any text of the page with mouse drag.

Actual Results:
Dimmed highlight all disappears

Expected Results:
Dimmed highlight all should not disappear.

Comment 1

[Gerry Chang [:gchang]]

This feature will be disabled in 51.

Comment 2

[Marcia Knous [:marcia]]

Tracking 52+ for this dimmed highlight feature.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

We'll ignore mouse events with the following properties:
1) a modifier key was pressed during the click,
2) an anchor element was clicked,
3) the 'selectstart' event fired earlier, indicating a text selection action
4) the mouse pointer had moved earlier, indicating that the click was not intended to hide the dimmed background
5) 'relatedTarget' is set, indicating a drag action or touch event.

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

Comment 4

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

Comment on attachment 8794879 [details]
Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

https://reviewboard.mozilla.org/r/81134/#review79726

::: toolkit/modules/FinderHighlighter.jsm:328
(Diff revision 1)
> +        event.ctrlKey || event.metaKey || event.shiftKey || event.clientX != event.movementX ||
> +        event.clientY != event.movementY || event.relatedTarget || event.target.localName == "a"

I don't think you're using movementX and movementY as they are intended to be used here. These are the deltas from the previous event, so why would movementX be equal to clientX?

::: toolkit/modules/FinderHighlighter.jsm:329
(Diff revision 1)
>      let dict = this.getForWindow(window);
>  
> +    // Do not hide on anything but a left-click.
> +    if (event && event.type == "click" && (event.button !== 0 || event.altKey ||
> +        event.ctrlKey || event.metaKey || event.shiftKey || event.clientX != event.movementX ||
> +        event.clientY != event.movementY || event.relatedTarget || event.target.localName == "a"

We should hide if it's an <a> element without an href attribute.

::: toolkit/modules/FinderHighlighter.jsm:1196
(Diff revision 1)
>      let target = this.iterator._getDocShell(window).chromeEventHandler;
>      target.addEventListener("MozAfterPaint", dict.highlightListeners[0]);
>      target.addEventListener("resize", dict.highlightListeners[1]);
>      target.addEventListener("scroll", dict.highlightListeners[2]);
>      target.addEventListener("click", dict.highlightListeners[3]);
> +    target.addEventListener("selectstart", dict.highlightListeners[4]);

"selectstart" is only enabled on Nightly, see https://bugzilla.mozilla.org/show_bug.cgi?id=1231923 for tracking enabling it outside of Nightly.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> ::: toolkit/modules/FinderHighlighter.jsm:328
> (Diff revision 1)
> > +        event.ctrlKey || event.metaKey || event.shiftKey || event.clientX != event.movementX ||
> > +        event.clientY != event.movementY || event.relatedTarget || event.target.localName == "a"
> 
> I don't think you're using movementX and movementY as they are intended to
> be used here. These are the deltas from the previous event, so why would
> movementX be equal to clientX?

Well, that's why I observed anyway. But this was my poor-mans' attempt at implementing 'selectstart' detection, so this approach is flawed anyway. I'll remove the movement-related stuff.

> ::: toolkit/modules/FinderHighlighter.jsm:329
> (Diff revision 1)
> >      let dict = this.getForWindow(window);
> >  
> > +    // Do not hide on anything but a left-click.
> > +    if (event && event.type == "click" && (event.button !== 0 || event.altKey ||
> > +        event.ctrlKey || event.metaKey || event.shiftKey || event.clientX != event.movementX ||
> > +        event.clientY != event.movementY || event.relatedTarget || event.target.localName == "a"

No, because links jumping to an <a name=foo/> in the same page shouldn't hide the dimmed background. So <a href=#foo/> shouldn't hide, but <a href=google.com/> will cause a LocationChange anyway, so we're covered.
This actually makes the case to only ignore clicks on anchor elements _with_ the href attribute set.

> "selectstart" is only enabled on Nightly, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1231923 for tracking enabling
> it outside of Nightly.

Bah! I n-i'd Ehsan in that bug to help us both out!

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8794879 [details]
Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

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

Comment 7

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

Comment on attachment 8794879 [details]
Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

https://reviewboard.mozilla.org/r/81134/#review79756

::: toolkit/modules/FinderHighlighter.jsm:330
(Diff revisions 1 - 2)
>        dict.busySelecting = false;
>        return;
>      }
> +    dict.busySelecting = false;

If both of these set busySelecting to false, then there is no code path at this point that doesn't set busySelecting to false.

We should just move dict.busySelecting = false; to be above the "Do not hide ..." comment. Does that make sense code-wise to you?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8794879 [details]
Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

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

Comment 9

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

Comment on attachment 8794879 [details]
Bug 1304073 - don't hide the modal highlight dimmed background upon a mouse click under certain conditions.

https://reviewboard.mozilla.org/r/81134/#review82494

This should wait until bug 1231923 lands and sticks.

::: toolkit/modules/FinderHighlighter.jsm:1204
(Diff revision 3)
>      let target = this.iterator._getDocShell(window).chromeEventHandler;
>      target.addEventListener("MozAfterPaint", dict.highlightListeners[0]);
>      target.addEventListener("resize", dict.highlightListeners[1]);
>      target.addEventListener("scroll", dict.highlightListeners[2]);
>      target.addEventListener("click", dict.highlightListeners[3]);
> +    target.addEventListener("selectstart", dict.highlightListeners[4]);

Did you test this with the patch in bug 1231923? I don't see a reply on there from you yet.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec4c356610af
don't hide the modal highlight dimmed background upon a mouse click under certain conditions. r=jaws

Comment 11

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

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

Comment 12

[roxleitan]

Verified fixed using the latest Nightly 52.0a1 (Build ID:20161019030208) on Windows 10 x64 and Mac OS X 10.11 and using Nightly  52.0a1 (Build ID:20161017030209) on Ubuntu 16.04.