279022 - "Highlight all" cannot be undone after switching tabs

Status: REOPENED

Description

[Marcia Knous [:marcia - needinfo? me]]

Found using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b)
Gecko/20050118 Firefox/1.0+. Tracy also has seen this using today's Mac build.

STR:

1. Open a page (we were searching CNN articles) and search for a word using
Find.    Then highlight the term in the page.
2. Open a few tabs and then go back to the page that you have highlighted and
try to unhighlight.
3. Unable to unhighlight any of the terms

Expected behavior should be that you should be able to unhighlight the terms if
you wish.

Comment 1

[Marcia Knous [:marcia - needinfo? me]]

Confirming that the behavior exists in 1.0 and on Windows. Changing HW and OS to
all.

Comment 2

[sairuh (rarely reading bugmail)]

could this be related to bug 273304?

marcia also observed that this occurred in ffox 1.0.

Comment 3

[Mike Connor [:mconnor]]

Hmm.  Using 1.0 I can get this to unhighlight on the second click.  Basically,
highlight is deselected when you switch tabs, but the highlighting is still in
place.  We're not recognizing that tab X has highlighting on, even though the
DOM changes we've made to do the highlight are still active.  Either we need to
remember whether tab X has highlighting on 

I'm not sure I like how highlight is implemented, period.

Comment 4

[Marcia Knous [:marcia - needinfo? me]]

On the Mac using FF 1.0, I can't get the text to unhighlight even when I click
twice. I agree with mconnor - I think we may need to take a look at how this has
been implemented.

Comment 5

[Asa Dotzler [:asa]]

nice to have but not a blocker. 

Comment 6

[Masayuki Nakano [:masayuki] (JST, +0900)]

taking.

Comment 7

[VanillaMozilla]

The summary should really be changed to "Highlight all can not be undone if tab switched".

Bug 315623 is a duplicate of this, but I cannot mark it.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

*** Bug 315623 has been marked as a duplicate of this bug. ***

Comment 9

[Peter Kasting]

This bug should get fixed properly by bug 263683, whenever I get to that.  It would probably be difficult to fix it without implementing that bug.

Comment 10

[Phil Ringnalda (:philor)]

*** Bug 351076 has been marked as a duplicate of this bug. ***

Comment 11

[Ben]

In Firefox 2.0 RC2 I see the same behaviour when changing pages within a tab, for example: clicking a link and going back. I opened bug 356031 for it, but it can very well be a duplicate of this one. Any ideas on it being a duplicate?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

*** Bug 360538 has been marked as a duplicate of this bug. ***

Comment 13

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

*** Bug 360538 has been marked as a duplicate of this bug. ***

Comment 14

[vicky.sun]

This bug can also be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1.1) Gecko/20070227 Firefox/2.0.0.1.

Comment 15

[vicky.sun]

It can not be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a3pre) Gecko/20070302 Minefield/3.0a3pre

Comment 16

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

No, it's still present in 1.9 branch. Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Minefield/3.0a3pre. 

Comment 21

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Bug 263683 doesn't fix this one. The issue is still persistent with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/2008080102 Minefield/3.1a2pre ID:2008080102

Comment 22

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Graeme, is it something you could have a look at? Looks like that Masayuki Nakano doesn't work on it anymore.

Comment 23

[Graeme McCutcheon [:graememcc]]

Henrik, thanks for cc.

So, hooking up the findbar to keep an eye on tab switches should be easy enough. The problem is keeping track of what highlights if any were on what tabs. You could maybe go for some kind of array, but then you have to do lots of shuffling about any time a tab is deleted, added or moved, which doesn't make me a fan of the idea.

A good idea would be to get the SELECTION_FIND, and check the value of its rangeCount property: != 0 means there are highlights in the page. However, with editable elements having their own selection controllers, you would also need to check this for every editable element on the page. The only ways I can see to do that are: 

1) walk the entire DOM tree to find all the editable elements. (Slow)
2) Fix bug 339400. (Complicated)

Comment 24

[Graeme McCutcheon [:graememcc]]

> 1) walk the entire DOM tree to find all the editable elements. (Slow)

Actually, it may not be that bad an idea, really.

> Graeme, is it something you could have a look at?
Yeah, once bug 451286 and bug 451540 make it through review and into the tree, I'll get onto this.

Comment 26

[Graeme McCutcheon [:graememcc]]

Created attachment 343072 [details] [diff] [review]
Patch v1

This patch should fix this for both tab-switching and bfcache.

A few notes:

- deciding whether case-sensitivity was set when the search terms were originally highlighted would be tricky, so it always performs case-insensitive searches for removing highlighting

