Closed Bug 793275 Opened 8 years ago Closed 8 years ago

Create Find Events and Allow Find Override

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(1 file, 2 obsolete files)

Create a way for pdf.js to intercept actions from the findbar and use it's own find handle the actions.
Attached patch findbar events v1 (obsolete) — Splinter Review
- Trigger four new find events and optionally allows the event handler to prevent typeaheadfind to be used.  
- Adds a new find state "pending" which shows the spinner (find can be slow in large pdfs). 
- Adds a new public function to update the find bar control status.
Blocks: 748936
Comment on attachment 663513 [details] [diff] [review]
findbar events v1

Looking for initial feedback if this is an acceptable solution.  We have this mostly working with pdf.js, but it hasn't landed in MC yet.
Attachment #663513 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 663513 [details] [diff] [review]
findbar events v1

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

On the whole I think I'm ok with this approach. Makes it easy for anyone to hook in without too much API overhead. You might find it easier to do a browser-chrome test but if you're more comfortable with mochitests then that's fine. Let me know when you're ready for a more nitty review.

::: toolkit/components/typeaheadfind/nsITypeAheadFind.idl
@@ +16,5 @@
>  
>  
>  /****************************** nsTypeAheadFind ******************************/
>  
> +[scriptable, uuid(88dbc674-172c-4aff-af50-e2d903c152b2)]

You don't need to change the IID for just adding a new constant.

@@ +76,5 @@
>                                          // Unsuccessful find
>    const unsigned short FIND_WRAPPED  = 2;
>                                          // Successful find, but wrapped around
> +  const unsigned short FIND_PENDING  = 3;
> +                                        // Unknow status, find has not finished

Bad spelling :)
Attachment #663513 - Flags: feedback?(dtownsend+bugmail) → feedback+
Attached patch findbar events v2 (obsolete) — Splinter Review
Fixed the two above notes.  I also removed code that automatically put it in to the pending state when find was started since with that it was possible to get the findbar in a bad state if the find event handler synchronously updated the state itself. Now its the responsibility of the find event handler to set it to the pending state if needed.

> You might find it easier to do a
> browser-chrome test but if you're more comfortable with mochitests then
> that's fine.
I'm fine with doing either, I just used mochitests since that's what the other findbar testing code used.
Attachment #663513 - Attachment is obsolete: true
Attachment #668311 - Flags: review?(dtownsend+bugmail)
Comment on attachment 668311 [details] [diff] [review]
findbar events v2

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

Please add to the test checks that the findbar shows the pending state after dispatching the events.

::: toolkit/content/tests/chrome/findbar_events_window.xul
@@ +68,5 @@
> +      nextTest();
> +    }
> +
> +    function checkSelection(done) {
> +      setTimeout(function() {

Please use SimpleTest.executeSoon instead of setTimeout.

@@ +128,5 @@
> +        eventTriggered = true;
> +        ok(e.detail.highlightAll, "find event should have highlight all set");
> +        e.preventDefault();
> +        // Since we're preventing the default make sure nothing was highlighted.
> +        setTimeout(function() {

SimpleTest.executeSoon

@@ +133,5 @@
> +          var controller = gFindBar.browser.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                   .getInterface(Ci.nsISelectionDisplay)
> +                                   .QueryInterface(Ci.nsISelectionController);
> +          var selection = controller.getSelection(controller.SELECTION_FIND);
> +          ok(selection.rangeCount == 0, "No text is highlighted");

For thoroughness it might be good to just move this check into checkSelection, it should never be true for any of the tests right?

::: toolkit/content/widgets/findbar.xml
@@ +891,5 @@
>          <body><![CDATA[
> +          var result  = this._dispatchFindEvent("highlightallchange");
> +          if (!result) {
> +            return;
> +          }

Prevailing style in this file is not to brace single statements like this.

@@ +1521,5 @@
>          <body><![CDATA[
> +          var result  = this._dispatchFindEvent("");
> +          if (!result) {
> +            return this.nsITypeAheadFind.FIND_PENDING;
> +          }

As above

@@ +1778,5 @@
>  
> +          var result  = this._dispatchFindEvent("again", aFindPrevious);
> +          if (!result) {
> +            return this.nsITypeAheadFind.FIND_PENDING;
> +          }

I think you should instead dispatch this from _findAgain otherwise in some cases you may get two events from one attempt to find ("findagain" followed by "find"). As this stands right now you're also not updating the UI to be pending on an attempt to find again, returning a status from here doesn't seem to be useful.
Attachment #668311 - Flags: review?(dtownsend+bugmail) → review-
Fixed all the above comments, except for what we discussed on IRC.  I also added a comment for the reason why dispatch findAgain where we do.
Attachment #668311 - Attachment is obsolete: true
Attachment #669613 - Flags: review?(dtownsend+bugmail)
Attachment #669613 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/2af93a18b6b1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
See Also: → 952952
You need to log in before you can comment on or make changes to this bug.