Open Bug 279022 Opened 17 years ago Updated 6 years ago

"Highlight all" cannot be undone after switching tabs


(Toolkit :: Find Toolbar, defect, P5)






(Reporter: marcia, Assigned: graememcc)


(Depends on 1 open bug)



(1 file, 7 obsolete files)

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.


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.
Confirming that the behavior exists in 1.0 and on Windows. Changing HW and OS to
Hardware: Macintosh → All
could this be related to bug 273304?

marcia also observed that this occurred in ffox 1.0.
Flags: blocking-aviary1.1?
Version: Trunk → 1.0 Branch
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.
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.
Flags: blocking-aviary1.1? → blocking-aviary1.1+
nice to have but not a blocker. 
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
Assignee: firefox → masayuki
Target Milestone: --- → Firefox1.6-
Version: 1.0 Branch → Trunk
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.
*** Bug 315623 has been marked as a duplicate of this bug. ***
Summary: Highlighting and Tabs - Inconsistent Behavior → "Highlight all" cannot be undone after switching tabs
QA Contact: fast.find
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.
Depends on: 263683
*** Bug 351076 has been marked as a duplicate of this bug. ***
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?
*** Bug 360538 has been marked as a duplicate of this bug. ***
*** Bug 360538 has been marked as a duplicate of this bug. ***
This bug can also be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv: Gecko/20070227 Firefox/
Target Milestone: Firefox 2 → ---
It can not be reproduced on Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a3pre) Gecko/20070302 Minefield/3.0a3pre
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. 
Duplicate of this bug: 375076
Duplicate of this bug: 344622
Duplicate of this bug: 441809
Duplicate of this bug: 440006
Product: Firefox → Toolkit
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
Graeme, is it something you could have a look at? Looks like that Masayuki Nakano doesn't work on it anymore.
Assignee: masayuki → nobody
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)
> 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.
Duplicate of this bug: 430502
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → graememcc_firefox
Attachment #343072 - Flags: review?(mano)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Sorry, an unused variable crept in there.
Attachment #343072 - Attachment is obsolete: true
Attachment #343076 - Flags: review?(mano)
Attachment #343072 - Flags: review?(mano)
Duplicate of this bug: 436334
Asaf, do you have time for a review? Thanks.
Whiteboard: [has patch][nees review mano]
Whiteboard: [has patch][nees review mano] → [has patch][needs review mano]
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?
> 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...
Attached patch Patch v1.2 (obsolete) — Splinter Review
Meh. That last comment is bogus. Attaching the progress listener to the <tabbrowser> does the right thing.
Attachment #343076 - Attachment is obsolete: true
Attachment #354616 - Flags: review?(mano)
Attachment #343076 - Flags: review?(mano)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Forgot to 'hg add' testcase.
Attachment #354616 - Attachment is obsolete: true
Attachment #354617 - Flags: review?(mano)
Attachment #354616 - Flags: review?(mano)
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[";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
Duplicate of this bug: 471497
Whiteboard: [has patch][needs review mano] → [needs new patch]
Attached patch Patch v1.3 (obsolete) — Splinter Review
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.
Attachment #354617 - Attachment is obsolete: true
Attachment #360835 - Flags: review?(mano)
Whiteboard: [needs new patch]
Attached patch Patch v1.4 (obsolete) — Splinter Review
Nit-fix based on comments in 451540. Again, this patch is intended to be applied on top of 451540.
Attachment #360835 - Attachment is obsolete: true
Attachment #360904 - Flags: review?(mano)
Attachment #360835 - Flags: review?(mano)
Attached patch For checkin (obsolete) — Splinter Review
Keywords: checkin-needed
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
backed out
Resolution: FIXED → ---
Attached patch Patch v1.5Splinter Review
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.
Attachment #360904 - Attachment is obsolete: true
Attachment #373455 - Attachment is obsolete: true
Attachment #373569 - Flags: review?(mano)
Can we mark it as fixed?
> Can we mark it as fixed?

No. :(

Backed out:
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).
Depends on: 493892
> 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?
Depends on: 547218
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.