Find dialog needs to be posed from JS not C++

VERIFIED FIXED in mozilla0.9.1

Status

SeaMonkey
UI Design
P2
normal
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: Conrad Carlen (not reading bugmail), Assigned: Simon Fraser)

Tracking

Trunk
mozilla0.9.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments)

9.64 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
6.63 KB, patch
Details | Diff | Splinter Review
5.07 KB, patch
Details | Diff | Splinter Review
1.69 KB, text/plain
Details
2.28 KB, text/plain
Details
5.78 KB, text/plain
Details
810 bytes, text/plain
Details
1.33 KB, text/plain
Details
1.77 KB, text/plain
Details
3.42 KB, text/plain
Details
821 bytes, patch
Details | Diff | Splinter Review
768 bytes, patch
Details | Diff | Splinter Review
4.04 KB, text/plain
Details
(Reporter)

Description

17 years ago
The Find component pops a XUL dialog box from C++ code when asked to find. This
is inside-out. I had to add a new API for text searching when embedding in order
to get away from this code<->UI coupling. The find dialog should be posed from
JS, get a scriptable find API, and call its methods. Actually, a find iface
should just be available from an nsIDomWindow. This would allow for better
sharing of code between embedding apps and seamonkey.
(Assignee)

Comment 1

17 years ago
Keep law in the loop

Comment 2

17 years ago
The problem with opening the dialog from JS is that it is almost certain to be
the client code that does so, which thus limits the implementation choices for
this component.

Consider the consequences of applying this same rationale to the file picker,
for example.  If every client of that service were to code
    window.openDialog( "chrome://global/content/filePicker.xul" );
then the ability to implement nsIFilePicker::Show via a native platform dialog
would be lost.

If all find clients code
    window.openDialog( "chrome://global/content/fileDialog.xul" );
then we impose a similar limitation.  It also makes it more difficult to use an
alternative find dialog, even one implemented in xul/js.

That's why the find dialog was structured the way it is (like nsIFilePicker).
We use xpcom to seperate the implementation of the dialog from the clients.
That is standard Mozilla practice, I believe (to separate components and
communicate between them using xpcom interfaces), even if it is only honored in
the breach in the context of much of our front-end code.

One of the consequence of such use of xpcom, of course, is the ability to
implement interfaces in one's choice of language.  The "find component" happens
to be implemented in C++ (because it was the only option at the time).  Another
consequence of using xpcom is that one can modify the implementation of a
component without impact on, or knowledge of, the clients that use the
component.  I think nsIFindComponent has succeeded in that, too.   Despite
whatever churn has occurred in that dialog and its implementation, clients have
been oblivious to it.  That's a good thing, not a bad one.

Given all that, I'm not sure why the current situation is such a cause for concern.

Which isn't to say that nsIFindComponent doesn't have problems.  But I think
those should be expressed in terms of the interface and its methods, not the
choice of implementation language, or limitations of that implementation.

>I had to add a new API for text searching when embedding in order to get away
>from this code<->UI coupling.

I'm not sure why a new API was necessary.  This is relatively simple to do with
nsIFindComponent as it stands (just call FindNext rather than Find).  The only
difficulty might be that there would still be the (unreachable) code in there
that called nsIDomWindowInternal::OpenDialog (with a chrome URL).  But I suspect
that could have been remedied by tweaking the implementation that was there
rather than by adding a whole new interface.

>The find dialog should be posed from
>JS, get a scriptable find API, and call its methods.

Isn't that how it works already?  You just "pose" it by calling
nsIFindComponent::Find (instead of by using window.openDialog).

>Actually, a find iface
>should just be available from an nsIDomWindow.

I interpret this as saying that one should QI the nsIDOMWindow to some
"nsIFindThingy" interface?  I'm not sure I agree with that.  What happens if
nsIDOMWindow's implementation sucks?  If there is a separate "find" service,
then it can more easily be replaced.  That service could surely still be part of
the "embeddable" Gecko browser.

nsIDOMWindow should already have some sort of "find" method, since that's part
of DOM Level 0.  I'm not sure what would be needed beyond that.  It could
implement that by using the nsIFindComponent service, theoretically.

Comment 3

17 years ago
This should go to law@netscape.com, he owns the xpfe find component.
Assignee: kin → law

Updated

17 years ago
Blocks: 70771
(Assignee)

Comment 4

17 years ago
Mine.
Assignee: law → sfraser
(Assignee)

Updated

17 years ago
Blocks: 63241
(Assignee)

Comment 5

17 years ago
OK, lots of changes coming here. I'm using this bug to move browser's find 
implementation to use the same impl. as embedding (nsIWebBrowserFind), so the 
changes are in two parts:
1. Make nsIWebBrowserFind available in the non-embedding case (by a GetInterface
   on an nsIDocShell).

