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)
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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•24 years ago
|
Blocks: 70229
Summary: [API] need nsIWebBrowserChromeFocus so we can propagate focus info. → need nsIWebBrowserChromeFocus so we can propagate focus info.
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
Comment 2•24 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•24 years ago
|
||
not sure. I think either you or blizzard suggested this?
Comment 4•24 years ago
|
||
Reporter | ||
Comment 5•24 years ago
|
||
This patch seems to be in the wrong bug.? This patch fills out
nsIWebBrowserFocus, not nsIWebBrowserChromeFocus.
Comment 6•24 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•24 years ago
|
||
->dr to get review/approval and checkin patch.
Assignee: saari → dr
Status: ASSIGNED → NEW
Comment 8•24 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•24 years ago
|
||
saari, you are corrrect. What does it have to do with the docshell tree owner?
Comment 10•24 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•24 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•24 years ago
|
||
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
Comment 13•24 years ago
|
||
Valeski, you are correct. We need this additional interface to pass the
information back up to the embeddor.
Comment 14•24 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•24 years ago
|
||
That is correct.
Comment 16•24 years ago
|
||
bonzer, patch coming right up
Assignee | ||
Comment 17•24 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•24 years ago
|
||
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 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•24 years ago
|
||
Comment 22•24 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•24 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•24 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•24 years ago
|
Whiteboard: [ should be ready "real soon now" ] → Wanted for 0.9 embedding
Assignee | ||
Comment 25•24 years ago
|
||
Assignee | ||
Comment 26•24 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•24 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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 31•24 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•24 years ago
|
||
Assignee | ||
Comment 33•24 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•24 years ago
|
||
dan, how recent was your tree? I keep getting errors when patching
Assignee | ||
Comment 35•24 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•24 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•24 years ago
|
||
Comment 38•24 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•24 years ago
|
||
a= asa@mozilla.org (on behalf of drivers) for checkin to 0.9
Assignee | ||
Comment 40•24 years ago
|
||
fix checked in (danm checked in .mcp update)
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 41•24 years ago
|
||
nsIWebBrowserChromeFocus.idl exists with 2 methods: focusNextElement() and
focusPrevElement()
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•