Closed Bug 70224 Opened 24 years ago Closed 24 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.
Attached patch forgot to cvs diff content. (deleted) — Splinter Review
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.
Attached patch cleaner patch against tip (deleted) — Splinter Review
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...
Attached patch *** Hallelujah *** (deleted) — Splinter Review
...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.
Attached patch updated patch per danm (deleted) — Splinter Review
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: 24 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: