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

This feature will be disabled in 51.

Tracking 52+ for this dimmed highlight feature.

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.

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

Pushed by mdeboer@mozilla.com: 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

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.