Open
Bug 279022
Opened 17 years ago
Updated 5 years ago
"Highlight all" cannot be undone after switching tabs
Categories
(Toolkit :: Find Toolbar, defect, P5)
Toolkit
Find Toolbar
Tracking
()
REOPENED
mozilla1.9.2a1
People
(Reporter: marcia, Assigned: graememcc)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 7 obsolete files)
14.00 KB,
patch
|
mano
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
Confirming that the behavior exists in 1.0 and on Windows. Changing HW and OS to all.
Hardware: Macintosh → All
Comment 2•17 years ago
|
||
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
Comment 3•17 years ago
|
||
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.
Reporter | ||
Comment 4•17 years ago
|
||
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.
Updated•16 years ago
|
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Comment 5•16 years ago
|
||
nice to have but not a blocker.
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
Comment 6•16 years ago
|
||
taking.
Assignee: firefox → masayuki
Target Milestone: --- → Firefox1.6-
Version: 1.0 Branch → Trunk
Comment 7•16 years ago
|
||
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•16 years ago
|
||
*** Bug 315623 has been marked as a duplicate of this bug. ***
Updated•16 years ago
|
Summary: Highlighting and Tabs - Inconsistent Behavior → "Highlight all" cannot be undone after switching tabs
Updated•15 years ago
|
QA Contact: fast.find
Comment 9•15 years ago
|
||
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
Comment 10•15 years ago
|
||
*** Bug 351076 has been marked as a duplicate of this bug. ***
Comment 11•15 years ago
|
||
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•15 years ago
|
||
*** Bug 360538 has been marked as a duplicate of this bug. ***
Comment 13•15 years ago
|
||
*** Bug 360538 has been marked as a duplicate of this bug. ***
Comment 14•14 years ago
|
||
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.
Updated•14 years ago
|
Target Milestone: Firefox 2 → ---
Comment 15•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•13 years ago
|
Product: Firefox → Toolkit
Comment 21•13 years ago
|
||
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•13 years ago
|
||
Graeme, is it something you could have a look at? Looks like that Masayuki Nakano doesn't work on it anymore.
Assignee: masayuki → nobody
Assignee | ||
Comment 23•13 years ago
|
||
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)
Assignee | ||
Comment 24•13 years ago
|
||
> 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.
Assignee | ||
Comment 26•13 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #343072 -
Flags: review?(mano)
Assignee | ||
Comment 27•13 years ago
|
||
Sorry, an unused variable crept in there.
Attachment #343072 -
Attachment is obsolete: true
Attachment #343076 -
Flags: review?(mano)
Attachment #343072 -
Flags: review?(mano)
Comment 29•13 years ago
|
||
Asaf, do you have time for a review? Thanks.
Whiteboard: [has patch][nees review mano]
Updated•13 years ago
|
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?
Assignee | ||
Comment 31•13 years ago
|
||
> 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...
Assignee | ||
Comment 32•13 years ago
|
||
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)
Assignee | ||
Comment 33•13 years ago
|
||
Forgot to 'hg add' testcase.
Attachment #354616 -
Attachment is obsolete: true
Attachment #354617 -
Flags: review?(mano)
Attachment #354616 -
Flags: review?(mano)
Attachment #354617 -
Flags: review?(mano) → review-
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
Updated•12 years ago
|
Whiteboard: [has patch][needs review mano] → [needs new patch]
Assignee | ||
Comment 36•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [needs new patch]
Assignee | ||
Comment 37•12 years ago
|
||
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)
Comment on attachment 360904 [details] [diff] [review] Patch v1.4 r=mano
Attachment #360904 -
Flags: review?(mano) → review+
Assignee | ||
Comment 39•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 40•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ea00ee9634c4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment 41•12 years ago
|
||
backed out http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240055131.1240059712.12201.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 42•12 years ago
|
||
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)
Attachment #373569 -
Flags: review?(mano) → review+
Assignee | ||
Comment 43•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e4cef2105e3e
Comment 44•12 years ago
|
||
Can we mark it as fixed?
Assignee | ||
Comment 45•12 years ago
|
||
> Can we mark it as fixed? No. :( Backed out: http://hg.mozilla.org/mozilla-central/rev/46c6190c9103
Comment 46•12 years ago
|
||
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•12 years ago
|
||
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).
Assignee | ||
Comment 48•12 years ago
|
||
> 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?
Updated•5 years ago
|
Priority: -- → P5
You need to log in
before you can comment on or make changes to this bug.
Description
•