Firefox's native highlighting lost when highlighting by SearchWP is switched on/off (or on switching tabs, or sometimes even when scrolling)

legege / searchwp

Firefox Add-on: Find what you seek faster, using jump-to-word buttons and rich highlighting.

12 stars 1 forks source link

Firefox's native highlighting lost when highlighting by SearchWP is switched on/off (or on switching tabs, or sometimes even when scrolling) #102

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago

searchwp breaks the highlight feature of Firefox.

What steps will reproduce the problem?
1. Press Ctrl-F and search for a word in a website
2. press "Highlight All" in the find bar
3. switch to another tab and back

What is the expected output? What do you see instead?
All Highlights should still be there. instead they are all gone.

What version of the product are you using? On what operating system?
Win7

Original issue reported on code.google.com by nap...@gmail.com on 24 Nov 2014 at 8:04

GoogleCodeExporter commented 9 years ago

This is in fact an issue. It also happens when switching SearchWP's 
highlighting on/off (without switching the tab, switching the tab actually 
triggers re-highlighting internally which is issue 65)

Basically SearchWP's highlighting does not play well with Firefox's native 
highlighting.

Original comment by Eduard.Braun2 on 27 Nov 2014 at 12:48

Changed title: Firefox's native highlighting lost when highlighting by SearchWP is switched on/off (or on switching tabs)
Changed state: Accepted
Added labels: Priority-Low
Removed labels: Priority-Medium

GoogleCodeExporter commented 9 years ago

Original comment by Eduard.Braun2 on 29 Nov 2014 at 5:20

Added labels: Priority-Medium
Removed labels: Priority-Low

GoogleCodeExporter commented 9 years ago

Native highlighting is sometimes even lost when only scrolling in a page.

This was also reported as an AMO review:
'''"I've found that this extension interferes with the highlighting in the find 
bar. To clarify: If you search for a word, then use the highlight function in 
the find bar, the highlighting works until you scroll. Once you start scrolling 
up and down the page, the highlight is gone. I've narrowed the problem to this 
extension by process of elimination (i.e., disabling extensions one by one)."'''

Original comment by Eduard.Braun2 on 2 Feb 2015 at 9:10

Changed title: Firefox's native highlighting lost when highlighting by SearchWP is switched on/off (or on switching tabs, or sometimes even when scrolling)

Quicksaver commented 9 years ago

The cause is a simple one: NodeHighlighter replaces text portions with document fragments containing those same text portions highlighted. This physically removes DOM nodes and adds new ones, and since highlights are technically just special text selections, removing the DOM nodes where these exist also removes the highlights (selections) themselves.

Simplest solution would of course be to just re-do the highlights after SearchWP did its thing.

I'm the developer of FindBar Tweak, and a user of both add-ons first reported this issue to me, which brought me here. Please let me know when you get back to developing the add-on and are interested in fixing this issue, as I would like to ensure compatibility between both add-ons. With the current SearchWP code, it's actually very hard to fix this just on FBT's side, especially since I see you've already committed changes that move the highlighting to the frame script, and I can't touch those from my add-on at all.

Ede123 commented 9 years ago

Hi Quicksaver,

thanks for feedback! Actually this is on the to-do list for next version and I planned to fix it exactly the way you suggest (disabling/re-enabling the native highlighting).

It's not ideal but I wasn't able to come up with a better solution that is compatible with SearchWPs current way of highlighting (modifying the DOM and replacing nodes) while being performant enough.

Regarding compatibility with FindBar Tweak: Let's continue in issue #105

Quicksaver commented 9 years ago

It's not ideal but I wasn't able to come up with a better solution that is compatible with SearchWPs current way of highlighting (modifying the DOM and replacing nodes) while being performant enough.

Agreed, I came to the same conclusion when I checked SearchWP's code, which is why I didn't have any other alternative suggestions. When I work on https://github.com/Quicksaver/FindBar-Tweak/issues/76 (no idea when that will happen though), perhaps I'll be able to help with that. ;)

Quicksaver commented 9 years ago

BTW apparently SearchWP removes Firefox's native highlighting not only when it adds its matches when changing the DOM tree, but also when it doesn't: https://github.com/legege/searchwp/blob/master/src/chrome/content/highlighter/highlighter.js#L59-L64

I guess this can be problematic for heavy dynamic/AJAX pages, that have constant onStateChange's (calling refresh() -> unhighlight() -> etc.). Thought I should mention it, as that means disabling SearchWP's markers through its toolbar button, or even clearing the search bar so it has nothing to mark, won't do much help to keep the native highlights.

Quicksaver commented 9 years ago

Unless I misread something in SearchWP code of course.

Ede123 commented 9 years ago

No, you're completely right. ;-)

This is all on my to-do list and I hope to be able to fix it anytime soon, but it's gonna take some time since this part of SearchWP is a bit of a mess tight now and I'd like to clean it up in the process if possible.

Quicksaver commented 9 years ago

Of course, I only mentioned it to make sure this was the case. :)