2. Eliminate Find methods on nsBrowserInstance, and rewrite the navigator
   and find dialog JS to use nsIWebBrowserFind.
No longer blocks: 63241
(Assignee)

Comment 6

17 years ago
Created attachment 31373 [details] [diff] [review]
Wean nsWebBrowser of its own implementation of nsIWebBrowserFind
(Assignee)

Comment 7

17 years ago
Created attachment 31374 [details] [diff] [review]
Make nsDocShell support GetInterface of nsIWebBrowserFind
(Assignee)

Comment 8

17 years ago
Created attachment 31375 [details] [diff] [review]
Fix find dialog js to use nsIWebBrowserFind, and start to obselete C++ find component
(Assignee)

Comment 9

17 years ago
Created attachment 31376 [details] [diff] [review]
remove find stuff from nsBrowserInstance, and change JS for find and findAgain
(Assignee)

Comment 10

17 years ago
Created attachment 31377 [details]
New file mozilla/embedding/components/find/public/nsIWebBrowserFind.idl
(Assignee)

Comment 11

17 years ago
Created attachment 31378 [details]
New file: mozilla/embedding/components/find/src/nsWebBrowserFind.h
(Assignee)

Comment 12

17 years ago
Created attachment 31379 [details]
New file: mozilla/embedding/components/find/src/nsWebBrowserFind.cpp
(Assignee)

Comment 13

17 years ago
Created attachment 31381 [details]
New file: mozilla/xpfe/components/find/src/nsFindModule.cpp
(Assignee)

Comment 14

17 years ago
Created attachment 31382 [details]
New file: mozilla/xpfe/components/find/public/nsIFindService.idl
(Assignee)

Comment 15

17 years ago
Created attachment 31383 [details]
New file: mozilla/xpfe/components/find/src/nsFindService.h
(Assignee)

Comment 16

17 years ago
Created attachment 31384 [details]
New file: mozilla/xpfe/components/find/src/nsFindService.cpp
(Assignee)

Comment 17

17 years ago
Here's the deal with the old find component:
* I just moved the module stuff into a new file, nsFindModule.cpp
* I added a new nsFindService, whose *only* task is to act as an application-wide 
storage for global find parameters. The Find dialog gets and sets the values in 
this service.
* The stuff in nsFindCompnent.cpp will eventually go away, but it has to stick 
around until we implement the Replace stuff for editor (which uses its own 
dialog). After that, the nsFindService will be the only thing left in this DLL, 
so maybe it should be folded into another library.

Here's what happened to nsWebBrowser:
* nsWebBrowser used to actually implemenet nsIWebBrowserFind, and the 
GetInterface would turn around and just do a QI. In order to share the 
nsWebBrowserFind implementation, I moved it into embedding/components, and made 
it so that you can CreateInstance one. Now, both nsWebBrowser and nsDocShell do 
this when someone does a GetInterface for nsIWebBrowserChrome.
(Assignee)

Comment 18

17 years ago
Created attachment 31385 [details] [diff] [review]
xulBindings change to expose nsIWebBrowserFind on <browser>
(Assignee)

Comment 19

17 years ago
OK, apart from Makefile magic, that's pretty much it.

Reviews?
(Assignee)

Comment 20

17 years ago
Created attachment 31387 [details] [diff] [review]
same change in browserBindings.xml
(Reporter)

Comment 21

17 years ago
From nsIWebBrowserFind.idl - when does init() have to be called? Only before the
first call to findNext or whenever you want it to search some specific frame.
That method is stuck in the file below the comment for findNext and could use
its own comment. Otherwise, looks good.
(Reporter)

Comment 22

17 years ago
Since bug 63421 has been rolled into this one, WRT searching multiple frames...
I think searching through multiple frames is a UI preference and I'm not sure
what the convention is. In any split-pane editor I've used, searching happens
only in the active pane. Not exactly the same situation but similar. This is the
way the nsIWebBrowserFind impl works now. It uses the focused frame. If you want
to search in the next frame, just click in it. I think the real problem is that
we don't have visual indication which subframe is focused. If we had that, it
would make a lot more sense. 

Comment 23

17 years ago
this is awesome!
As for searching multiple frames, frankly I think at the very minimum it should
be the default for the browser (as opposed to embedding), as some web pages
don't even make it clear that there ARE multiple frames (i.e. via tricks with
borderless frames & cool backgrounds, etc)
I'll sr= later today if I can.
(Assignee)

Comment 24

17 years ago
I have not implemented the seaching of multiple frames yet, but the code will 
live down in nsWebBrowserFind. Note that I added an nsIDOMWindow param to Init(), 
and an out nsIDOMWindow to FindNext.

