250409 - window.find() should use Find Toolbar

Status: NEW

(Toolkit :: Find Toolbar, defect, P2)

Description

[Jesse Ruderman]

1. javascript:window.find(); void 0

Result: Find dialog appears.

Expected: Find toolbar should appear.

Note that with some parameters, window.find() doesn't bring up a dialog.  In
those cases, window.find() should stay silent rather than bringing up the toolbar.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

-> Find Toolbar component.

Comment 2

[Neil Paris]

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

Comment 3

[Jeremy Arnold]

Replicated this bug. Old find dialog also fails to function.

Comment 4

[Asaf Romano (gone)]

Attachment: backend part

Comment 5

[Asaf Romano (gone)]

Attachment: browser/ frontend

I will update the xpfe frontend once the backend part is ready.

Comment 6

[Asaf Romano (gone)]

So, is this change OK for embedding apps?

I've tested camino's window.find() behavior (without this patch), apparently it
usess the same implementation (i.e. opens a [pretty broken] xul find dialog).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177309 [details] [diff] [review]
backend part

With that patch, how is the embeddor supposed to know which window wanted the
find?  Did you even test this patch with multiple windows open?  Or with
window.find() being called in a background tab in Firefox?  As far as I can
tell, the results won't be pretty...

Other than that, since this is an embedding api, I would like to see, along
with this patch, clear documentation on exactly what embeddors need to do to
deal with this (what to register for, what sort of calls to expect under what
circumstances, etc.

I'm not sure where said documentation should go in our code... where do we
document frozen observer topics (biesi, darin, bsmedberg?)?  We really need
that anyway, for the startup/shutdown stuff...

Finally, the "front end" change just fixes firefox.  That leaves seamonkey,
camino, thunderbird, the existing embedding apps all broken.... Some of these
never worked, but please fix the things you break if you decide to break them,
ok?

Comment 8

[Asaf Romano (gone)]

(In reply to comment #7)
> (From update of attachment 177309 [details] [diff] [review] [edit])
> With that patch, how is the embeddor supposed to know which window wanted the
> find?  Did you even test this patch with multiple windows open?  Or with
> window.find() being called in a background tab in Firefox?  As far as I can
> tell, the results won't be pretty...

ooh, right. need to rush less :-|

I'm wondering if a gui-event would be a better answer.

> Finally, the "front end" change just fixes firefox.

see comment 5.

Comment 9

[Asaf Romano (gone)]

Also, I'm not sure what is the expected result of window.find() in a background tab.

Comment 10

[Darin Fisher]

At the moment, frozen observer topics are generally documented with service
interface definitions, which IMO is probably not ideal.  See for example
nsIServiceManager.idl.  I think it'd better to document observer topics in a
header file for the given module.

Perhaps instead of nsEmbedCID.h (and the like), we should have nsEmbedAPI.h
where we define various ContractIDs, categories, and observer topics specific to
the module.

The other thing we should do is to document these things on DevMo.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 177311 [details] [diff] [review]
browser/ frontend

for now

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'm wondering if a gui-event would be a better answer.

Or you could pass the window to NotifyObservers()...

> Also, I'm not sure what is the expected result of window.find() in a
> background tab.

"Ask a UI designer"?  ;)  Seriously, it should be whatever the behavior is for a
background window.

> we should have nsEmbedAPI.h

We have that already: 
http://lxr.mozilla.org/seamonkey/source/embedding/base/nsEmbedAPI.h

(Note that we have nsEmbedAPI.cpp too....)

Comment 13

[Darin Fisher]

Yeah, maybe nsEmbedAPI.h could be re-purposed.  At any rate, the idea of a
header file per module that defines all the various contracts "exported" by that
module makes sense to me.  What do you think?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Seems reasonable (I guess this would be the DOM module, then....)

Mano, you could indeed use an event to do this, but see bug 286013.

Comment 15

[Asaf Romano (gone)]

not for 1.8, probably.

Comment 16

[Martijn Wargers (dead)]

Note bug 672395 that wants to kill it.
I just filed bug 725496 for screwing up mobile Fennec regarding this.