Closed Bug 70224 Opened 23 years ago Closed 23 years ago

need nsIWebBrowserChromeFocus so we can propagate focus info.

Categories

(Core Graveyard :: Embedding: APIs, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: jud, Assigned: dr)

References

()

Details

(Keywords: embed, Whiteboard: for embedding, patch in hand, eta 4/24. r=danm, sr=hyatt. need a=.)

Attachments

(8 files)

here's the proposed idl from the notes url:


proposed idl
// ALT-TAB waspressedand focus should leave mozilla.

        void setFocusAtPreviousElement();
// TAB was pressedandfocus should leave mozilla

        void setFocusAtNextElement();
Blocks: 70229
Summary: [API] need nsIWebBrowserChromeFocus so we can propagate focus info. → need nsIWebBrowserChromeFocus so we can propagate focus info.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
p1
Priority: -- → P1
Why isn't this just part of nsIWebBrowserFocus?

Is Alt-tab intentional? Shift-Tab and Tab are usually back and forward 
respectively...
not sure. I think either you or blizzard suggested this?
This patch seems to be in the wrong bug.? This patch fills out
nsIWebBrowserFocus, not nsIWebBrowserChromeFocus.
Yeah, well, I still don't know why nsIWebBrowserChromeFocus exists (I was only
at the nsIWebBrowserFocus meeting) and since they seem to have overlapping
functionality I thought I'd post it here.

Honestly, I don't even know how nsIWebBrowserChromeFocus is supposed to differ.
Someone vend me a clue please.
->dr to get review/approval and checkin patch.
Assignee: saari → dr
Status: ASSIGNED → NEW
Okay, current understanding is that this is meant to be the callback to notify 
an embedding application that they should recieve focus from a tab or shift tab 
exiting the mozilla client. Correct?

Bryner wonders if that doesn't overlap with nsIDocshellTreeOwner functionality
saari, you are corrrect.  What does it have to do with the docshell tree owner?
More clarification, docshelltreeowner implements nsIBaseWindow::FocusAvailable
Bryner thought that might be a more elegant place to get the focus call outs, 
although we'd have to add a tabbing direction to it.

Do embeddors have to implement focus available or will there just be no 
docshelltree owner in the embedded case? Just trying to figure out where to 
give up and call out.
mozilla's docshelltreeowner (nsDocShellTreeOwner.cpp) will indeed be there in
the embedded case, but the embeddor's equiv to nsIBaseWindow is
nsIEmbeddingSiteWindow which does not have a :FocusAvailable().

If I'm understanding your question correctly, there will not be a
:FocusAvailable() on the embeddor side of things (though, of course, our
internal implementation of it is there).

bliz, did I botch anything in my above description?
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
Valeski, you are correct.  We need this additional interface to pass the
information back up to the embeddor.
so will the embedding object that implements nsIEmbeddingSiteWindow also be 
implementing this new nsIWebBrowserChromeFocus api, or do I need to get a 
different object from somewhere else to call these APIs on?

That is correct.
bonzer, patch coming right up
Status: NEW → ASSIGNED
working on it...

i'll only have gtkEmbed's WebBrowserChrome implement nsIWebBrowserChromeFocus
for the moment, for immediate testing and checkin asap. the other test harnesses
should follow as an "exercise for the reader"... i'll do those after 0.9 settles
down.
Keywords: embed
OS: Linux → All
Hardware: PC → All
Whiteboard: [ should be ready by thurs. 4/19 ]
bryner, saari:

Have a look at the patch attached (the two attachments). I've set up gtkEmbed to
implement the new interface -- you might want to play around with a different
embedding test harness (just implement those two methods on the same object that
implements nsIEmbeddingSiteWindow).

This patch is ill-behaved for me. I have it dump "embedded" or "not embedded" in
the FocusAvailable method on DocShell. Pages with focusable form controls dump
"embedded" if you try to tab out of them, and pages without dump "not embedded".
This is strange because the logic there (with the QIs failing and dumping into
my code) seems to imply that there's a difference between the DocShellTreeOwner
having focus and the BaseWindow having it... I'm baffled.
well, the object that implements docshell tree owner doesn't implement 
nsIEmbeddingSiteWindow in the mfcEmbed case. So any ideas about where I can get 
to the object implementing nsIEmbeddingSiteWindow/nsIWebBrowserChromeFocus?