Thinking further, I think maybe I need a setter for the window to search, and the 
searchSubframes param, or maybe FindNext should look like this:

  boolean FindNext(in nsIDOMWindow inSearchFrame, in boolean inSearchSubframes, 
out nsIDOMWindow outFoundFrame);
Status: NEW → ASSIGNED
(Assignee)

Comment 25

17 years ago
I'm going to change nsIWebBrowserFind a little to allow control over searching 
subframes, so don't review just yet.
(Reporter)

Comment 26

17 years ago
Couldn't nsIWebBrowserFind's impl keep track of the DOMWindows instead of having
both in and out DOMWindow params? How about this:

const unsigned long SEARCH_FROM_FIRST_FRAME;
const unsigned long SEARCH_FROM_CURRENT_FRAME;

and then FindNext is just
 boolean FindNext(in boolean inSearchSubframes, in unsigned long searchFromFrame);

(Assignee)

Comment 27

17 years ago
Created attachment 31489 [details]
New nsIWebBrowserFind.idl
(Assignee)

Comment 28

17 years ago
I attached a new version of nsIWebBrowserFind.idl for your delectation.

This preserves the original nsIWebBrowserFind as it was, and that interface will 
behave as it used to (don't search subframes, search the focussed or content area 
frame).

To control subframe searching, you will be able to QI nsIWebBrowserFind to 
nsIWebBrowserFindInFrames, and set the various frame searching attributes on 
that. Its attributes allow you to search in subframes, specify a frame to contain 
the search withing, and get/set the current search frame.

How does that look to everyone?
(Reporter)

Comment 29

17 years ago
Beautiful! Thanks for the additional comments too.
(Assignee)

Comment 30

17 years ago
0.9.1
Target Milestone: --- → mozilla0.9.1
(Assignee)

Updated

17 years ago
Blocks: 63241
(Assignee)

Comment 31

17 years ago
See bug 63241 for newer versions of nsWebBrowserFind.cpp/h, which support 
searching in multiple frames.
(Assignee)

Updated

17 years ago
Priority: -- → P2
(Assignee)

Comment 32

17 years ago
cc adam for docShell reviews.

Comment 33

17 years ago
Simon, is it possible to have one find service per docshelltree or does it have 
to be one per docshell?

If the latter then r=adamlock@netscape.com
(Assignee)

Comment 34

17 years ago
That's a good question. I think, to provide embedders and mozilla with maximum 
flexibility, we need a find instance per content area (and a XUL window may have 
> 1 content area under the same chrome docShell hierarchy). Since we don't (I believe) have class that is instantiated just for 'content root' docShells, then the find instance has to hang of of nsDocShell.
(Reporter)

Comment 35

17 years ago
There is the docshell tree owner for which there is one per docshell tree. It's
implemented in nsDocShellTreeOwner.cpp for embedding and nsContentTreeOwner.cpp
for seamonkey. Now, if only these two implementations could be unified (maybe
part of bug 76585?).
Blocks: 65257

Comment 36

17 years ago
bill (understandably) wants a reply to the issues he brought up early in this bug.

- as for why using an API that is proprietary to Find instead of using
window.openDialog, ask yourself "why does Find need to be any different than any
other dialog posed from seamonkey" - the answer is, "it shouldn't be any different"
  Why not? You described the file picker as a point of comparison. That is an
irrelevant example because the file picker is one of the rare cases where
mozilla allows the user to interact with the native system underneath it. It is
an exception, not a rule.
  The find dialog interacts only with mozilla, therefore it should look and
behave like other mozilla dialogs. There is no value in making it pluggable.
Just like the preferences window, we're not expecting people to replace the
prefwindow with native code, in seamonkey. 

- as for language choice and abstraction: There are two issues here, and I think
nsIFindComponent blurs them together:
  - The presentation of the find functionality in seamonkey. This is what needs
to be immediately accessable from JavaScript - and it should be done in the same
method as every other dialog in the application: via window.openDialog. This
ensures proper parenting, and with a single control path for opening windows,
mainetance for issues like modality, focus, etc, becomes standard across all
windows. 
  - The backend functionality: This is a completely seperate issue, which is
what embedding is basically trying to break out - we need presentation-free APIs
to tell a content area to "find" other content, maintain state about the
"current" content, and so forth.

- as for QI'ing nsIDOMWindow to find, I don't think that's what simon meant
- as for implementing DOM stuff with nsIFindComponent, again we need a
presentation-free method of doing this so that we can share it between embedding
and seamonkey. the embedding team already decided on interfaces - it's a shame
bill wasn't involved, but for what it's worth I think they had the right idea
(and Simon's polishing seems to be doing the right thing as well)

(Assignee)

Comment 37

17 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
rubberstamp
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.