Closed
Bug 178324
Opened 22 years ago
Closed 15 years ago
refactor focus handling
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9.2
People
(Reporter: blizzard, Assigned: enndeakin)
References
(Depends on 2 open bugs, Blocks 2 open bugs, Regressed 1 open bug)
Details
Attachments
(14 files, 11 obsolete files)
514 bytes,
application/xhtml+xml
|
Details | |
18.28 KB,
text/plain
|
Details | |
4.08 KB,
text/plain
|
Details | |
854.22 KB,
patch
|
Details | Diff | Splinter Review | |
8.80 KB,
text/plain
|
Details | |
84.14 KB,
image/png
|
Details | |
6.71 KB,
patch
|
Details | Diff | Splinter Review | |
5.09 KB,
text/plain
|
Details | |
1.98 KB,
text/plain
|
Details | |
9.23 KB,
text/plain
|
Details | |
4.11 KB,
text/plain
|
Details | |
3.80 KB,
text/plain
|
Details | |
1.68 KB,
patch
|
Details | Diff | Splinter Review | |
811.02 KB,
patch
|
Details | Diff | Splinter Review |
It's time to refactor the way that we handle focus since I'm getting sick and tired of dealing with this house of cards. Every time we have a focus issue a little more code gets added to handle an edge case, making everything just that much more fragile. I think it's time we really sit down and figure out how to fix it for real.
Reporter | ||
Comment 1•22 years ago
|
||
Order of things that need to happen: o Figure out what layout is really using o Document the focus capabilities of the various platforms o Figure out what the new ESM code is going to need to look like We need to keep in mind embedding and accessibility and i18n issues (input methods).
Reporter | ||
Comment 2•22 years ago
|
||
Oh, yeah, plugins need to be documented, too.
I think it might be a good idea to reorganize the existing ESM by adding three internal classes: one for managing focus, one for managing mouse in/out, and one for managing drag/drop.
Actually I'm not very familiar with the focus code and I don't know what warts blizzard is intent on filing off. What are the specific design choices that you want to change?
Comment 5•22 years ago
|
||
Many have talked about refactoring focus, but it has withstood all such talk. It would be a great achievement -- I think we would need a really great test suite. See bug 83552 for the massive numbers of focus bugs that have either been fixed or are still open.
OS: Linux → All
QA Contact: jrgm → sairuh
Reporter | ||
Comment 6•22 years ago
|
||
There are a few big ones: o remove activation/deactivation: X doesn't really have this concept and I think we could save a lot of code, both for unix and for other platforms, by using focus changes to the toplevel window instead of activation. If you are worried about having to check to see whether or not the window can be activated before raising the window, this can be done by always calling down to the nsIWidget::SetFocus(); o Don't use focus() in XP code to mean both "set focus to this window" and "raise this window." This makes me insane. We clearly don't always mean raise the window when we change the focus element. window.focus() called from web scripts should keep its semantics, for sure, but other than that I think we need a clear distinction between these two pieces of functionality. o Make the ESM focus code resistent to recusion There are lots of instances where the gtk code will call back into itself from a focus in or focus out event. Actually, these callbacks are generally the result of a call chain like: toplevel focus in -> send GOTFOCUS event -> ESM -> widget->SetFocus() -> tries to grab widget focus anyway, I think there's a lot of room to clean things up here, especially in this area. o virtual focus handling: I would love to have virtual key handling for focus to be contained in the ESM entirely but I don't think that we can get away with that because of at least input methods on unix and accessibility requirements, but it would be nice. o add an event type We might want to seperate the idea of focus from the widget system into two concepts: GOTFOCUS ( as in, the widget system just set focus to this element and this is where all key events are going right now) SETFOCUS ( if focus was in this toplevel window, key events _would_ be set here) There's a lot of room here for discussion though to find the most portable and concise system. I'm still hoping there's someone who can describe what layout needs so that we can start to figure out how the platforms all work.
Reporter | ||
Comment 7•22 years ago
|
||
Aaron, can you tell us what requirements accessibility imposes on any changes? Or anything that might improve accessibility from a focus standpoint?
Updated•22 years ago
|
QA Contact: sairuh → rakeshmishra
Comment 8•22 years ago
|
||
Here's a list of important focus behaviors wrt accessibility: (Forgive me for any redundancies). 1. The focus must always be clearly visible, and can be in only 1 place at a time 2. The focus cannot be in a different place from where the selection or caret is active. 3. The focus cannot get lost or stuck 4. The focus must not stop twice on the same item, unless focus has cycled through the entire document or window 5. The user must be able to tab navigate through all focusable items in a document or window 6. The focus must be exposed correctly through DOM events, accessibility events and through an API that can get the current focused window and element 7. The initial focus for a window or document must follow platform and common sense standards 8. The focus must follow special platform rules, for example sloppy focus on certain window managers 9. In a document, the focus must be in sync with the selection. This allows tabbing from the last found text, for example. 10. The focus behavior should not be erratic or problematic during page loads 11. There should be clear and clean wayz to deal with focus programmatically, via script or C++ 12. Accesskeys, typeaheadfind and navigation by frames are important ways of moving the focus more conveniently than just using the Tab key. Again, please also check bug 83552 -- the meta bug for focus issues.
Blocks: focusnav
Reporter | ||
Comment 9•22 years ago
|
||
> 6. The focus must be exposed correctly through DOM events, accessibility events
> and through an API that can get the current focused window and element
I think this is the one issue that needs understanding here.
What DOM events and accessibility events are related to focus that you know about?
Comment 10•22 years ago
|
||
Do we correctly handle the notion of 'latent' focus at the moment? Focus changes in a non-active window should be properly handled, with elements taking 'latent' focus. On window activation, the latent-focussed widget gets real focus. Also, do we need to distinguish between key and mouse focus? (see bug 175893). Another important issue is focus handling for embedders; embedders need to get into the keyboard loop.
Comment 11•22 years ago
|
||
While we're at it, I'll see if there's anything I can contribute from all the ESM hacking we've done (mostly by Pete Collins) for geometric navigation (i.e. no mouse available at all; only left/right/up/down, including across frame boundaries). This factoring would probably have made our job far easier than it was (er, is, since we're still trying to get it to work right).
OS: All → Linux
I think we can get away with just the GOTFOCUS event if we do latent focus properly. When a GOTFOCUS event arrives we determine from that which document tree is affected, and therefore which latently focused element is now 'really focused', but we shouldn't ever have to actually change which element that is. If we got focus from a mouse event then we'll process that event separately to set the focused element. Maybe it would make more sense for there to be one ESM per document tree instead of per document. (I used to think it was the former, but it's the currently the latter --- right?)
OS: Linux → All
Comment 13•22 years ago
|
||
>Also, do we need to distinguish between key and mouse focus?
Yes
Reporter | ||
Comment 14•22 years ago
|
||
> Do we correctly handle the notion of 'latent' focus at the moment? Focus changes > in a non-active window should be properly handled, with elements taking 'latent' > focus. On window activation, the latent-focussed widget gets real focus. This is exactly what I was talking about wrt SETFOCUS/GOTFOCUS and activation. Right now from the widget standpoint there is no clear distinction. It's a strange mix of activation and .focus() calls. I think we need to come up with a real answer on this because it's never worked correctly on unix. I don't know about other platforms. > > Also, do we need to distinguish between key and mouse focus? (see bug 175893). I don't think so since "mouse focus" is really giving focus as a result of a mouse click and shouldn't be any different than giving focus based on tabbing in between elements. I might be mis-reading that but I can't be sure. > > Another important issue is focus handling for embedders; embedders need to get > into the keyboard loop. Yep. This is one of the reasons why I want to hack on this so that we can get this completely right.
OS: All → Linux
Comment 15•22 years ago
|
||
o Document the focus capabilities of the various platforms We need a table of the focus events, and their sequences in all cases (it varies by case within one OS) on all platforms to have a hope of making this work.
OS: Linux → All
Comment 16•22 years ago
|
||
>> Also, do we need to distinguish between key and mouse focus? (see bug 175893). >I don't think so since "mouse focus" is really giving focus as a result of a >mouse click and shouldn't be any different than giving focus based on tabbing in >between elements. I might be mis-reading that but I can't be sure. Look at how iFrame focus ring behavior working on IE on Win32. You need the distinction.
Reporter | ||
Comment 17•22 years ago
|
||
Can you give me a URL where I can test it and what I should be looking for? I'm not totally sure how a mouse can have focus and how it can affect focus in such a way as it needs to be tracked seperately. Teach me! :)
Comment 18•22 years ago
|
||
>I don't think so since "mouse focus" is really giving focus as a result of a
>mouse click and shouldn't be any different than giving focus based on tabbing in
>between elements. I might be mis-reading that but I can't be sure.
Also on Mac OS X (using full keyboard access mode), clicking a button does not
give it keyboard focus, but I can still tab to it. Buttons getting focus on
click is a real issue for command dispatching, for example.
Comment 19•22 years ago
|
||
> 7. The initial focus for a window or document must follow platform and common
sense standards
to clarify (iirc from UI decisions), initial focus for a browser window or
tabbed browser should be in the content area except for:
a. about:blank - then focus should be in the urlbar.
b. if the web author specified another, particular location (eg, a textfield in
an html form).
Reporter | ||
Comment 20•22 years ago
|
||
> Buttons getting focus on
> click is a real issue for command dispatching, for example.
How do you mean? Can you explain this a little more?
Comment 21•22 years ago
|
||
You should seriously consider adding pre, during, and post versions of each event. This would help clean up some of the nasty code concerning what elements can accept focus (which plays into buttons accepting focus like in Simon's example). Although making more multi event dependent state switches may not be desireable. To clarify Simon's example, you click on a button that takes the selection and makes it bold. But clicking on the button gave it focus so you lost selection. Can't have that.
Reporter | ||
Comment 22•22 years ago
|
||
> You should seriously consider adding pre, during, and post versions of each
> event.
Are you talking about layout events here or widget events?
Comment 23•22 years ago
|
||
Clicking on a button should not give it keyboard focus, period. For example, say you have some kind of <htmlarea> (implemented as an <iframe>), above which the page author has put a row of buttons as a toolbar. If clicking on the Bold button steals keyboard focus from the <iframe>, then the user has to click back in the iframe to be able to type again. Or the page author has to mess with lots of focus() calls.
Comment 24•22 years ago
|
||
>Can you give me a URL where I can test it and what I should be looking for? I'm >not totally sure how a mouse can have focus and how it can affect focus in such >a way as it needs to be tracked seperately. IE draws or doesn't draw focus rectangles around iframes depending on if you tab into them or click on them. So it is making a behavior distinction on how something got focused, mouse vs keyboard. In almost all cases the two can effectively be the same though. >> You should seriously consider adding pre, during, and post versions of each >> event. >Are you talking about layout events here or widget events? Probably needed at the widget, NS_ level to work.
Comment 25•22 years ago
|
||
I always thought the iframe thing was a bug (in both IE and Mozilla).... The distinction for buttons is there, true, and the same argument could apply equally well to links...
Comment 26•22 years ago
|
||
Simon, that's not true. On Win32, buttons do receive keyboard focus when clicked. Toolbar buttons do not, but dialog buttons (and form controls in a page) do. Anyway, whether or not controls receive focus should be determined by CSS properties (separated into one for mouse activity and one for key activity), e.g., like -moz-user-focus but split into two properties). That combined with platform #ifdefs using the preprocessor (or by forking files) should give us all the flexibility we need regarding the accepting of focus. (We can add new values to the properties if we encounter any unusual cases.) Note there is also a CSS3 property for explaining what happens with regards to selection when a control becomes focused. If necessary, we should implement it. Off the top of my head I don't remember all the values.
Comment 27•22 years ago
|
||
I sure don't mind if there's a css property or variable describing how something got focused (keyboard, mouse, script). Just to be clear, the important thing is that there is always only 1 focus, and that the focus appearance is not misleading. There can't be a mouse focus separate from the keyboard focus, of course.
Comment 28•22 years ago
|
||
> Clicking on a button should not give it keyboard focus, period
OK, so this behavior seems to differ between platforms. Mac OS X does not give
focus on click, Windows does.
Comment 29•22 years ago
|
||
>Maybe it would make more sense for there to be one ESM per document tree
>instead of per document. (I used to think it was the former, but it's the
>currently the latter --- right?)
That is why we have the focus controller hanging off the window instead of
putting all focus control in the ESM. Focus is a per top level window singleton,
the ESM is not. Changing the ESM to one per document tree/top level window is
another huge chunk of work and probably something you want to keep out of this
round of changes IMO.
Well, during this round maybe we can move as much focus goop as possible into the focus controller. Then later we can move mouse event handling into a corresponding mouse controller. There is some really horrible stuff in ESM mouse handling to work around the fact that there's an ESM-per-document.
Comment 31•22 years ago
|
||
>o remove activation/deactivation:
I take issue with this, we really do need to distinguish between intra and inter
top level window focus events. The resulting behavior is different (think about
widget activation states). Also, embeddors need to distinguish between focus and
activation state. If I activate a window with an embedded gecko, I want gecko to
look active (that look is different per platform), but it won't necessarily have
focus.
Comment 32•22 years ago
|
||
>Well, during this round maybe we can move as much focus goop as possible into
the focus controller.
That sounds reasonable
Comment 33•22 years ago
|
||
> Well, during this round maybe we can move as much focus goop as possible into
> the focus controller.
Perhaps a staged approach with tests after reasonable stopping points will help
us keep the code clean and bug-free. There are going to be a lot of regressions
with a focus rewrite, because it's a difficult problem. I think the best way to
keep things under control is to try to check it as it progresses. :-)
Stage 1. Move glop out of esm into nsFocusController
Stage 2. ?
Reporter | ||
Comment 34•22 years ago
|
||
> I take issue with this, we really do need to distinguish between intra and inter
> top level window focus events. The resulting behavior is different (think about
> widget activation states). Also, embeddors need to distinguish between focus and
> activation state. If I activate a window with an embedded gecko, I want gecko to
> look active (that look is different per platform), but it won't necessarily have
> focus.
Can you give me a few concrete examples here? The only one that comes to mind
for me is selection. (Selection, btw, is done completely wrong for unix since
window activation should not affect the selected state.)
Comment 35•18 years ago
|
||
Comment 36•18 years ago
|
||
3 years later.... https://bugzilla.mozilla.org/attachment.cgi?id=225235 mouseover and click on first link. return to original page now mouseover second link. Note: blur event has not been fired.
Please don't attach a testcase for your pet bug to an architecture bug; it makes it impossible to use this bug for whatever it was originally for. (I really don't care if it's important, I'm not going to look at it here; it's like standing up in the middle of a wedding and screaming that everybody must do something right now to stop global warming.)
Comment 38•18 years ago
|
||
re:#36 & #37 Thanks dbaron new bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=341231 sorry to spoil the party, It's hard to know where focus bugs are and whose working on them... Might a statement here help elucidate how it is intended to map out the problem? for instance if this is an architecture bug, why is it only blocking one bug? perhaps #341231 might be added? or: https://bugzilla.mozilla.org/show_bug.cgi?id=264244 Boris Zbarsky wrote "We have existing bugs on this." with no link.... ~
Updated•18 years ago
|
Assignee: blizzard → mats.palmgren
Comment 39•17 years ago
|
||
The keyboard focusing is actually a very sinister security issue. I can't count the number of times over the years that I have selected a username/password editable field in Firefox while art and script garbage was still loading in the background, and had keyboard focus ripped out from under me while I was typing, sending my password into some other editable field. More often, I type the whole email/password into ANY site, while looking elsewhere, look at the screen again, and all the typing was eaten by some button, and some different button has been activated by 'Enter' because focus went elsewhere. Let alone all the times filling out forms or typing into that 'Search box' in the corner, and had a loading web page grab my focus while I was typing THERE. As a rule, if I have CLICKED or otherwise manually navigated to an editable field and ABOVE ALL, if I have started typing into it, the browser should never, ever, EVER steal focus away. Period. This has been broken from day one. If you want to be 'standard compliant' to an infuriating, pedantic "standard", then stick a checkbox in the settings to turn this behavior on selectively, so most people won't have to suffer.
Assignee | ||
Comment 40•15 years ago
|
||
This patch: - moves focus handling into a single class and out of the EventStateManager, FocusController and other places - stores the active window and focused frame in this class - stores the focused element in the (inner) nsGlobalWindow, or the last element that was focused - removes a lot of hacks - includes tests Not tested extensively yet are embedding and accessibility. Some work with ime still to do and some existing tests fail with this patch.
Assignee: mats.palmgren → enndeakin
Attachment #225235 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment 41•15 years ago
|
||
I gave a try to the build from https://build.mozilla.org/tryserver-builds/2008-12-30_08:15-neil@mozilla.com-focus/ on Vista. I found one issue with Google Docs: The caret is not visible. This is reproducible with a designMode enabled document: data:text/html,<script>document.designMode="on"</script>
Comment 42•15 years ago
|
||
This is fantastic cleanup Neil. Good work, I hope the patch makes it all the way in!
Comment 43•15 years ago
|
||
Comment on attachment 354846 [details] [diff] [review] refactor focus patch work in progress I tried to skim the patch but I have a couple of queries: 1. What replaces the code that restores the focus to where it was before print preview? 2. Can you be sure that no C++ callers try to directly focus a XUL textbox?
Assignee | ||
Comment 44•15 years ago
|
||
(In reply to comment #43) > 1. What replaces the code that restores the focus to where it was before print > preview? I'm not sure why I removed that. I've added it back in. I notice though a bug where changing the scale causes a second print preview to occur which resets this. Is there a bug filed for this? > 2. Can you be sure that no C++ callers try to directly focus a XUL textbox? Hmmm. I think I had assumed that it would get overridden, but thinking about it, I suppose it would not.
Comment 45•15 years ago
|
||
I'm glad to report that this fixes the issue in gmail if you alt-enter to open a url, the focus is no longer pushed to the first element inside the window (which breaks gmail's key press event listeners), and instead leaves it in the correct frame. I'm assuming we'll be able to easily add something to remove focus from the location bar on pressing.. say escape, and have the browser regain focus? I see in the tryserver build that pressing esc from the findbar correctly has gmail pick up events instead of trunk's behavior of triggering quickfind.
Comment 46•15 years ago
|
||
(In reply to comment #45) > I'm assuming we'll be able to easily add something to remove focus from the > location bar on pressing.. say escape, and have the browser regain focus? Yeah, that would be very useful. Actually i have added that in my own extension, but i really think that Escape key behavior should be default in firefox.
Comment 47•15 years ago
|
||
(In reply to comment #45) > I'm assuming we'll be able to easily add something to remove focus from the > location bar on pressing.. say escape, and have the browser regain focus? Er, I certainly hope this would NOT be the default, at least not on the Mac, as it's not at all what any other Mac browser does (or has ever done). Escape, when pressed while focus is in a text field, cancels any active edit in that text field and restores the selection to whatever was selected before the edit began. The location bar and the Find bar are two extremely different pieces of UI, however, and it's not entirely clear which one comment 45 is referring to, since it refers to both in successive sentences.
Comment 48•15 years ago
|
||
File a new bug on that, please, if you want or don't want it; it's off-topic for this bug.
Updated•15 years ago
|
QA Contact: rakeshmishra → xptoolkit.widgets
Assignee | ||
Comment 49•15 years ago
|
||
Fixes: - the caret now appears properly for designmode documents - the focus is properly restored when exiting print preview mode - focus now works properly when using IME - changed the tab switching behaviour to focus the new tab when either something in the new tab is focused, the old tab is focused or nothing is focused - supported the mobile offscreen browser feature - fixed up some area element checks so that they check that it is an html element - fixed the test for bug 398289 to start after a timeout , as it was failing because <dialog> focuses content inside it on load, causing the content to scroll. - all tests now pass Only one issue remains that I found today: accesskeys for XUL textboxes do not work Plugins have been tested and work. Accessibility and embedding has not been tested.
Attachment #354846 -
Attachment is obsolete: true
Assignee | ||
Comment 50•15 years ago
|
||
Fresh builds are at: https://build.mozilla.org/tryserver-builds/2009-01-27_11:34-neil@mozilla.com-refocus2/
Comment 51•15 years ago
|
||
I tried these latest try server build. I crash in this testcase, while pressing the tab key.
Assignee | ||
Comment 52•15 years ago
|
||
I don't see any crash for that testcase. How are you running it? Is the security dialog repeatedly popping up? Is there a stacktrace?
Comment 53•15 years ago
|
||
Yeah, well, I added the privileged code, so it crashes automatically. I'm afraid I'm not able to get a stacktrace, because it's a tryserver build. I'm on Windows, btw.
Assignee | ||
Comment 54•15 years ago
|
||
OK thanks Martijn, I could reproduce on Windows. The child frame in the testcase has no presshell and a null-check was missing when tab-navigating into child frames.
Assignee | ||
Comment 55•15 years ago
|
||
Fixes in this version: - fixed crash when tabbing into a frame with no document - now handles accesskeys for textboxes, editable menulists and scales properly and added a test for this - reworked is focusable checks so that special focus/blur methods aren't needed for those elements (point 2 of comment 43) - fixed a leak when retrieving the focused element in child frames smaug implied that he'd like to review this.
Attachment #359125 -
Attachment is obsolete: true
Attachment #362321 -
Flags: review?(Olli.Pettay)
Comment 56•15 years ago
|
||
Comment on attachment 362321 [details] [diff] [review] focus patch r+ for Cocoa widget changes
Attachment #362321 -
Flags: review+
Comment 57•15 years ago
|
||
Josh, have you tested the Cocoa widgets changes? There are a *lot* of bugs they may reopen.
Comment 58•15 years ago
|
||
Masayuki, you should know about the changes this patch (attachment 362231 [details]) makes. They may reopen some of the bugs you've fixed.
Assignee | ||
Comment 59•15 years ago
|
||
I noticed a merge error in the patch in nsChildView.mm. The IME realted changes made by bug 466408 weren't meant to be removed. I'll add those back in subsequent versions.
Comment 60•15 years ago
|
||
Neil, did *you* do any testing of the Cocoa widgets changes?
Assignee | ||
Comment 61•15 years ago
|
||
I've tested it extensively. For instance, I tested that Cocoa bugs 354768, 314160, 355138, 358379, 377105, 381448, 403232, 408266, 419338, 432817, 432131, 431429, 428047, 470286, 445531, 413882, 396952, 395465, 404433, 430419, 396832, 402926, 443024, 156583 don't regress with this patch. The only issue I've noticed since the patch was an issue with ime in certain fields which I will investigate.
Comment 62•15 years ago
|
||
(In reply to comment #61) OK. And thanks for letting me know. I will (of course) also dig through old bugs, to find anything you might have missed. I'll report back here.
Comment 63•15 years ago
|
||
I tested a build for over an hour, though I did not test IME.
Comment 64•15 years ago
|
||
Neil, a few questions/nits from the changes in the accessibility module: > already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus() > [...] >+ nsIDOMNode *focusedNode = nsnull; >+ if (focusedElement) { >+ CallQueryInterface(focusedElement, focusedNode); This new code makes use of CallQueryInterface. From a recent pitfall with following constructive discussion, we learned from bz and peterv that it's preferred to limit CallQI to outparams, and use smart pointers in all other cases. Would it be possible to do the above using smart pointers and not using CallQI? From nsDocAccessible: >+ nsCOMPtr<nsIDOMDocument> domdocument; Nit: Our style we use almost consistently would have called this variable domDocument.
Comment 65•15 years ago
|
||
(In reply to comment #64) > Neil, a few questions/nits from the changes in the accessibility module: > > already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus() > > [...] > >+ nsIDOMNode *focusedNode = nsnull; > >+ if (focusedElement) { > >+ CallQueryInterface(focusedElement, focusedNode); > > This new code makes use of CallQueryInterface. From a recent pitfall with > following constructive discussion, we learned from bz and peterv that it's > preferred to limit CallQI to outparams, and use smart pointers in all other > cases. Would it be possible to do the above using smart pointers and not using > CallQI? already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus() is a perfect use case for CallQueryInterface, because it returns already_AddRefed<...>
Comment 66•15 years ago
|
||
Though, one could use nsCOMPtr<nsIDOMNode> focusedNode; and return focusedNode.forget();
Comment 67•15 years ago
|
||
Enn, you're removing nsIFocusEventSuppressorService. Have you ensured that the change doesn't regress bug 421209 or bug 395609. See bug 395609 comment #75.
Reporter | ||
Comment 68•15 years ago
|
||
Adding some GTK+ widget people. The interactions of focus on gtk and embedding can often be very exciting.
Assignee | ||
Comment 69•15 years ago
|
||
(In reply to comment #67) > Enn, you're removing nsIFocusEventSuppressorService. > Have you ensured that the change doesn't regress bug 421209 or bug 395609. > See bug 395609 comment #75. The testcases in those bugs don't crash. But then with this patch, the gotfocus/lostfocus events aren't used and focus events can only be fired on window activate/deactivate. In addition, the focus system is called when a document is hidden/unloaded (nsFocusManager::FrameHidden) to clear out any focus state. So I don't think those bugs would occur. That said, I'll add a minor level of extra protection in the next version of the patch to block focus events for docshells that are being destroyed.
Comment 70•15 years ago
|
||
Comment on attachment 362321 [details] [diff] [review] focus patch I don't think you can simply remove the method swizzling for nsCocoaWindow_NSWindow_sendEvent:. The NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK guard there was specifically added to fix bug 442245, removing it will regress that bug.
Assignee | ||
Comment 71•15 years ago
|
||
(In reply to comment #70) > (From update of attachment 362321 [details] [diff] [review]) > I don't think you can simply remove the method swizzling for > nsCocoaWindow_NSWindow_sendEvent:. The NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK guard > there was specifically added to fix bug 442245, removing it will regress that > bug. Are you saying that the default implementation of sendEvent is deficient that it has to be overridden? I would have assumed that the default exception handling would have been correct.
Comment 72•15 years ago
|
||
(In reply to comment #71) I think I know the answer to this one: It's not that the default implementation of [NSWindow sendEvent:] is deficient. It's that the way we've fixed bug 163260 makes the browser crash on non-fatal Objective-C exceptions. Since the particular non-fatal exception that triggers the crash at bug 442245 happens when a keyboard event is handled (by system code, while it's being processed by [NSWindow sendEvent:]), [NSWindow sendEvent:] is a convenient place to install a handler (via method swizzling) to stop these exceptions from being fatal. But it occurs to me that, as long as we're only concerned with non-embedders, we might not have to use method swizzling at all. In non-embedders all top-level windows are ToolbarWindow objects. Maybe all we'd have to do is override ToolbarWindow's sendEvent: method, and bracket the call to super with a NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK try/catch block.
Comment 73•15 years ago
|
||
Could it just go around the call to [super sendEvent:anEvent]? http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/715126a986e4/widget/src/cocoa/nsCocoaWindow.mm#l1927
Comment 74•15 years ago
|
||
(In reply to comment #73) Yes, it could.
Comment 75•15 years ago
|
||
Comment 76•15 years ago
|
||
Updated•15 years ago
|
Attachment #362321 -
Flags: review+
Comment 77•15 years ago
|
||
Comment on attachment 362321 [details] [diff] [review] focus patch (Following up comment #62) I've now tested your patch (with the fix for bug 466408 restored) on the list of (Minefield-specific) focus-related problems in bug 314160 comment #19. It passed with flying colors -- I didn't have a single problem. I'm truly impressed. Of course, since your patch is cross-platform and very complex, it will need to pass many more tests and get many more reviews before it can be landed. But as best I can tell it has no problems on OS X. So I'm seconding Josh's r+ for the Cocoa widgets changes (with the fix for bug 466408 restored, of course).
Comment 78•15 years ago
|
||
Here's what I tested with -- Neil's feb13 patch with the fix for bug 466408 restored and updated to current trunk code. And here are some tryserver builds: https://build.mozilla.org/tryserver-builds/2009-02-20_12:30-smichaud@pobox.com-bugzilla178324-feb13-enndeakin/ They should help with testing.
Updated•15 years ago
|
Attachment #362321 -
Flags: review?(emaijala)
Attachment #362321 -
Flags: review?(Olli.Pettay)
Attachment #362321 -
Flags: review-
Comment 79•15 years ago
|
||
Comment on attachment 362321 [details] [diff] [review] focus patch Ere, perhaps you could look at at least the Windows part?
Assignee | ||
Comment 80•15 years ago
|
||
> diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul > ... > - onblur="document.getElementById('identity-box').style.MozUserFocus = 'ignore';"> > + onblur="setTimeout(function() document.getElementById('identity-box').style.MozUserFocus = '', 0);"> > Why this change? This line (and the onfocus line above it) are meant so that shift+tabbing to the identity box may be used, by making the identity box only focusable when the url field is focused, but not focusable otherwise. However, with this patch, firing the blur event would cause the -moz-user-focus to change to 'ignore' making it non-focsuable again. It worked before because the focus code wasn't very good at making sure that focusable elements were actually focusable. > diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > - this._fastFind.setDocShell(this.mCurrentBrowser.docShell); > + try { > + this._fastFind.setDocShell(this.mCurrentBrowser.docShell); > + } catch (ex) { } > Why this change? I think this was remnant from when I hadn't finished the find functionality. I've removed this change. > diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h > --- a/content/base/public/nsIContent.h > +++ b/content/base/public/nsIContent.h > You should update the IID How odd. I must have merged wrong as this was in other versions. > You've ensured these changes don't cause any new security problems? > Like, this doesn't expose any non-same-origin element to the non-privileged caller? It should only ever be returning an element in the same document. But I'll add a security check and some assertions just to be safe. > @@ -2921,53 +2421,71 @@ nsEventStateManager::PostHandleEvent(nsP > viewMan->GrabMouseEvents(nsnull, result); > } > } > } > break; > } > > if (nsEventStatus_eConsumeNoDefault != *aStatus) { > - nsCOMPtr<nsIContent> newFocus; > - PRBool suppressBlur = PR_FALSE; > - if (mCurrentTarget) { > - mCurrentTarget->GetContentForEvent(mPresContext, aEvent, getter_AddRefs(newFocus)); > Can you really remove this? The change doesn't break image maps or anything? I reverted much of this change. > @@ -4893,68 +3790,18 @@ nsEventStateManager::SetContentState(nsI > For content state changes you may want to ask a review from dbaron. OK > diff --git a/content/events/test/test_bug450876.html b/content/events/test/test_bug450876.html > + is(document.hasFocus(), true, "document should be focused"); > wu.sendKeyEvent('keypress', 9, 0, 0); > - is(document.activeElement, document.body, "body element should be focused"); > + is(document.activeElement, document.getElementById('a'), "body element should be focused"); > + is(document.hasFocus(), false, "document should not be focused"); > Why these changes? The focused element in a frame doesn't clear on the old document when shift+tabbing to a new frame. > Why is this code needed now here? > What if we're in print preview, so that the primary shell does have > link handler but print preview shell doesn't (if that is a possible case)? I still need to investigate this > diff --git a/content/xbl/src/nsXBLPrototypeHandler.cpp b/content/xbl/src/nsXBLPrototypeHandler.cpp > --- a/content/xbl/src/nsXBLPrototypeHandler.cpp > +++ b/content/xbl/src/nsXBLPrototypeHandler.cpp > @@ -451,17 +452,19 @@ nsXBLPrototypeHandler::DispatchXBLComman > mEventName->ToString(type); > > if (type.EqualsLiteral("keypress") && > mDetail == nsIDOMKeyEvent::DOM_VK_SPACE && > mMisc == 1) { > // get the focused element so that we can pageDown only at > // certain times. > nsCOMPtr<nsIDOMElement> focusedElement; > - focusController->GetFocusedElement(getter_AddRefs(focusedElement)); > + nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > + if (fm) > + fm->GetFocusedElement(getter_AddRefs(focusedElement)); > Does this change the behavior, if focused element isn't in this document? I changed this to use the target's window. Really this whole special case for the space key doesn't belong here though. > -<body style="height: 100%" onload="onBodyLoad();"> > +<body style="height: 100%" onload="setTimeout(onBodyLoad, 0);"> > Why this change is needed? Because the focus is updated by the dialog used in the child. (I'm unclear why a dialog is used in the testcase though) and it adjusts the persistence check. I don't recall the exact reason, but I remember it being correct. > Could we move Get/SetPopupNode and GetController*() to some other interface > and kill the whole nsIFocusController? I plan to do this in a followup bug. > diff --git a/dom/public/idl/base/nsIDOMWindowUtils.idl b/dom/public/idl/base/nsIDOMWindowUtils.idl > ... > + * Do not use this method. Just use element.focus instead. > + * > Hmm, would it be easy to convert all the callers to use element.focus and > just remove the method. Sure, in a follow up bug though. > diff --git a/dom/public/idl/base/nsIFocusManager.idl b/dom/public/idl/base/nsIFocusManager.idl > ... > + * If the frame's reference is to a frame element (iframe, browser, > + * editor), then the child frame contains the element that is currently > + * focused. If the frame's reference is to a root element, then the root is > + * focused. If a frame's reference is null, then no element is focused, yet > + * the frame is still focused. > frame's reference? I don't like using "frame" in this context. > Perhaps DOMWindow? I updated to use window everywhere. I wasn't sure which term to use. I don't however like having both an 'activeWindow' and a 'focusedWindow' property. > + attribute nsIDOMWindow activeWindow; > I guess we can't unactivate all windows? The property will be set to null if there is no active window. The focus manager doesn't provide a means of lowering windows, but in the future I hope to combine this with the window mediator/window watcher. > Does activeWindow point to the topmost window (window.top in chrome UI)? > Based on focusedFrame documentation, yes. The topmost window yes. > Should this be readonly attribute, so that setting focus goes always > via setFocus(...)? Or how is setting focusedElement related to > setFocus? It's actually just a shorthand for SetFocus(element, FLAG_RAISE), but I agree that it could be removed. In fact, it's probably better that way and require that the flags be set explicitly. > + void clearFocus(in nsIDOMWindow aFrame); > And perhaps even this could be part of the same setFocus() ? Not really. None of the arguments are in common and its functionality is quite different. > + /** > + * ...The return value will be null if element is focused. > This is strange for a method which is called "getFocusedElementForFrame" That should say 'if *no* element is focused'. > Can nsIFocusManager handle case like Bug 419059? I'd consider that bug to be invalid though as accesskeys shouldn't be working for hidden elements. The bug commenters are confusing accesskeys (which don't work when a menu is closed) with key shortcuts (which do). Anyway, the testcase worked in previous versions, and the testcase still works with this patch in the sense that the link will still get activated, but it will not get focused. Hidden elements cannot be focused anyway so the focus behaviour shouldn't change. > diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml > - if (!this.hasAttribute('focused')) { > - if (event.originalTarget != this.inputField) > - this.inputField.focus(); > - this.setAttribute('focused','true'); > - } > + this.setAttribute('focused','true'); > I don't understand this change. The code here was retargeting the focus to the inner anonymous input field. The focus manager does this directly now. > diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp > ... > +#ifdef USE_XIM > if (gFocusWindow) { > - nsRefPtr<nsWindow> kungFuDeathGrip = gFocusWindow; > -#ifdef USE_XIM > You've made sure you can remove kungFuDeathGrip? I'll add it back in just in case. > if (!gFocusWindow) { > - containerWindow->mActivatePending = PR_FALSE; > + // XXXndeakin P2 hmmm. is this right? > DispatchActivateEvent(); > } > When could this happen? When could what happen? Activating a window upon a button press? I think my comment here is just something I wanted to test more, but I can remove it now. > Have you checked that focus handling changes don't allow popups in cases > where popups are currently disabled? Yes, and it works the same as far as I can tell. > +#ifdef MOZ_XUL > +#include "nsIDOMXULTextboxElement.h" > +#include "nsIDOMXULMenuListElement.h" > +#endif > You have tested that this all compiles with and without XUL? I will check this somehow. > Nit, I'd prefer > if (...) { > ... > } else { > ... > } Well I don't actually ;) > + // don't fire events on the root content > So why not? What if the root is <input> or similar? I'll have to think about this one. +nsresult +nsFocusManager::GetNextTabbableContent(nsIPresShell* aPresShell, Have you tested that tabbing works with print preview too? Yes.
Assignee | ||
Comment 81•15 years ago
|
||
>> + // don't fire events on the root content >> So why not? What if the root is <input> or similar? > >I'll have to think about this one. Actually, creating an xhtml document with only an <input> element doesn't work anyway. It creates a squished field which cannot be typed into.
Assignee | ||
Comment 82•15 years ago
|
||
> Why is this code needed now here?
> What if we're in print preview, so that the primary shell does have
> link handler but print preview shell doesn't (if that is a possible case)?
Do you think it would be worth passing an extra prescontext or presshell argument to all of the IsFocusable methods just for this one case?
Assignee | ||
Comment 83•15 years ago
|
||
> +#ifdef MOZ_XUL
> +#include "nsIDOMXULTextboxElement.h"
> +#include "nsIDOMXULMenuListElement.h"
> +#endif
> You have tested that this all compiles with and without XUL?
It seems to compile ok, when I hack up some fixes due to trunk not actually building without XUL currently.
Comment 84•15 years ago
|
||
Comment on attachment 362321 [details] [diff] [review] focus patch I'm thoroughly impressed with Enn's work on the patch. It does fix e.g. bug 261074 which I couldn't fix in Windows widget code alone. So far I couldn't find any regressions.
Attachment #362321 -
Flags: review?(emaijala) → review+
Assignee | ||
Comment 85•15 years ago
|
||
Hopefully I fixed all the issues. Builds of this patch are at https://build.mozilla.org/tryserver-builds/2009-02-24_18:09-neil@mozilla.com-refocus3b/ Smaug and I discussed and we will look at the print preview issue in a followup bug. Focus in print preview seems to work as is, but further investigation is needed.
Attachment #364039 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•15 years ago
|
Attachment #364039 -
Flags: review?(dbaron)
Assignee | ||
Comment 86•15 years ago
|
||
Comment on attachment 364039 [details] [diff] [review] focus patch v2 request from smaug to have dbaron review the content state changes
Comment 87•15 years ago
|
||
test_focus.xul doesn't run on linux if mouse is not over that window. When mouse is moved over to that window, tests start to run again. I use the "focus follows mouse" setting in the window manager. When the mouse is at the place where test_focus.xul window opens, tests usually pass, but I did see some failures once. So, the test could be more reliable. (Other unreliable test is the largemenu, but that is about some other bug) There are also some leaks when running chrome tests. For example nsFocusManager is leaked. Not sure if the patch causes those leaks. But in any case, nsFocusManager should participate to cycle collection. Also running browser-chrome leaks. All tests pass. Running mochitest leaks and there are 14 failures with html/content/test/test_bug430351.html
Comment 88•15 years ago
|
||
A number of current mochitests already fail if the mochitest window doesn't have focus, so this is not exactly making things worse. :(
Comment 89•15 years ago
|
||
test_focus.xul failure is something new and else than "normal" focus problem. Mouse is over the main window, because it is used to start the test. Then a new window is created to run test_focus.xul, but those fails because apparently that doesn't get the focus the way it expects.
Assignee | ||
Comment 90•15 years ago
|
||
For the chrome tests, I see an nsRunnable and an nsBaseAppShell leaking but nothing else. Is this the leak you mean? Cycle collection would be good to have and I'll add it.
> When the mouse is at the place where test_focus.xul window opens,
Which window do you mean? test_focus.xul itself only loads in the main test iframe. I assume that opening a new window in focus follows mouse mode doesn't get focus immediately unless the mouse happens to be over it.
This test opens a number of child windows in order to test focus switching between windows. I would assume that no other test does this, but I wouldn't imagine the need for window focus for this test is actually something that wouldn't have failed before this patch as well.
Comment 91•15 years ago
|
||
(In reply to comment #90) > I assume that opening a new window in focus follows mouse mode doesn't > get focus immediately unless the mouse happens to be over it. Often (perhaps usually) the new window does get focus with "focus follows mouse". But it depends on the window manager, what hints are provided to the window manager (about the purpose of the windows), and timing of events. (If the user continues to type in the first window after initiating the other window but before the other window opens, then the window manager will likely prevent the new window from stealing focus.)
Comment 92•15 years ago
|
||
Comment on attachment 364039 [details] [diff] [review] focus patch v2 I redid the Minefield-specific tests from bug 314160 comment #19, and once again I had no problems. But this time I also ran all the automated tests listed at https://developer.mozilla.org/en/Mozilla_automated_testing, and saw a couple of anomalies: The 439206-1.html crashtest failed (crashed) consistently (twice in a row) with this patch, but not at all without it. The test_bug430351.html Mochitest had a bunch of failures with this patch, and none without it. I tested the patch by applying it to "old" code and then updating it to current code. The non-patched build I tested (as a control) was the "same" code without the patch.
Comment 93•15 years ago
|
||
> The 439206-1.html crashtest failed (crashed) consistently (twice in
> a row) with this patch, but not at all without it.
Oddly, the 439206-1.html test doesn't crash when I load it
individually -- only when I use -reftest to load it as part of a list
of crashtests.
Comment 94•15 years ago
|
||
Comment on attachment 364039 [details] [diff] [review] focus patch v2 >> The 439206-1.html crashtest failed (crashed) consistently (twice in >> a row) with this patch, but not at all without it. > > Oddly, the 439206-1.html test doesn't crash when I load it > individually -- only when I use -reftest to load it as part of a > list of crashtests. Dumb mistake. It's the *next* crashtest, 473284.xul, that crashes (though only with this patch). It crashes loaded individually, and loaded with -reftest in a list.
Comment 95•15 years ago
|
||
Updated•15 years ago
|
Attachment #364039 -
Flags: review?(Olli.Pettay) → review-
Comment 96•15 years ago
|
||
Comment on attachment 364039 [details] [diff] [review] focus patch v2 So I'd like to see the memory leak fixes.
Comment 97•15 years ago
|
||
(In reply to comment #85) > Builds of this patch are at > https://build.mozilla.org/tryserver-builds/2009-02-24_18:09-neil@mozilla.com-refocus3b/ I'm on Windows XP SP2 (Japanese). Steps to reproduce: 1. Add Japanese keyboard and Microsoft Natural Input 2002 ver.8.1 (or Microsoft Natural Input 2003) 2. Change intl.enable_tsf_support to true and restart Firefox. 3. Focus location bar, search bar, <input>, <textarea> or somewhere which can be inputted. 4. Bring another window to front. 5. Bring the window of 3. to front. Actual result: Microsoft Natural Input can not be selected. Expected result: Microsoft Natural Input can be selected.
Assignee | ||
Comment 98•15 years ago
|
||
I don't see any issue with IME as described in comment 97. Enabling the preference and using Japanese Natural Input popups up the IME panel whenever a text entry field is focused and disappears when it is blurred. I'm not using a Japanese OS but I doubt that's the issue.
Comment 99•15 years ago
|
||
Comment 100•15 years ago
|
||
The bar in the screenshot is "Language Bar", see http://support.microsoft.com/kb/306993 It's displayed by default on Japanese Windows, but I don't know whether it's same on Western Windows.
Comment 101•15 years ago
|
||
I have been using the focus patch from attachment 362321 [details] [diff] [review] for a while in a Minefield build on OS/2 (unfortunately, I didn't manage to rebuild with the newer patch yet). I only found one problem, that I thought I should share: I cannot always reproduce it, but there is a high probability that the following causes the problem on my setup: - go to http://www2.computeruniverse.net/comment.asp?id=90194875&language=deutsch - open another Minefield window with some other webpage - scroll down in the first window to the entry fields (below "Begründungen") - click on a field (focus should be in the field) - click in the other window (focus should move there) - click back onto the entry field in the first window --> fairly often the focus does not end up in that field but somehow stays in the other window; while the field reacts on mouse-down (color changes) it changes color back to white on mouse-up and the caret doesn't appear
Assignee | ||
Comment 102•15 years ago
|
||
Peter, can you create a build which has the two debugging defines near the top of nsFocusManager.cpp enabled, and post or send me the log of what happens? It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel as is done on Windows.
Assignee | ||
Comment 103•15 years ago
|
||
(In reply to comment #100) > The bar in the screenshot is "Language Bar", see > http://support.microsoft.com/kb/306993 > > It's displayed by default on Japanese Windows, but I don't know whether it's > same on Western Windows. I get the ime language bar when a textbox is focused and it gets disabled whenever a non-textbox is focused. But the "Natural Input" choice (with the blue icon) is always disabled on the menu, even in other applications. The other "IME" choice with the red icon works fine. I wonder if there's something else I need to enable/install in order to get this extra menu enabled.
Comment 104•15 years ago
|
||
(In reply to comment #103) > "Natural Input" choice (with the > blue icon) is always disabled on the menu, even in other applications. The > other "IME" choice with the red icon works fine. I wonder if there's something > else I need to enable/install in order to get this extra menu enabled. Nakano-san: Could you help with this?
(In reply to comment #80) > > For content state changes you may want to ask a review from dbaron. So what exactly is it that you'd like me to review? Based on going through the patch, it looks like you've removed a large number of esm->SetContentState calls from various parts of the code (perhaps all of the existing ones) and replaced them with two calls in nsFocusManager. You've also removed the "interesting" part of the (aState & NS_EVENT_STATE_FOCUS) code in nsEventStateManager::SetContentState. This all seems pretty reasonable to me. Is that the bit that you wanted me to review, or anything else? (I'm probably not in a position to judge if the old and new sets of calls to SetContentState are equivalent, but the new set sure seems more likely to be accurate.) The one thing I wonder about is how the :focus pseudo-class ought to interact with the somewhat tree-like nature of focus. That said, I don't know if there are ever two nodes in the tree in the same document... rather than an iframe being focused in an outer document and something inside that iframe also being focused. If that's not the case, then it's probably all simple... although I wonder if that really ought to be the case (e.g., should scrollbars from 'overflow:scroll' really behave differently from scrollbars from an iframe?). Sorry for the delay in getting to this.
Comment 106•15 years ago
|
||
(In reply to comment #103) > But the "Natural Input" choice (with the > blue icon) is always disabled on the menu, I found a similar (maybe same) issue without Natural Input. Steps to reproduce: 1. Add Drawing Pad in Regional and Language Options -> Languages -> Details. 2. Change intl.enable_tsf_support to true and restart Firefox. 3. Focus location bar, search bar, <input>, <textarea> or somewhere which can be inputted. Drawing Pad should be in the Language Bar, even though it's disabled. 4. Bring another window to front. 5. Bring the window of 3. to front. Actual result: Drawing Pad is not in the Language Bar. Expected result: Drawing Pad should be still in the Language Bar.
Comment 107•15 years ago
|
||
I found another issue. Steps to reproduce: 1. Add Drawing Pad in Regional and Language Options -> Languages -> Details. 2. Change intl.enable_tsf_support to true and restart Firefox. 3. Click a <input> or <textarea> in a content. 4. Click the Location Bar. 5. Repeat 3, 4, 3, 4, ... Actual result: Drawing Pad appears in the Language Bar at only either the content or the Location Bar. Expected result: Drawing Pad should appear in the Language Bar at both the content and the Location Bar.
Comment 108•15 years ago
|
||
(In reply to comment #104) > (In reply to comment #103) > > "Natural Input" choice (with the > > blue icon) is always disabled on the menu, even in other applications. The > > other "IME" choice with the red icon works fine. I wonder if there's something > > else I need to enable/install in order to get this extra menu enabled. > > Nakano-san: Could you help with this? Maybe, WinXP of en-US is not installed the "Natural Input". Because it is not displayed in the list of "Text Services and Input Languages".
Comment 109•15 years ago
|
||
Sakai-san, can you reproduce the bug on current trunk build? I changed the behavior at focus changing in bug 477911. It might help the bug.
Comment 110•15 years ago
|
||
(In reply to comment #109) > Sakai-san, can you reproduce the bug on current trunk build? I changed the > behavior at focus changing in bug 477911. It might help the bug. Trunk doesn't have this problem, both current and also before bug 477911. I want to test current trunk with focus refactoring, but the focus patch v2 can't be merged because of conflict.
Comment 111•15 years ago
|
||
Ah, of course, I meant "current trunk with focus refactoring patch"...
Comment 112•15 years ago
|
||
(In reply to comment #102) > Peter, can you create a build which has the two debugging defines near the top > of nsFocusManager.cpp enabled, and post or send me the log of what happens? > > It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel > as is done on Windows. Thanks for the hints, Neil. Hopefully will manage to give those a try on the weekend.
Comment 113•15 years ago
|
||
(In reply to comment #109) > I changed the > behavior at focus changing in bug 477911. It might help the bug. I've tested 2009-02-24's trunk + focus patch v2 + bug 477911. https://build.mozilla.org/tryserver-builds/2009-03-10_01:13-dev-null@hotmail.co.jp-try-9564b123105/ But it still has problem.
Comment 114•15 years ago
|
||
I found that IME's transaction is not committed when the window is being deactivated. I think it should be committed.
Comment 115•15 years ago
|
||
(In reply to comment #114) > I found that IME's transaction is not committed when the window is being > deactivated. Also on Mac, IME's transaction is not committed when switch to another window in same process.
Comment 116•15 years ago
|
||
More problem, on Windows XP SP2 (Japanese) Steps to reproduce: 1. Focus <input> or <textarea> in content. 2. Select Natural Input. 3. Start composition with IME. 4. Click Location Bar during IME transaction. Actual Result: IME committed string is inserted into Location Bar. Expected Result: IME committed string should not inserted into Location Bar. 2009-02-24's trunk + focus patch v2 (Comment #85) fails. 2009-02-24's trunk + focus patch v2 + bug 477911 (Comment #113) fails. 2009-02-24's trunk (without patch) works fine. 2009-03-10's trunk (without patch) works fine.
Comment 117•15 years ago
|
||
(In reply to comment #105) > Is that the bit that you wanted me to review, or anything else? Yes
Comment 118•15 years ago
|
||
(In reply to comment #117) > (In reply to comment #105) > > Is that the bit that you wanted me to review? > Yes
Assignee | ||
Comment 119•15 years ago
|
||
There are new test builds at https://build.mozilla.org/tryserver-builds/2009-03-10_12:28-neil@mozilla.com-refocus3c/ Can you check if these fix the IME issues?
Comment 120•15 years ago
|
||
(In reply to comment #119) > Can you check if these fix the IME issues? Thanks for new test build. On Windows, all mentioned problems have gone. But on Mac OS X 10.5.6, comment #115 is still reproduced.
Comment 121•15 years ago
|
||
(In reply to comment #120) > On Windows, all mentioned problems have gone. But I found another problem on Windows. 1. If intl.enable_tsf_support is changed to true, reset it and restart Firefox. 2. Focus <input>, <textarea>, Location Bar or somewhere which can be inputted. 3. Select Microsoft IME. 4. Start composition, for eaxample input some text in Hiragana mode. 5. Activate another window. Actual Result: IME's transaction is not committed. Expected Result: IME's transaction should be committed.
Comment 122•15 years ago
|
||
(In reply to comment #119) > There are new test builds at > https://build.mozilla.org/tryserver-builds/2009-03-10_12:28-neil@mozilla.com-refocus3c/ Summary: On Windows, with intl.enable_tsf_support is false (default), IME composition string is not committed when another window is activated. It should be committed. On Mac, IM composition string is not committed when another window in same process is activated. It should be committed. IM composition string is not committed when another application is activated, but it's not a problem. AFAIK it's a spec of Bug 327003. I don't see any other problem now.
Assignee | ||
Comment 123•15 years ago
|
||
> On Windows, with intl.enable_tsf_support is false (default),
> IME composition string is not committed when another window is activated. It
> should be committed.
> On Mac,
> IM composition string is not committed when another window in same process is
> activated. It should be committed.
> IM composition string is not committed when another application is activated,
> but it's not a problem. AFAIK it's a spec of Bug 327003.
Can you explain what I should be seeing in these cases in more detail? I don't see any difference in behaviour when switching windows between builds with this patch and those without.
Comment 124•15 years ago
|
||
(In reply to comment #123) > > On Windows, with intl.enable_tsf_support is false (default), > > IME composition string is not committed when another window is activated. It > > should be committed. 1. Choose MS-IME (or Microsoft IME). 2. Focus <input>, <textarea> or something. 3. Choose Hiragana in Input Mode icon. Input Mode icon appears as "A", "あ" or something. Hiragana mode is "あ". 4. Type some text. The inputted string should be underlined. 5. Switch to another window. E.g. click on another window. Result with patch: The text of 4. is still underlined. Result without patch: The text of 4. is not underlined after switching window. > > On Mac, > > IM composition string is not committed when another window in same process is > > activated. It should be committed. 1. Enable Kotoeri in System Preferences -> International -> Input Menu. 2. Focus <input>, <textarea> or something. 3. Choose Hiragana in input menu in the menubar. 4. Type some text. The inputted string should be underlined. 5. Switch to another window in the same application. E.g. Choose File -> New Window by mouse. Result with patch: The text of 4. is still underlined. Result without patch: The text of 4. is not underlined. > > IM composition string is not committed when another application is activated, > > but it's not a problem. 1. Enable Kotoeri in System Preferences -> International -> Input Menu. 2. Focus <input>, <textarea> or something. 3. Choose Hiragana in input menu in the menubar. 4. Type some text. The inputted string should be underlined. 5. Switch to another application. E.g. click on another application. Result with and without patch: The underlined text of 4. is still underlined.
Assignee | ||
Comment 125•15 years ago
|
||
> The one thing I wonder about is how the :focus pseudo-class ought to interact > with the somewhat tree-like nature of focus. That said, I don't know if there > are ever two nodes in the tree in the same document... There is only ever one focused node in the entire application focused at a time. Each window, however maintains a reference to a content node that would be focused if that window was focused. If that node happens to be an iframe and the toplevel window is focused, it means that the the real focus in within that child frame. The node in the child frame is focused (and has had SetContentState(childframeNode) called), but the iframe itself is not actually focused. The references are used to navigate to the real focus and, for example, drill down to find out what to restore the focus to when a window is raised. > although I wonder if that really ought to be the case (e.g., should > scrollbars from 'overflow:scroll' really behave differently from > scrollbars from an iframe?). I don't follow what you're asking here.
Assignee | ||
Comment 126•15 years ago
|
||
This patch addresses the comments and does this following: - removed some unnecessary QI usage - added cycle collection to the focus manager - added some null-checks to the MoveFocus/ClearFocus methods - fixed the problem with test_bug430351.html - update for new event suppression during synchronous task code - move select look and feel flag initializetion code in nsHTMLInputElement so that parsing html from xpcshell doesn't fail - fix Linux memory leaks by updating the focus window in nsWindow.cpp properly - fixed a leak in nsPrintEngine.cpp - fix IME issues - update command dispatcher code, it was setting the focus on the current window instead of the root window - added command dispatcher tests - added the 'mouse' flag for the context menu focusing code (nsXULPopupListener.cpp) The patch now passes all tests without leaks on the Linux tryserver.
Attachment #367540 -
Flags: review?(Olli.Pettay)
Comment 127•15 years ago
|
||
Neil, I've tested your v3 patch on OS X 10.5.6 and found one problem: Your new test_focus.xul Chrome test has 35 failures. This is true whether it's run by itself or with all the other Chrome tests. All the Minefield-specific tests at bug 314160 comment #19 still pass. The crash running the 473284.xul crashtest (stack trace in comment #95) no longer happens.
Assignee | ||
Comment 128•15 years ago
|
||
OK, thanks. I didn't test the new command dispatcher checks I added with the different cases of the keyboard access pref set. I'll update the test a bit but that should be the only change needed.
Comment 129•15 years ago
|
||
test_focus.xul does still depend on whether mouse is at the place where the new window opens (on linux). No leaks when running chrome/browser-chrome/mochitest.
(In reply to comment #125) > There is only ever one focused node in the entire application focused at a > time. Each window, however maintains a reference to a content node that would > be focused if that window was focused. Is it the latter that's used for matching :focus? > If that node happens to be an iframe and > the toplevel window is focused, it means that the the real focus in within that > child frame. The node in the child frame is focused (and has had > SetContentState(childframeNode) called), but the iframe itself is not actually > focused. The references are used to navigate to the real focus and, for > example, drill down to find out what to restore the focus to when a window is > raised. So if the parent document has :focus { border-color: green; } and the focus is somewhere inside an iframe, does the iframe match the :focus style? (What's the behavior before the patch and with?)
Comment 131•15 years ago
|
||
Comment 132•15 years ago
|
||
Comment on attachment 367540 [details] [diff] [review] focus patch v3 r-, especially because of delayed event handling.
Attachment #367540 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 133•15 years ago
|
||
(In reply to comment #130) > > There is only ever one focused node in the entire application focused at a > > time. Each window, however maintains a reference to a content node that would > > be focused if that window was focused. > > Is it the latter that's used for matching :focus? The former. Only one node will match :focus at any given time. Are there cases where this shouldn't be the case? > So if the parent document has :focus { border-color: green; } and the focus is > somewhere inside an iframe, does the iframe match the :focus style? The iframe does not match, and this is the same behaviour as is currently the case. The only difference I've noticed is that current code doesn't clear the :focus style when another window is raised, while with this patch, it does. But that's actually a bug in the current code.
Assignee | ||
Comment 134•15 years ago
|
||
> Does this case happen only with <input type="file">? If so, move this > to nsGenericInputElement::PreHandleEvent. Or do we get focus event also > with other form elements? This code is used for all form controls. > Have you tested that :focus works still correctly in CSS? It does, but that wouldn't relate to this code anyway. > Does this cause any changes to 'change' event which <select> fires in its > ::SetFocus() I don't see any difference. The change event fires before the blur as is the case currently. > nsresult > nsHTMLLabelElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor) > { > if (mHandlingEvent || > - (!NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) && > - aVisitor.mEvent->message != NS_FOCUS_CONTENT) || > + !NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) || > aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault || > !aVisitor.mPresContext) { > return NS_OK; > } > So you handle possible recursion somewhere else? Not sure what you're referring to here. The for attribute referring to the same node itself? > + > + // make sure that the element is really focused in case an earlier > + // listener in the chain changed the focus. > + if (editableRoot) { > + nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > + NS_ENSURE_TRUE(fm, NS_OK); > + > + nsCOMPtr<nsIDOMElement> element; > + fm->GetFocusedElement(getter_AddRefs(element)); > + if (!SameCOMIdentity(element, target)) > + return NS_OK; > + } > Why you move this here? If I recall, this code is needed to fix bug 246576, yet changing the code yet leaving it where it was caused an issue where the caret wasn't appearing in design mode documents. In a designmode document, the caret should be shown despite even though no content is focused. > So we don't need activation event hereto set window's state? > This doesn't regress bug 480768 or bug 480767? Activation/Deactivation events don't go through the presshell any more, they only go to the webshell which calls the focus manager directly.
Comment 135•15 years ago
|
||
(In reply to comment #134) > > So you handle possible recursion somewhere else? > > Not sure what you're referring to here. The for attribute referring to the same > node itself? The reason for mHandlingEvent, Bug 401160. Though that has in-testsuite+
Attachment #364039 -
Flags: review?(dbaron) → review+
Comment on attachment 364039 [details] [diff] [review] focus patch v2 OK, r=dbaron on the issues I was asked to review (see comment 80, comment 86, comment 105, comment 117, comment 125, comment 130, comment 133 -- though primarily comment 105)
Assignee | ||
Comment 137•15 years ago
|
||
- addressed comments: - changed document.activeElement to just check the owner document - renamed ContentInvalid to ContentRemoved - added a presshell argument to the delayed events structure - add cycle collection for nsGlobalWindow::mFocusedNode - several minor code adjustments - changed mActiveWindow to be an nsPIDOMWindow - removed an unnecessary docshell SetVisibility call in nsGlobalWindow::Focus - added a check when lowering a window so that the same window cannot be raised while it is happening -- otherwise the OS can get confused as to which window is actually in front. This caused an issue where the More Info button in the identity popup was opening a window behind the browser. - added a Focus method to the html label element since it wasn't redirecting focus properly. Added a test for this. - modified the nsHTMLLabelElement so that redirected focus from clicks is treated as a key focus. Both this and the previous item were found while testing the comment about labels. - modified the selection code a bit in nsHTMLInputElement - fixed a Mac leak on the browser-chrome tests. It was caused because the window lowered notification never fires when nsIAppStartup::eForceQuit is used to quit. This is the changes in nsFocusManager::WindowLowered
Attachment #362321 -
Attachment is obsolete: true
Attachment #364039 -
Attachment is obsolete: true
Attachment #367540 -
Attachment is obsolete: true
Attachment #370650 -
Flags: review?(Olli.Pettay)
Comment 138•15 years ago
|
||
Assignee | ||
Comment 139•15 years ago
|
||
> Aren't you removing the functionality of CheckAncestryAndGetFrame call here? > .activeElement should apparently return the frame element, if active element > is in some subframe. Passing PR_FALSE to nsFocusManager::GetFocusedDescendant means only get the focus from that window, whereas passing PR_TRUE would be used to get nodes down if child documents. In the former case, if a child is focused, the frame element is returned. So it already does what CheckAncestryAndGetFrame is doing. > Hmm, this gets the global "focused element", right? > And you replace mCurrentFocus with GetFocusedContent(). This means > that mCurrentTargetContent may start to point to some element outside mDocument. > Does this change the behavior of mousescroll for example? On OSX > focus may be in one window, but mousescroll is dispatched to another window. > (that is the behaviour of OSX applications, not only gecko's) > I guess it doesn't change that behavior, but maybe something else. > Anyway, something to think about. I'll change this so that only GetContentState uses this technique, and changed GetFocusedContent to just get the focus in that window. > What happens if one tries to focus <input type="file" style="display: none;">? > I guess nothing, and that is the current behavior too? Elements with 'display: none' aren't focusable. This check is made is nsFrame::IsFocusable, and hasn't been changed. The current behaviour of: <input type="file" style="display: none;"><button onclick="this.previousSibling.focus();">Fred</button> is that clicking the button clears the focus. With this patch, the focus remains on the button. Not sure if there's a reason for one behaviour or the other, but the latter seems more logical to me -- it's also what Safari does. IE throws an exception upon trying to focus a hidden element, so there isn't a compatibiliy issue here. > Does this actually change the behaviour if someone sets new focused element? > GetFocusController() returns the controller of the WindowRoot, and each > window has its own WindowRoot. FocusManager is global. > So the testcase could be something where there are 2 or more windows and > SetFocusedElement is called on not-focused-window. I don't follow this comment. There's one WindowRoot per top-level chrome window. If a window isn't active, SetFocus changes what would be focused in that window, but doesn't change anything in the active window. Similarly, if this is an inactive tab. GetRootFocusedContentAndWindow retrieves the window for that top-level chrome window and clears it. commandDispatcher.focusedElement is the element in the toplevel window or descendant that is focused. The behaviour should be the same, except that now it isn't incorrect in some cases like before ;) > Same question here. My changes here were slightly incorrect as setting the focus manager's focusedWindow was raising and it should not have been. I will fix this.
Comment 140•15 years ago
|
||
> This check is made is nsFrame::IsFocusable You can't possibly be checking for display:none in that method. display:none means there is no frame. > There's one WindowRoot per top-level chrome window. Or top-level content window in the embedding case, no? Not sure whether that affects the analysis.
Assignee | ||
Comment 141•15 years ago
|
||
(In reply to comment #140) > > This check is made is nsFrame::IsFocusable > > You can't possibly be checking for display:none in that method. display:none > means there is no frame. True. I was confusing that with visibility with is checked there. > > There's one WindowRoot per top-level chrome window. > > Or top-level content window in the embedding case, no? Not sure whether that > affects the analysis. Yes, I don't think it affects this case though.
Comment 142•15 years ago
|
||
Updated•15 years ago
|
Attachment #370650 -
Flags: review?(Olli.Pettay) → review-
Comment 143•15 years ago
|
||
Comment on attachment 370650 [details] [diff] [review] focus patch v4 I'd like to see still the coming changes to ESM.
Assignee | ||
Comment 144•15 years ago
|
||
+nsIContent* +nsGlobalWindow::GetFocusedNode() +{ + FORWARD_TO_INNER(GetFocusedNode, (), NS_OK); + + return mFocusedNode; +} > Is it guaranteed that mFocusedNode is in document? Yes, I'll change the aseertion in SetFocusedNode to use GetCurrentDoc. Also, nsFocusManager::ContentRemoved calls SetFocusedNode(nsnull) when focused content is removed. > When navigating back to the page, the focus should be in the same element > where it was before the page was bfcached. OK, I'll remove that.
Assignee | ||
Comment 145•15 years ago
|
||
Attachment #370650 -
Attachment is obsolete: true
Attachment #371732 -
Flags: superreview?(roc)
Attachment #371732 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 146•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 Neil to review toolkit changes (print preview changes for example)
Attachment #371732 -
Flags: review?(neil)
Assignee | ||
Updated•15 years ago
|
Attachment #371732 -
Flags: review?(mconnor)
Assignee | ||
Comment 147•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 mconnor to review browser changes
Assignee | ||
Comment 148•15 years ago
|
||
The changes in this patch from the last one: - address all comments - changed the recently added patch for bug 481560 as it was relying on firing keys at non-focused windows - added a test for focus handling during history navigation
Given that Olli has reviewed this, is going to finish reviewing it, is a super-reviewer, and is generally a better reviewer than me, should I really super-review this?
Updated•15 years ago
|
Attachment #371732 -
Flags: superreview?(roc)
Attachment #371732 -
Flags: superreview?(Olli.Pettay)
Attachment #371732 -
Flags: review?(Olli.Pettay)
Comment 150•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 Ok, if Neil reviews this too and mconnor checks at least the toolkit parts, I could sr.
Assignee | ||
Comment 151•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 should get an accessibility review too
Attachment #371732 -
Flags: review?(marco.zehe)
Comment 152•15 years ago
|
||
(In reply to comment #102) > Peter, can you create a build which has the two debugging defines near the top > of nsFocusManager.cpp enabled, and post or send me the log of what happens? Neil, I have _finally_ done this, with your latest patch. The log is attached here. I guess you can already tell this from the log, but all the entries with <<ClearFocus begin>> <<ClearFocus end>> <<SetFocusedWindow begin>> Shift Focus: (none) Flags: 0 Current Window: 0x22d4ce80 New Window: 0x209d2940 Current Element: 0 In Active Window: 0 In Focused Window: 0 are the ones where the focus didn't arrive in the entry field of the first window. Only the very last entry (after I Shift-reloaded the problematic page) is connected to a successful switch into the entry field. > It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel > as is done on Windows. I have looked at this for some (short) time. I couldn't construct such a function for OS/2 so far. Will look at this again, but there are also many other things about focus throughout os2/nsWindow.cpp which might be interfering.
Assignee | ||
Comment 153•15 years ago
|
||
(In reply to comment #152) > Created an attachment (id=371931) [details] > OS/2 problem log OK, it looks as if clicking the mouse on an window in the background isn't informing of the window being activated properly. I'm guessing that WM_ACTIVATE works a bit differently on os2 than on windows and that it gets sent to child windows as well (on windows it only gets sent to toplevel windows) If that's the case, then I think my original suspicion about using DispatchFocusToTopLevel was correct. The change to nsFrameWindow.cpp shouldn't have been done, and it should instead be something like: case WM_ACTIVATE: DEBUGFOCUS(frame WM_ACTIVATE); if (SHORT1FROMMP(mp1) && !(WinQueryWindowULong(mFrameWnd, QWL_STYLE) & WS_MINIMIZED)) { - bDone = DispatchFocusToTopLevel(NS_GOTFOCUS, PR_TRUE); + bDone = DispatchFocusToTopLevel(NS_GOTFOCUS); } break; where DispatchFocusToTopLevel is a function which retrieves the top level window, resets the gJustGotActivate flag and calls DispatchFocus, similar to the Windows DispatchFocusToTopLevel method this patch adds. I don't know how to retrieve the toplevel window on os2 though.
Please don't spend a lot of time trying to make this work on OS/2.
Assignee | ||
Comment 155•15 years ago
|
||
Yeah, to clarify, I don't plan on implementing it myself (as I don't know how and wouldn't be able to test it), but would incorporate any changes needed.
Comment 156•15 years ago
|
||
I'm waiting others to review before doing the (hopefully) final sreview.
Comment 157•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 >+ // blur the field to ensure that the popup is closed and that the previous >+ // search has stopped, then start a new search. >+ autocomplete.blur(); Out of interest, what does blur() do these days? Does it actually work given that it's the inner input that actually has focus? >- synthesizeMouse(document.documentElement, 1, 1, { }); >- >+ // the first tab focuses the document so skip it Why this change? Can you be sure where the focus is?
Comment 158•15 years ago
|
||
In a SeaMonkey build against mozilla-central with the patch, the form fill popup causes focus to be lost. This is particularly annoying when deleting characters from the end of the field, as the next backspace then goes back in history...
Assignee | ||
Comment 159•15 years ago
|
||
(In reply to comment #157) > Out of interest, what does blur() do these days? Does it actually work given > that it's the inner input that actually has focus? If the element is focused, blur() clears the focus from the document. The focus manager retargets focus/blur events from textboxes to their inputs. See nsFocusManager::GetRedirectedFocus > >- synthesizeMouse(document.documentElement, 1, 1, { }); > >- > >+ // the first tab focuses the document so skip it > Why this change? Can you be sure where the focus is? The synthesizeMouse line isn't necessary as the document is already focused -- the tests run 'onfocus'. Before it just didn't work too well.
Neil, mconnor, marcoz: please review this soon! I think my compositor widget-removal work is depending on it...
Comment 161•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 >diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp >--- a/accessible/src/base/nsAccessible.cpp >+++ b/accessible/src/base/nsAccessible.cpp >@@ -1453,17 +1453,16 @@ nsAccessible::TakeFocus() > > nsCOMPtr<nsIDOMNSHTMLElement> htmlElement(do_QueryInterface(content)); > if (htmlElement) { > // HTML Elements also set the caret position > // in order to affect tabbing order > return htmlElement->Focus(); > } > >- content->SetFocus(GetPresContext()); Why was this fallback removed? r=me with this explained/fixed-if-needed for the accessibility part.
Attachment #371732 -
Flags: review?(marco.zehe) → review+
Comment 162•15 years ago
|
||
(In reply to comment #160) > Neil, mconnor, marcoz: please review this soon! I think my compositor > widget-removal work is depending on it... And I want this in before clone-documents-for-printing (which blocks remove-support-for-multiple-presshells, and that would make event handling cleanup easier.)
Comment 163•15 years ago
|
||
(In reply to comment #159) > (In reply to comment #157) > > Out of interest, what does blur() do these days? Does it actually work given > > that it's the inner input that actually has focus? > If the element is focused, blur() clears the focus from the document. So where does focus go? > > >- synthesizeMouse(document.documentElement, 1, 1, { }); > > >- > > >+ // the first tab focuses the document so skip it > > Why this change? Can you be sure where the focus is? > The synthesizeMouse line isn't necessary as the document is already focused -- > the tests run 'onfocus'. Before it just didn't work too well. So if the document is already focused why does the first tab focus the document? > seems like the autocomplete panel in navigator.xul needs noautofocus="true" Any idea why it didn't need it before? Where was noautofocus implemented, and does the default depend on whether the element is a panel or not?
Assignee | ||
Comment 164•15 years ago
|
||
(In reply to comment #163) > So where does focus go? The document/window remain focused, but no element is focused, like what happens when clicking a blank area of a web page. The next tab key will just start from the beginning. > > the tests run 'onfocus'. Before it just didn't work too well. > So if the document is already focused why does the first tab focus the > document? It should probably say 'document root', as in the state where a focus ring appears around the document body when you tab into it. Well, it would do if was an html document. In xul, nothing of note gets drawn. > Any idea why it didn't need it before? Where was noautofocus implemented, and > does the default depend on whether the element is a panel or not? Don't know why it didn't before. It's handled in nsXULPopupManager::FirePopupShowingEvent. It may be that it needs to look at the current focus only within the same document. I'll investigate a bit more, but noautofocus should be used anyway, I would think.
Assignee | ||
Comment 165•15 years ago
|
||
(In reply to comment #164) > Don't know why it didn't before. It's handled in > nsXULPopupManager::FirePopupShowingEvent. It may be that it needs to look at > the current focus only within the same document. I'll investigate a bit more, > but noautofocus should be used anyway, I would think. I think this is because nothing is focused within the document the popup is within. (The existing code is wrong I think.) But focus should be cleared when the panel opens unless it has the noautofocus attribute, so that focus can move to the panel. In this case, the autocomplete panel doesn't need focus directly and wants it kept on the page, so it should be using noautofocus.
Comment 166•15 years ago
|
||
(In reply to comment #159) > The focus manager retargets focus/blur events from textboxes to their inputs. > See nsFocusManager::GetRedirectedFocus Isn't that a little delicate? What if someone wants to create a new widget which wraps an existing focusable element? > > >- synthesizeMouse(document.documentElement, 1, 1, { }); > > >- > > >+ // the first tab focuses the document so skip it > > Why this change? Can you be sure where the focus is? > The synthesizeMouse line isn't necessary as the document is already focused -- > the tests run 'onfocus'. Before it just didn't work too well. Sorry, I'm not following - I still can't see how the document can already have focus yet the first tab focuses the document. (In reply to comment #165) > > but noautofocus should be used anyway, I would think. > I think this is because nothing is focused within the document the popup is > within. (The existing code is wrong I think.) OK, makes sense.
Assignee | ||
Comment 167•15 years ago
|
||
(In reply to comment #166) > (In reply to comment #159) > > The focus manager retargets focus/blur events from textboxes to their inputs. > > See nsFocusManager::GetRedirectedFocus > Isn't that a little delicate? What if someone wants to create a new widget > which wraps an existing focusable element? Indeed, which is why I plan on changing it very soon afterwards. > Sorry, I'm not following - I still can't see how the document can already have > focus yet the first tab focuses the document. Before pressing tab, nothing in the document is focused, but the document still receives key events. After pressing tab, the document root is focused (in html a focus ring appears around the entire document).
Comment 168•15 years ago
|
||
> Before pressing tab, nothing in the document is focused, but the document still
> receives key events. After pressing tab, the document root is focused (in html
> a focus ring appears around the entire document).
So that's a deliberate change from the current behaviour? Currently when you follow a link, the focus moves to the new document (but the focus ring does not appear around the document because the focus was not moved by the tab key), and tabbing takes you to the first tabbable element in the new document.
Attachment #371732 -
Flags: review?(mconnor) → review?(gavin.sharp)
Comment on attachment 371732 [details] [diff] [review] focus patch v5 mconnnor says Gavin agreed to do this
Comment 170•15 years ago
|
||
Comment on attachment 371732 [details] [diff] [review] focus patch v5 (Still no answer to comment #168.) This blew up on me when I tried to find in page on a bugzilla attachment details page. Steps to reproduce problem: 1. Edit a bugzilla attachment 2 [review]. Click Edit attachment as comment At this point focus is dead. Worse is to come though if you built SeaMonkey using comm-central on mozilla-central plus this patch. 3. Since focus is dead, use the mouse to open the find dialog (Edit - Find in this Page) 4. Try to find something. 5. Watch find crash as it fails to find a frame for the focused element.
Attachment #371732 -
Flags: review?(neil) → review-
Assignee | ||
Comment 171•15 years ago
|
||
Addresses comments. Changes in this version: - address accessibility comments (nsAccessible.cpp) - modify utilityOverlay.js focusElement() so that focus() isn't called on a window but instead on its frame. This was causing additional work to raise the window earlier than it should which have which affected the performance tests. - fix up the comment in test_tabindex.xul - had to fix up the newly added ime tests added in bug 460059 to not use nsDOMWindowUtils::Focus and adjust because ime state shouldn't change for designMode documents since they don't allow focusing html elements - adjusted the input type="file" focus() method to focus the button instead of the file. This was also an issue with the ime tests from bug 460059 but also happens to fix bug 453377 as well. - had to fix up the download manager test test_select_all.xul as it was failing on linux - addressed Neil's comment 168 about the first tab on a document (change in nsFocusManager::DetermineElementToFocus) - addressed Neil's comment 170. Null-pointer crash in nsWebBrowserFind.cpp and change to nsPresShell::HandleEvent to use the root node if the currently focused content no longer has a frame - Masayuki pointed out that the documentation for nsDOMWindowUtils::Focus says that passing null should clear the focus so added this, even though the method is deprecated. - removed a stray line left over from DocumentViewerImpl::Close
Attachment #371732 -
Attachment is obsolete: true
Attachment #376622 -
Flags: superreview?(Olli.Pettay)
Attachment #376622 -
Flags: review?(Olli.Pettay)
Attachment #371732 -
Flags: superreview?(Olli.Pettay)
Attachment #371732 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #376622 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #376622 -
Flags: review?(neil)
Comment 172•15 years ago
|
||
Comment on attachment 376622 [details] [diff] [review] focus patch v6 Some oddities observed while testing this patch: * A quick "click" on the taskbar is insufficient to switch between separate windows of the same application. (It's not easy to reproduce because your click has to be short and precise but I can't reproduce it at all on a build without the patch.) * Calling focus() on a window raises the window but kills focus (same effects as edit attachment used to have, but without the find crash). * The find dialog focuses the page that you're finding in, when it should retain focus. >+function focusElement(aElement) There seem to be three callers of this: * Focusing the URLbar on a timeout (which may have allowed the window to lose activation) which looks as if it should be replaced with a simple focus call. * Focusing the active tab, which maybe should be renamed focusContent() ? * And one rather sneaky use, which is to transfer focus to the active tab of another window, but only if that window didn't have a popup. I don't think your replacement caters for that! >+ // if a content window, focus the <browser> instead as window.focus() >+ // raises the window >+ if (aElement instanceof Window) >+ getBrowserFromContentWindow(aElement).focus(); This interestingly seems to focus the previously focused content element, i.e. * Focus somewhere in content * Press Ctrl+L * Press Ctrl+Shift+J * Evaluate top.opener.getBrowser().selectedBrowser.focus() I can see that being useful :-) >+ // otherwise, if there is no focused content, just use the root content >+ if (!mCurrentEventContent) >+ mCurrentEventContent = mDocument->GetRootContent(); >+ mCurrentEventFrame = nsnull; >+ >+ // if the focused content has no frame, use the root content as well. >+ // This ensures that key events still get sent properly if a frame goes >+ // away while the frame is focused. >+ if (!GetCurrentEventFrame()) >+ mCurrentEventContent = mDocument->GetRootContent(); >+ mCurrentEventFrame = nsnull; Can't you write this as if (!mCurrentEventContent || !GetCurrentEventFrame()) mCurrentEventContent = mDocument->GetRootContent(); since if the root content doesn't have an event frame then it's not going to help to get the root content again ;-)
Assignee | ||
Comment 173•15 years ago
|
||
(In reply to comment #172) > * A quick "click" on the taskbar is insufficient to switch between separate > windows of the same application. (It's not easy to reproduce because your click > has to be short and precise but I can't reproduce it at all on a build without > the patch.) Can't reproduce this myself. What platform/os is this? > * Calling focus() on a window raises the window but kills focus (same effects > as edit attachment used to have, but without the find crash). How do I reproduce this? > * The find dialog focuses the page that you're finding in, when it should > retain focus. Modified nsWebBrowserFind.cpp to fix this > * And one rather sneaky use, which is to transfer focus to the active tab of > another window, but only if that window didn't have a popup. I don't think your > replacement caters for that! Modified the code here to hopefully handle this last case > Can't you write this as > if (!mCurrentEventContent || !GetCurrentEventFrame()) > mCurrentEventContent = mDocument->GetRootContent(); > since if the root content doesn't have an event frame then it's not going to > help to get the root content again ;-) Fixed
Attachment #376740 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #376740 -
Flags: review? → review?(neil)
Comment 174•15 years ago
|
||
Comment on attachment 376740 [details] [diff] [review] this patch applies on top of the other one and fixes most of Neil's issues This fixes the find bug; I can't test the other code. I was also unable to reproduce my odd taskbar issue.
Attachment #376740 -
Flags: review?(neil) → review+
Comment 175•15 years ago
|
||
I tripped over another crash today. As I recall, I was dismissing an alert while a page was loading in the background, and when the previous window activated the focused element's document no longer had a window to raise.
Comment 176•15 years ago
|
||
I crashed in a build with this patch today, here's the stack. It's the only crash I've seen since applying it several days ago.
I have windowless IFRAMEs working pretty well ... except for focus. Please please let's get this landed!
Assignee | ||
Updated•15 years ago
|
Comment 178•15 years ago
|
||
Comment on attachment 379612 [details] crash stack >bug Thread 0 Crashed: >0 libgklayout.dylib 0x12234f4d nsFocusManager::SetFocusInner(nsIContent*, int, int) + 221 Is it possible that newWindow is null here: nsCOMPtr<nsIDocShell> newDocShell = newWindow->GetDocShell(); GetCurrentWindow() may return null.
Assignee | ||
Updated•15 years ago
|
Comment 179•15 years ago
|
||
Updated•15 years ago
|
Attachment #376622 -
Flags: superreview?(Olli.Pettay)
Attachment #376622 -
Flags: superreview-
Attachment #376622 -
Flags: review?(Olli.Pettay)
Attachment #376622 -
Flags: review-
Comment 180•15 years ago
|
||
Comment on attachment 380503 [details] comments nsXULCommandDispatcher::GetSuppressFocusScroll(PRBool* aSuppressFocusScroll) > { >+ return NS_OK; > } Shouldn't this return a value of some sort?
Comment 181•15 years ago
|
||
Comment on attachment 376622 [details] [diff] [review] focus patch v6 I no longer have a tree to which this applies. Would you mind posting the latest update please?
Attachment #376622 -
Flags: review?(neil)
Assignee | ||
Comment 182•15 years ago
|
||
> + // XXXndeakin P3 accessibility shouldn't be caching the focus > Please file a bug and add the bug number here. Filed bug 495624. > Either change the name of the method to not contain word "Element", or at least > file a new bug to do that. I'll file a new bug. Some code or extension may be using this method. > + // If the legend isn't focusable, focus whatever is focusable following > + // the legend instead, bug 81481. > Is there a testcase for this? Even just manual test? Yes, in window_focus.xul > + // XXXndeakin even though this is only used for key events which should be > + // going to the focused frame anyway, this doesn't seem like the right way > + // to determine if something is an editor. > Please file a new bug to fix this. Add bug number here. Filed bug 495625. > Why this change? If something needs now a timeout which wasn't needed before, > that sounds like a bug. See comment 49.
Assignee | ||
Comment 183•15 years ago
|
||
Changes: browser_tabfocus.js - focused the window in the new browser_tabfocus.js test before starting the test. An earlier test for some reason still has a window open and it was interfering with this one. nsDocument::GetActiveElement - changed to use NS_WARNING nsDocument::SuppressEventHandling - update for latest event suppression changes nsXULCommandDispatcher::GetSuppressFocusScroll - set return value correctly nsFocusManager::SetFocusInner - check for null newWindow nsIPresShell.h - update for latest event suppression changes nsPresShell.cpp::nsDelayedFocusBlur - remove, update for latest event suppression changes PresShell::HandleEvent - update for latest event suppression changes PresShell::FireOrClearDelayedEvents - remove, update for latest event suppression changes browser_passwordmgrdlg.js - changed to use focus events rather than load events plus the changes from attachment 376740 [details] [diff] [review].
Attachment #376622 -
Attachment is obsolete: true
Attachment #376740 -
Attachment is obsolete: true
Attachment #380854 -
Flags: superreview?(Olli.Pettay)
Attachment #380854 -
Flags: review?(Olli.Pettay)
Attachment #376622 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #380854 -
Flags: review?(gavin.sharp)
Comment 184•15 years ago
|
||
Updated•15 years ago
|
Attachment #380854 -
Flags: superreview?(Olli.Pettay)
Attachment #380854 -
Flags: superreview+
Attachment #380854 -
Flags: review?(Olli.Pettay)
Attachment #380854 -
Flags: review+
Comment 185•15 years ago
|
||
Comment on attachment 380854 [details] [diff] [review] focus patch v7 This will probably cause some regressions, but better to notice them as early as possible.
Comment 186•15 years ago
|
||
it would be nice to get this on trunk when the tree is no longer restricted it fixes a ton of bugs in my view
Comment 187•15 years ago
|
||
Actually, I think it should land during the restriction, or a closed tree, to keep the noise to a minimum, to find any possible reggressions on the 'tree'.
Indeed, landing while the tree is in the restricted state it is now (we'd need special permission since this isn't a 1.9.1 blocker) would help a lot with tracing regressions since we carry nightlies much longer than hourlies.
Comment 189•15 years ago
|
||
anybody care to contact one of the devs or whoever to get such permision
Comment 190•15 years ago
|
||
The patch still needs review (from Gavin) before that can happen. And the patch in the bug this bug depends on needs to land. Once this is set to land, I do think we should close the tree for a day to let it land. Please post to .planning (and cc .platform) to schedule that once it's clear when this will be ready? If this is ready before we've reopened m-c, probably best to just land before reopening. If not, we shouldn't hold the tree closed for days waiting on this; better to close it as needed when this patch is ready.
Comment 191•15 years ago
|
||
We'll get this on mozilla-central once we've frozen for RC1 on mozilla-1.9.1. Not ideal for everyone, but we need to focus on releasing software. We can keep the tree closed for this landing only if you'd like. bz's gonna ask around in the newsgroups. There is no way this is going onto 1.9.1, not that I think any serious-thinking person has proposed that.
Comment 192•15 years ago
|
||
Patch v7 crashes on Linux when enabling caret browsing, STR: 1. press F7, click OK => crash #6 in nsFocusManager::UpdateCaret 1622 nsCOMPtr<nsIDocShell> focusedDocShell = mFocusedWindow->GetDocShell(); (gdb) p mFocusedWindow $1 = {mRawPtr = 0x0} Also, there's a new warning when clicking on text: WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp, line 335
Assignee | ||
Comment 193•15 years ago
|
||
> Nit, this could be nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(parent); return window.forget(); Will fix this. > This is not what happens currently. We don't dispatch focus or blur events > either if window is blurred because some other window is focused. > Only focus and blur events which are caused by some script are dispatched > immediately even if the document is suppressed. > So, is the comment just a bit wrong or are you actually changing the behavior. > And if you're changing the behavior, is there a reason for that, or is it > just a bug? Discussed with smaug, and it's just the comment that is misleading which I'll adjust. The comment only applies to the focus event, blur always passes non-zero for aFocusMethod. > + // XXXndeakin this doesn't seem right. It should be checking for this only > + // on the nearest ancestor frame which is a chrome frame. But this is > + // what the existing code does, so just leave it for now. > File a bug for this. Filed bug 496307 > #6 in nsFocusManager::UpdateCaret > 1622 nsCOMPtr<nsIDocShell> focusedDocShell = > mFocusedWindow->GetDocShell(); > (gdb) p mFocusedWindow > $1 = {mRawPtr = 0x0} I added a null check here. > Also, there's a new warning when clicking on text: > WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp, > line 335 I'll investigate this.
Comment 194•15 years ago
|
||
(In reply to comment #193) > > #6 in nsFocusManager::UpdateCaret > > 1622 nsCOMPtr<nsIDocShell> focusedDocShell = > > mFocusedWindow->GetDocShell(); > > (gdb) p mFocusedWindow > > $1 = {mRawPtr = 0x0} > > I added a null check here. Why is it null?
Comment 195•15 years ago
|
||
(In reply to comment #175) > I tripped over another crash today. As I recall, I was dismissing an alert > while a page was loading in the background, and when the previous window > activated the focused element's document no longer had a window to raise. I tripped over what I think was the same crash again today. In this case I was dismissing a slow script alert caused by a jquery unload handler. I crashed at nsPIDOMWindow::GetDocShell (this = 0x0) called from nsFocusManager::IsWindowVisible (aWindow = 0x0) called from nsFocusManager::CheckIfFocusable (aContent = <xul:browser>, aFlags = 0x0)
Comment 196•15 years ago
|
||
I've just noticed that accelerators for radio buttons don't move focus...
Assignee | ||
Comment 197•15 years ago
|
||
> > Also, there's a new warning when clicking on text: > > WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp, > > line 335 > > I'll investigate this. I'll add a check for this in nsFocusManager::Blur so that GetPrimaryFrameFor isn't called with a null argument. > I tripped over what I think was the same crash again today. In this case I was > dismissing a slow script alert caused by a jquery unload handler. I crashed at > nsPIDOMWindow::GetDocShell (this = 0x0) called from > nsFocusManager::IsWindowVisible (aWindow = 0x0) called from > nsFocusManager::CheckIfFocusable (aContent = <xul:browser>, aFlags = 0x0) Perhaps another case where a window isn't available. I'll add a null-check if avoid the crash.
Assignee | ||
Comment 198•15 years ago
|
||
(In reply to comment #196) > I've just noticed that accelerators for radio buttons don't move focus... Should be an easy enough fix. It's another case where code was assuming that non-focusable elements could be focused. I'll file a followup bug though since it's a minor issue.
Comment 199•15 years ago
|
||
Comment on attachment 380854 [details] [diff] [review] focus patch v7 I looked at the documentation first, some minor comments for nsIFocusManager.idl: - there are mixed references to node/element and frame/window, which is slightly confusing. I guess the frame/window distinction is important, but it's not immediately obvious to me. - "one application level focus" -> "one application-wide focused element" ? - "if its window was active" -> "if the window was active" ? -"the focus is moved to the window currently associated with that nsIDOMWindow" is phrased awkwardly - could just remove that sentence, I think? - for activeWindow's comment, "focuses whatever is currently the current node in the current child window" -> "focuses the child window's focused node"? - for moveFocus, "relative to aElement" -> "relative to aStartElement" - for getFocusedElementForWindow, "then this element will have the focus" is confusing - I think it's trying to say "the returned element will be the application-wide focused element", right? - for getFocusedElementForWindow, there's another "the element will be" - probably clearer as "the return value will be" ? - for getFocusedElementForWindow, perhaps add "or to aWindow if aDeep is false" to the last paragraph? some typos elsewhere in the documentation: "was is an" "heirarchy" "focusued" A lot of test files use the |let someFunc = function () {}| syntax rather than named functions. Can you change them to just be function someFunc(){} instead? >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >+ var fm = Components.classes["@mozilla.org/focus-manager;1"]. >+ getService(Components.interfaces.nsIFocusManager); >+ var focusedBrowser = fm.getFocusedElementForWindow(window, false, {}); focusedBrowser should probably be focusedChromeElement. >diff --git a/browser/base/content/test/browser_bug481560.js b/browser/base/content/test/browser_bug481560.js > function test() { > waitForExplicitFinish(); > >+ // focus the url field so that it will can ensure the focus is there when >+ // the window is refocused after the dialog closes >+ gURLBar.focus(); Why is that relevant to this test? Also not sure I understand why this test needs to use a focus event rather than just onload as it was before, can you explain? >diff --git a/browser/base/content/test/browser_tabfocus.js b/browser/base/content/test/browser_tabfocus.js >+function test() { >+ var urlbar = document.getElementById("urlbar"); >+ urlbar.focus(); nit: can just use gURLBar here >+ // blurring an element in the current tab should clear the active focus >+ expectFocusShift(function () button1.blur(), >+ browser1.contentWindow, null, true, >+ "focusedWindow after blur in focused tab"); >+ is(fm.getFocusedElementForWindow(browser1.contentWindow, false, {}), null, "blur in focused tab"); This is() check seems redundant, expectFocusShift already tests that right? >+ // switching focus to chrome when it has no focused element should clear the focus >+ expectFocusShift(function () fm.clearFocus(window), >+ window, null, true, >+ "focusedWindow after switch to chrome with no focused element"); comment doesn't seem to match what you're testing exactly. Is there any way for a user to trigger the equivalent of clearFocus()'s behaviour? seems like clearFocus' documentation should mention that it sets focus to the passed in window if it is an ancestor of the current focused window. >+var _browser_tabfocus_test_lastfocuswindow = ""; nit: why = ""? >+function expectFocusShift(callback, expectedWindow, expectedElement, focusChanged, testid) >+ var focusedElement = fm.focusedElement; >+ is(focusedElement ? getId(focusedElement) : "none", >+ expectedElement ? getId(expectedElement) : "none", testid + " focusedElement"); why can't this compare focusedElement and expectedElement directly? >diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js >+ // Focus the content but don't raise the window, since the URI we just loaded >+ // may have resulted in a new frontmost window (e.g. "javascript:window.open('');"). >+ if ("getBrowserFromContentWindow" in w) { When would this check fail? We already assume above that w has a gBrowser property, and getTopWin only returns navigator:browsers. >diff --git a/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js b/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js Same question here - why does this need to use a focus event now? >diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp >+ nsCOMPtr<nsIDOMElement> focusedElement; unused? >diff --git a/toolkit/content/tests/widgets/test_focus_anons.xul b/toolkit/content/tests/widgets/test_focus_anons.xul >+ gExpectedBlur = gExpectedFocus; >+ gExpectedFocus = document.getAnonymousNodes($("scale"))[0]; >+ synthesizeKey("c", accessKeyDetails); >+ >+ gExpectedBlur = document.getAnonymousNodes($("scale"))[0]; can use gExpectedFocus here, no? >diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml > <handler event="focus" phase="capturing"> > <![CDATA[ >- if (!this.hasAttribute('focused')) { >- if (event.originalTarget != this.inputField) >- this.inputField.focus(); >- this.setAttribute('focused','true'); >- } >+ this.setAttribute('focused','true'); > ]]> > </handler> Why is this change needed? The textbox binding has similar code - does it need changing too? >diff --git a/toolkit/themes/pinstripe/global/menulist.css b/toolkit/themes/pinstripe/global/menulist.css > menulist[editable="true"] { >- -moz-user-focus: normal; I have no idea how to evaluate the significance of this change. Seems like this has been in pinstripe since forever, but not on other platforms... what practical effect does it have? r=me on all the toolkit/ and browser/ parts, but I didn't review the typeaheadfind changes very carefully (I'm assuming Neil looked at them).
Attachment #380854 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 200•15 years ago
|
||
(In reply to comment #199) > >+ // focus the url field so that it will can ensure the focus is there when > >+ // the window is refocused after the dialog closes > >+ gURLBar.focus(); > > Why is that relevant to this test? This was an issue on Linux. The content area was focused, so the chrome window didn't receive a focus event. > Also not sure I understand why this test needs to use a focus event rather than > just onload as it was before, can you explain? The window isn't focused until the focus event fires, which can happen after the load event fires. Tests that rely on the window being focused should use the focus event instead. > why can't this compare focusedElement and expectedElement directly? So that if it fails, the element ids appear in the log rather than the less useful object name. > > <handler event="focus" phase="capturing"> ... > Why is this change needed? The textbox binding has similar code - does it need > changing too? > > menulist[editable="true"] { > >- -moz-user-focus: normal; > I have no idea how to evaluate the significance of this change. Seems like this > has been in pinstripe since forever, but not on other platforms... what > practical effect does it have? Both the editable menulist related changes are there to just treat the input as focusable and never the menulist itself. Currently, for example, there's a bug where shift+tab doesn't work when an editable menulist is focused.
Assignee | ||
Comment 201•15 years ago
|
||
- addressed all of Gavin's and Olli's comments. - added a null-check before calling GetPrimaryFrameFor in nsFocusManager::Blur. This fixes the FrameManager.cpp warning. - adjusted the test to account for element.localName returning lowercase values now from bug 468708. - added a null-check to account for Neil's comment 198.
Attachment #380854 -
Attachment is obsolete: true
Assignee | ||
Comment 202•15 years ago
|
||
Checked in! (http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3) Thanks all!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 203•15 years ago
|
||
Bustage fix: http://hg.mozilla.org/mozilla-central/rev/0102de270b20
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite+
Depends on: 499805
Comment 204•15 years ago
|
||
it seems this has caused the regression in bug 500275
Comment 205•15 years ago
|
||
Looks like this caused build bustage in bug 507056
Updated•15 years ago
|
Depends on: 517866
Comment 206•15 years ago
|
||
Is bug 504922 related to this refactoring of the focus handling? If so, would this be fixed in the Firefox 3.5.x branch?
Comment 207•15 years ago
|
||
Is bug 370031 covered by this?
Updated•14 years ago
|
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.2
Comment 208•12 years ago
|
||
Comment on attachment 382498 [details] [diff] [review] focus patch v8 >@@ -10025,30 +9905,30 @@ NS_IMETHODIMP nsDocShell::EnsureFind() > // we promise that the nsIWebBrowserFind that we return has been set > // up to point to the focused, or content window, so we have to > // set that up each time. > > nsIScriptGlobalObject* scriptGO = GetScriptGlobalObject(); > NS_ENSURE_TRUE(scriptGO, NS_ERROR_UNEXPECTED); > > // default to our window >- nsCOMPtr<nsIDOMWindow> rootWindow = do_QueryInterface(scriptGO); >- nsCOMPtr<nsIDOMWindow> windowToSearch = rootWindow; Both the window to search and the root window used to default to the *content* window... >+ nsCOMPtr<nsIDOMWindow> windowToSearch(do_QueryInterface(mScriptGlobal)); >+ >+ nsCOMPtr<nsIDocShellTreeItem> root; >+ GetRootTreeItem(getter_AddRefs(root)); ...but now the root window is the *chrome* window. Oops. In fact, it appears that the whole meaning of the code block has changed; as far as I can tell, the old code wanted to retrieve the per-window focused frame (OK, so it didn't work when calling EnsureFind on a background tab, but we never did that), which I believe is what nsFocusManager::GetFocusedDescendant does, although I don't see any obvious way to call it, while the new code only works if the chrome window has focus (again, we generally don't call EnsureFind on a background window either).
Comment 209•12 years ago
|
||
Neil: file a new bug. Commenting in a bug that's been closed (and uncommented in!) for 3 years is unhelpful, and unlikely to result in any action. You should know better.
Comment 210•12 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #209) > Neil: file a new bug. Commenting in a bug that's been closed (and > uncommented in!) for 3 years is unhelpful, and unlikely to result in any > action. You should know better. ... And let us know the bug number!
You need to log in
before you can comment on or make changes to this bug.
Description
•