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)
Core Graveyard
Embedding: APIs
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)
3.85 KB,
patch
|
Details | Diff | Splinter Review | |
13.48 KB,
patch
|
Details | Diff | Splinter Review | |
1.46 KB,
patch
|
Details | Diff | Splinter Review | |
768 bytes,
patch
|
Details | Diff | Splinter Review | |
16.70 KB,
patch
|
Details | Diff | Splinter Review | |
18.30 KB,
patch
|
Details | Diff | Splinter Review | |
18.94 KB,
patch
|
Details | Diff | Splinter Review | |
26.08 KB,
patch
|
Details | Diff | Splinter Review |
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();
Reporter | ||
Updated•23 years ago
|
Blocks: 70229
Summary: [API] need nsIWebBrowserChromeFocus so we can propagate focus info. → need nsIWebBrowserChromeFocus so we can propagate focus info.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
Comment 2•23 years ago
|
||
Why isn't this just part of nsIWebBrowserFocus? Is Alt-tab intentional? Shift-Tab and Tab are usually back and forward respectively...
Reporter | ||
Comment 3•23 years ago
|
||
not sure. I think either you or blizzard suggested this?
Comment 4•23 years ago
|
||
Reporter | ||
Comment 5•23 years ago
|
||
This patch seems to be in the wrong bug.? This patch fills out nsIWebBrowserFocus, not nsIWebBrowserChromeFocus.
Comment 6•23 years ago
|
||
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.
Comment 7•23 years ago
|
||
->dr to get review/approval and checkin patch.
Assignee: saari → dr
Status: ASSIGNED → NEW
Comment 8•23 years ago
|
||
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
Comment 9•23 years ago
|
||
saari, you are corrrect. What does it have to do with the docshell tree owner?
Comment 10•23 years ago
|
||
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.
Reporter | ||
Comment 11•23 years ago
|
||
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?
Comment 12•23 years ago
|
||
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
Comment 13•23 years ago
|
||
Valeski, you are correct. We need this additional interface to pass the information back up to the embeddor.
Comment 14•23 years ago
|
||
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?
Comment 15•23 years ago
|
||
That is correct.
Comment 16•23 years ago
|
||
bonzer, patch coming right up
Assignee | ||
Comment 17•23 years ago
|
||
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.
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
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.
Assignee | ||
Comment 21•23 years ago
|
||
Comment 22•23 years ago
|
||
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.
Assignee | ||
Comment 23•23 years ago
|
||
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" ]
Comment 24•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: [ should be ready "real soon now" ] → Wanted for 0.9 embedding
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 years ago
|
||
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...
Comment 27•23 years ago
|
||
Are you sure the object you're calling it on is actually an instance of nsDocShellTreeOwner and not something else that implements the interface?
Comment 28•23 years ago
|
||
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.
Assignee | ||
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
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...
Assignee | ||
Comment 32•23 years ago
|
||
Assignee | ||
Comment 33•23 years ago
|
||
...and twenty minutes later, after understanding what danm wrote, our hero posts a patch that *actually works*. looking for r=, sr= now.
Whiteboard: Wanted for 0.9 embedding → Wanted for 0.9 embedding, ETA: Tue 24 Apr, at the latest.
Comment 34•23 years ago
|
||
dan, how recent was your tree? I keep getting errors when patching
Assignee | ||
Comment 35•23 years ago
|
||
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.
Comment 36•23 years ago
|
||
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.
Assignee | ||
Comment 37•23 years ago
|
||
Comment 38•23 years ago
|
||
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=.
Comment 39•23 years ago
|
||
a= asa@mozilla.org (on behalf of drivers) for checkin to 0.9
Assignee | ||
Comment 40•23 years ago
|
||
fix checked in (danm checked in .mcp update)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 41•23 years ago
|
||
nsIWebBrowserChromeFocus.idl exists with 2 methods: focusNextElement() and focusPrevElement()
Status: RESOLVED → VERIFIED
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•