- when switching tabs to a tab with highlighting, it doesn't overwrite the value in the findbar textbox with the currently highlighted term: I imagine that a common scenario is where a user types a search term, realises they weren't in the tab they wanted to search, and switches to the correct tab and quickly performs the search again. Overwriting the search term when changing tabs would destroy this capability. However, it will insert the term into the text box if it was empty.

Comment 27

[Graeme McCutcheon [:graememcc]]

Created attachment 343076 [details] [diff] [review]
Patch v1.1

Sorry, an unused variable crept in there.

Comment 29

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Asaf, do you have time for a review? Thanks.

Comment 30

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 343076 [details] [diff] [review]
Patch v1.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -3819,21 +3819,18 @@
>     }
>     UpdateBackForwardCommands(gBrowser.webNavigation);
> 
>     if (gFindBar.findMode != gFindBar.FIND_NORMAL) {
>       // Close the Find toolbar if we're in old-style TAF mode
>       gFindBar.close();
>     }
> 
>-    // XXXmano new-findbar, do something useful once it lands.
>-    // Of course, this is especially wrong with bfcache on...
>-
>-    // fix bug 253793 - turn off highlight when page changes
>-    gFindBar.getElement("highlight").checked = false;
>+    // fix bugs 253793 / 279022
>+    gFindBar.onLocationChange();
> 

So I wonder, any chance we could a listener within findbar.xml?

Comment 31

[Graeme McCutcheon [:graememcc]]

> So I wonder, any chance we could a listener within findbar.xml?

It would require two event listeners - we would have to listen for TabSelect, and also implement all the web progress listeners (although we would only have to do any actual work in onLocationChange).

Additionally, as far as the progress listeners are concerned, we would either have to add an all tabs event listener, and ignore onLocationChanges where the browser != the currently selected browser), or add and remove progress listeners to individual browsers on TabSelect.

Versus a one-liner change in browser.js, that while breaking encapsulation slightly, does the right thing...

Comment 32

[Graeme McCutcheon [:graememcc]]

Created attachment 354616 [details] [diff] [review]
Patch v1.2

Meh. That last comment is bogus. Attaching the progress listener to the <tabbrowser> does the right thing.

Comment 33

[Graeme McCutcheon [:graememcc]]

Created attachment 354617 [details] [diff] [review]
Patch v1.2

Forgot to 'hg add' testcase.

Comment 34

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 354617 [details] [diff] [review]
Patch v1.2


>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>-            if (!this.mTabbedMode)
>+            if (!this.mTabbedMode && this.mTabFilters[0])

Explain (better as a comment within the file).

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml
>--- a/toolkit/content/widgets/findbar.xml
>+++ b/toolkit/content/widgets/findbar.xml

>@@ -552,17 +581,20 @@
>           // Fixing bug 339400 would make this go away, and allow the removal case
>           // to be reduced to something like findSelection.removeAllRanges() 
> 
>           var retRange = null;
>           var finder = Components.classes["@mozilla.org/embedcomp/rangefind;1"]
>                                  .createInstance()
>                                  .QueryInterface(Components.interfaces.nsIFind);
> 
>-          finder.caseSensitive = this._shouldBeCaseSensitive(aWord);
>+          if (aHighlight)
>+            finder.caseSensitive = this._shouldBeCaseSensitive(aWord);
>+          else
>+            finder.caseSensitive = false;
> 

I don't understand this at all. Please add a comment.

>           while((retRange = finder.Find(aWord, this._searchRange,
>                                         this._startPt, this._endPt))) {
>             this._highlight(aHighlight, retRange, controller);
>             this._startPt = retRange.endContainer.ownerDocument.createRange();
>             this._startPt.setStart(retRange.endContainer, retRange.endOffset);
>             this._startPt.setEnd(retRange.endContainer, retRange.endOffset);
> 
>@@ -603,16 +635,124 @@
>           var isEditable = this._getEditableNode(node);
>           if (isEditable)
>             controller = isEditable.editor.selectionController;
>           var findSelection = controller.getSelection(this.nsISelectionController.SELECTION_FIND);
>           if (aHighlight)
>             findSelection.addRange(aRange);
>           else if (isEditable)
>             findSelection.removeAllRanges();
>+        ]]></body>
>+      </method>
>+
>+      <!--
>+        - Detects whether highlighting is present in any editable elements
>+        - of type of the given tag name
>+        - @param aDocument
>+        -        the document to check
>+        - @param aTagName
>+        -        the type of editable element to check
>+        - @returns the highlighted string, if there is highlighting present,
>+        -          null otherwise
>+        -->
>+      <method name="_getEditableHighlightString">