we know where and when to call the api, just not sure where to get to the 
object to call it on.
saari: If my code is Just Plain Wrong, I'd be more inclined to get this off our
0.9 radar and wait a couple days to decompress than trying to check an
"approximation" of the right thing into the trunk. Then we can figure out what
the Right Thing to do is, and I'll deal with checking into the branch...

Let me know what you're thinking.
Whiteboard: [ should be ready by thurs. 4/19 ] → [ should be ready "real soon now" ]
I don't see how waiting is going to get this done any sooner. Also, please put a
real date in status whiteboard, RSN is not helpful. cc self
Whiteboard: [ should be ready "real soon now" ] → Wanted for 0.9 embedding
saari, bryner: This is my best shot, but it still doesn't work, for reasons
mysterious to me. It seems as if nsDocShellTreeOwner::GetInterface isn't even
getting called, despite my obvious call to do_GetInterface in
nsDocShell::FocusAvailable.

If you trace through nsDocShellTreeOwner.cpp, notice the usage of mOwnerWin
(which is, I believe, what we're trying to get at) and of mOwnerRequestor.
They're both the same object. So the logic of GetInterface seems quirky to me.

Cc'ing adamlock -- I'm betting he knows this code best...
Are you sure the object you're calling it on is actually an instance of
nsDocShellTreeOwner and not something else that implements the interface?
  What Brian said. And also, though it's hard to tell by just eyeballing the 
code, I'd say you're missing one more piece of the implementation of the new 
interface. See WebBrowserChrome.cpp, around line 60. That's the part that hooks 
up QueryInterface.
  I'd also like to talk sometime about the "are we embedded" code. I'm not 
completely familiar with the problem, but I think you shouldn't need to care.
danm: we know you shouldn't need to care. we had a great little deathmatch
friday on irc with bryner, valeski, saari and me...

re: not being the implementation i think i am, i was fairly convinced
nsDocShellTreeOwner was the only implementor of nsIEmbeddingSiteWindow. i
suppose not, though, given that nsDocShellTreeOwner::GetInterface never gets
called. hmph.
So here's a slightly cleaner patch against the tip. Strangely,
nsDocShellTreeOwner::GetInterface seems to get called now (I didn't change
anything). It still doesn't work, but I have an idea why...
...and twenty minutes later, after understanding what danm wrote, our hero posts
a patch that *actually works*. looking for r=, sr= now.
Keywords: approval, review
Whiteboard: Wanted for 0.9 embedding → Wanted for 0.9 embedding, ETA: Tue 24 Apr, at the latest.
dan, how recent was your tree? I keep getting errors when patching
saari: probably more up-to-date than yours. i've been perpetually conflicting
with bryner or somebody, so i keep having to update to tip. i'll update right
now, regardless...
Whiteboard: Wanted for 0.9 embedding, ETA: Tue 24 Apr, at the latest. → for embedding, patch in hand, eta 4/24. need r, sr, a.
Latest patch looks good. r=danm after the modifications we've discussed, those 
being a ditching of the debug printf noise and empty implementations of 
nsIWebBrowserChromeFocus in PowerPlant and winEmbed.
Whiteboard: for embedding, patch in hand, eta 4/24. need r, sr, a. → for embedding, patch in hand, eta 4/24. r=danm. need sr, a.
sr=hyatt
Whiteboard: for embedding, patch in hand, eta 4/24. r=danm. need sr, a. → for embedding, patch in hand, eta 4/24. r=danm, sr=hyatt. need a=.
a= asa@mozilla.org (on behalf of drivers) for checkin to 0.9
fix checked in (danm checked in .mcp update)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
nsIWebBrowserChromeFocus.idl exists with 2 methods: focusNextElement() and 
focusPrevElement()
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: