Closed Bug 916534 Opened 6 years ago Closed 6 years ago

Find with highlights not working in frames after bug 666816

Categories

(Toolkit :: Find Toolbar, defect)

26 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: quicksaver, Assigned: evilpie)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

I have a question, the new Finder._highlight() method calls itself for frames, and just like before it seems to expect a return value (I don't know how to properly enclose code bits in comments, sorry):

    for (let i = 0; win.frames && i < win.frames.length; i++) {
      if (this._highlight(aHighlight, aWord, win.frames[i]))
        result = Ci.nsITypeAheadFind.FIND_FOUND;
    }

However there is only one case defined when there actually is a return value, which is when there is a selection controller and aHighlight is not true, in this case it returns true. When there isn't a controller it just returns (I'm assuming it just returns (I'm guessing this is the equivalent of "return undefined;" so that should work, but in all other cases it doesn't even return, just notifies.

My question is, is that as it is supposed to be? Shouldn't it only notify the listeners on the base level _highlight() call only and not on every frame subcall? And shouldn't it always return a found value?

If it returns undefined by default, and the listeners take care of the rest of the work, then it should still work. I don't think it is working though, as if you turn on the highlights here (frameset example) http://www.quackit.com/html/templates/frames/frames_example_4.html it will report "Phrase not found" always, even though the search exists and the highlights are placed.
The second paragraph was badly written, here it is reviewed:

However there is only one case defined when there actually is a return value, which is when there is a selection controller and aHighlight is not true; in this case it returns true. When there isn't a controller it just returns. I'm guessing this is the equivalent of "return undefined;" so that should work in theory, but in all other cases it doesn't even return, just notifies the listeners.
Thanks for reporting this issue, I will upload the patch tomorrow.
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
Attached patch highlight-fixSplinter Review
Thanks again for spotting this! The new code now almost matches the old code, plus some additional comment. Basically we only return true when we are removing the highlight, because for the UI this has the meaning of success. And it's kind of the same as trying to search for an empty string. Luis noticed that there was some unused function in findbar.xml, so I removed that as well.
Assignee: nobody → evilpies
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #805279 - Flags: review?(mdeboer)
Tom, I will review your patch asap (not feeling too well today), but I can already say you'll get additional brownie points if you add a unit test to catch future regressions. Do you have time to do that?
Flags: needinfo?(evilpies)
Attached patch really simple test (obsolete) — Splinter Review
Attachment #805377 - Flags: review?(mdeboer)
Flags: needinfo?(evilpies)
Comment on attachment 805279 [details] [diff] [review]
highlight-fix

Review of attachment 805279 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Finder.jsm
@@ +248,1 @@
>        return true;

I don't think explicitly returning here is not really necessary. I think you can just say `found = true` and let the value returning happen in the statement below.

I also think you can restrict this comment to `Removing the highlighting always succeeds, so return true.`
Attachment #805279 - Flags: review?(mdeboer) → review+
Comment on attachment 805377 [details] [diff] [review]
really simple test

Review of attachment 805377 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that this is a very simple test, but not exactly what I had in mind (my fault, I prolly didn't explain it well enough!):
The problem is that no unit test caught this failure. This means that there is no unit test that covers find with Highlight All in a page with frames. I meant to suggest adding a unit test that will catch this regression when it happens again.

Something else: you might want to check your default editor settings, as it seems to be set on 4-space indentation by default.
Attachment #805377 - Flags: review?(mdeboer) → feedback+
You might want to look at this again, there is an iframe with text content. I am pretty sure this test would have failed before, because we would get NOTFOUND.
Sorry to interrupt the discussion, just something I noticed (nit); I think it's unnecessary to explicitly pass "null" and "false" as arguments to _highlight() and _notify() respectively, as they will default to that state. Please give me a heads up if I'm wrong (and yourself as there are several calls to _notify() without those arguments explicitly set throughout the code).
Well it's not wrong, but I prefer passing them explicitly.
Comment on attachment 805377 [details] [diff] [review]
really simple test

By Jeeves, you're right! If you re-upload this patch with the correct indentation settings... r=me.
Attachment #805377 - Flags: feedback+ → review-
Attached patch highlight-testSplinter Review
Attachment #805377 - Attachment is obsolete: true
Attachment #805973 - Flags: review?(mdeboer)
Comment on attachment 805973 [details] [diff] [review]
highlight-test

Review of attachment 805973 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/browser/browser_Finder.js
@@ +8,5 @@
> +
> +function test () {
> +  waitForExplicitFinish();
> +
> +  tab = gBrowser.addTab("data:text/html,<iframe srcdoc='content' />")

nit: space not needed and missing semicolon: `'content'/>");`
Attachment #805973 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/e9e4af1645b5
https://hg.mozilla.org/mozilla-central/rev/9ebe2f2a6cc3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 805279 [details] [diff] [review]
highlight-fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: Findbar matches might not be indicated
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): not very risky, this code is now actually more similar to what we had before
String or IDL/UUID changes made by this patch: none
Attachment #805279 - Flags: approval-mozilla-aurora?
Attachment #805279 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.