How do you handle elements outside the html namespace (foo:input, foo:textare in a XHTML document). I think the right thing is to handle html elemnts only using getElementsByTagNameNS, what do you think? By the way, how is designMode handled?


>+        - Updates the findbar UI on a page change, to allow removal of highlighting
>+        - where appropriate
>+        -->
>+      <method name="_onLocationChange">

I would rather inline it within the listener itself

Comment 36

[Graeme McCutcheon [:graememcc]]

Created attachment 360835 [details] [diff] [review]
Patch v1.3

This patch is intended to apply on top of the new patch in bug 451540. Please review it in that context!

With the patch for bug 451540, we no longer have to search for editable elements - we now know what editors contain highlighting. Thus most of your concerns above simply go away - this bug collapses to simply checking for the existence of find highlights in the document's selection controllers, and checking if any of our cached editors belong to the new document.

Comment 37

[Graeme McCutcheon [:graememcc]]

Created attachment 360904 [details] [diff] [review]
Patch v1.4

Nit-fix based on comments in 451540. Again, this patch is intended to be applied on top of 451540.

Comment 38

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 360904 [details] [diff] [review]
Patch v1.4

r=mano

Comment 39

[Graeme McCutcheon [:graememcc]]

Created attachment 373455 [details] [diff] [review]
For checkin

Comment 40

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/ea00ee9634c4

Comment 41

[Dão Gottwald [::dao]]

backed out

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240055131.1240059712.12201.gz

Comment 42

[Graeme McCutcheon [:graememcc]]

Created attachment 373569 [details] [diff] [review]
Patch v1.5

Ah. Trivial fix here.

The change in this patch is to surround the removeProgressListener call in the _browser property setter with a try/catch.

There was an assumption that if this._browser was non-null then we must have been through the code in the setter that calls addProgressListener. If the findbar has a browserid attribute, as in the failing testcase, this isn't true, which triggers an exception in the setter code, which means the keypress listener registration code never gets reached, breaking quickfind etc etc.

Comment 43

[Graeme McCutcheon [:graememcc]]

http://hg.mozilla.org/mozilla-central/rev/e4cef2105e3e

Comment 44

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Can we mark it as fixed?

Comment 45

[Graeme McCutcheon [:graememcc]]

> Can we mark it as fixed?

No. :(

Backed out: http://hg.mozilla.org/mozilla-central/rev/46c6190c9103

Comment 46

[Shawn Wilsher :sdwilsh]

This bug was implicated in a Txul regression:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eeb44ecd8038&tochange=4480c18255f2
http://graphs-new.mozilla.org/graph.html#tests=[{%22machine%22:51,%22test%22:17,%22branch%22:1},{%22machine%22:52,%22test%22:17,%22branch%22:1},{%22machine%22:53,%22test%22:17,%22branch%22:1},{%22machine%22:54,%22test%22:17,%22branch%22:1},{%22machine%22:73,%22test%22:17,%22branch%22:1}]&sel=1242660540,1242833340

The regression went away with the backout being the only thing in range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=27bda06a2e7d&tochange=46c6190c9103
http://graphs-new.mozilla.org/graph.html#tests=[{%22test%22:%2217%22,%22branch%22:%221%22,%22machine%22:%2251%22},{%22test%22:%2217%22,%22branch%22:%221%22,%22machine%22:%2252%22},{%22test%22:%2217%22,%22branch%22:%221%22,%22machine%22:%2253%22},{%22test%22:%2217%22,%22branch%22:%221%22,%22machine%22:%2254%22},{%22test%22:%2217%22,%22branch%22:%221%22,%22machine%22:%2273%22}]&sel=1242691560,1242864360

Comment 47

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

It looks like this was also responsible for the mochitest-browser-chrome test timing out on the Linux unit test machine (with the same symptoms of what happened randomly in bug 493355).

Comment 48

[Graeme McCutcheon [:graememcc]]

> This bug was implicated in a Txul regression:

I don't think it would be a stretch to blame this on having the findbar become a webProgressListener.

Before I have another pass at this, it would need a module owner decision on whether this warrants taking the Txul hit.

Alternatively, the webProgressListener is only really needed to catch highlighted pages coming out of bfcache. An alternate scheme would be to remove any highlighting from a page when it is being stuffed into bfcache, which would then mean the findbar would only need to listen for TabChange etc events (if the given browser was indeed a <tabbrowser>), and then the findbar could easily keep track of which tabs are highlighted, and adjust it's UI accordingly.

Anyone have any thoughts?