Closed
Bug 413882
Opened 17 years ago
Closed 17 years ago
Google Docs almost unusable
Categories
(Core :: Widget: Cocoa, defect, P1)
Tracking
()
VERIFIED
FIXED
People
(Reporter: sdwilsh, Assigned: smichaud)
References
Details
(Keywords: dogfood, relnote)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
jaas
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
With google spreadsheets, when trying to enter anything into a cell, I frequently have to command + tab to make firefox lose focus, command + tab back, then press enter to get anything to work. Sometimes it works when I just press enter.
On irc someone had mentioned that there was a focus patch that had landed recently for cocoa widgets, and no windows user could confirm this. This is a fairly recent regression (within the last week), but I don't know when exactly.
Assignee | ||
Comment 1•17 years ago
|
||
OK, how do you get to "google spreadsheets"?
Assignee | ||
Comment 2•17 years ago
|
||
Nevermind, I figured it out.
But I'm not able to reproduce what you report.
I opened a new spreadsheet and typed stuff into the first cell -- no
problems. Then I moused to or tabbed to new (or existing) cells,
typed more text, and still had no problems.
Please provide more detail.
Reporter | ||
Comment 3•17 years ago
|
||
That doesn't even work for me. I'll try it in a clean profile later today, but I don't know how an addon could be messing with this :/
Comment 4•17 years ago
|
||
Shawn: Are you running Tiger or Leopard?
Assignee | ||
Comment 5•17 years ago
|
||
Marcia, thanks for the suggestion.
I can't reproduce this on Leopard, either.
So now I've tested (with today's Minefield nightly) on both Tiger and
Leopard -- no problems on either.
Reporter | ||
Comment 6•17 years ago
|
||
I'm on Tiger. I'll be around for the work week next week if that will help anyone.
Updated•17 years ago
|
Product: Firefox → Core
QA Contact: general → general
Assignee | ||
Comment 7•17 years ago
|
||
I'm not going to be at the work week, though Josh will.
But there currently isn't enough information in this bug to do
anything with, and there are a number of (easy) things you can do
before the work week, which might be really helpful:
1) Test with a clean profile.
2) Test with a new account (this is a bit of a stretch, but I've seen
it resolve some of the wierder JEP problems).
3) Then if the problem keeps happening, test with nightlies until you
find the precise regression range.
Comment 8•17 years ago
|
||
I have another bug on file related to Google documents. I have found when using it that some things don't work well on Mac (this is back when I was using it heavily for a class). I will take a look at this on Tiger and Leopard and report back if I am able to reproduce what shawn is seeing.
Reporter | ||
Comment 9•17 years ago
|
||
More details since I just got slightly better steps to reproduce:
It happens when I use the mouse on the page, and then try to do keyboard entry. So, say I click in a cell, and try to start typing. It doesn't work. However, if I switch away from Minefield, and switch back, it works. I'm able to reproduce that 100% of the time. Still need to try a clean profile.
Reporter | ||
Comment 10•17 years ago
|
||
Reproducible on a clean profile as well.
Reporter | ||
Comment 11•17 years ago
|
||
And on a new spreadsheet in a clean profile.
(sorry for bugspam, forgot to add that last bit before I submitted last comment)
Assignee | ||
Comment 12•17 years ago
|
||
Shawn, are you using any input managers (in /Library/InputManager or
/System/Library/InputManager)?
They're a notorious source of trouble.
Comment 13•17 years ago
|
||
I'm pretty sure I'm not, and I'm seeing the same issue (its a painful painful bug)
Assignee | ||
Comment 14•17 years ago
|
||
OK, what I _really_ need is enough information to reproduce this bug
myself. We're not anywhere close yet.
Reporter | ||
Comment 15•17 years ago
|
||
I don't use any input managers (or at least I've never done anything with it).
Other than a regression range, what else do you need (I don't quite have time for find the regression range just yet).
Assignee | ||
Comment 16•17 years ago
|
||
The regression range is really important -- so if I don't get one from
you (Shawn), I need it from someone else.
I'd also really like to see what happens when you make a new account
and test in that.
But even more important is a precise description of the steps leading
up to the problem.
Here's something to try for anyone who has this problem:
1) Quit everything and restart your computer.
2) As the very first thing you do, start Minefield.
3) As the very first thing you do, go to Google Documents
(http://documents.google.com) and do _exactly_ what's needed to
make this problem happen.
Then describe here that sequence of steps.
It's always possible that bad interactions with other software are
involved ... but that's harder to test for. Before anything else, I
need a good, clean, detailed and precise description of the steps
leading up to this problem. Which (with luck) I'll be able to use
myself to reproduce the problem.
Assignee | ||
Comment 17•17 years ago
|
||
> I don't use any input managers (or at least I've never done anything
> with it).
This is very easy to check for.
Look in /Library/InputManager, /System/Library/InputManager and
~/Library/InputManager. If those directories are empty or don't
exist, you're not using any input managers. If any of them isn't
empty, then you're quite likely to be using an input manager.
If any of them isn't empty, move their contents elsewhere, restart
your computer, and test again.
Comment 18•17 years ago
|
||
I have been able to reproduce this bug on Tiger using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre.
STR:
1. Create a new spreadsheet.
2. Use the mouse to click in the first cell and type a single letter.
3. Use the mouse to click in the cell beneath the first cell and type a single letter.
Result: I cannot type in the cell beneath that original cell until I switch focus out of Google docs.
The same thing does not seem to happen if I move horizontally across the page, only vertically down the page.
Assignee | ||
Comment 19•17 years ago
|
||
Thank you, Marcia!!!!
I can now reproduce the problem. In a few minutes I'll have a
regression range.
Assignee | ||
Comment 20•17 years ago
|
||
The regression range is between the 2008-01-17-04-trunk nightly, which
doesn't have the problem, and the 2008-01-18-04-trunk nightly, which
does.
So yes, my patch for bug 403232 does seem to be implicated :-(
I'll be working on this.
Assignee: nobody → joshmoz
Component: General → Widget: Cocoa
Flags: blocking1.9?
QA Contact: general → cocoa
Assignee | ||
Updated•17 years ago
|
Assignee: joshmoz → smichaud
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Assignee | ||
Comment 21•17 years ago
|
||
Here's a patch that fixes this problem.
My patch for bug 403232 stopped spurious NS_LOSTFOCUS and NS_GOTFOCUS
being sent to Gecko from [ChildView becomeFirstResponder:] and
[ChildView resignFirstResponder:]. But it turns out that it also
stopped some needed focus events from being sent.
This patch restores at least some of them.
(mouseDown events can change focus. When they do and Gecko is
involved, NS_LOSTFOCUS and NS_GOTFOCUS events should be sent to the
appropriate nsChildView objects.)
I tested on both OS X 10.4.11 and 10.5.1.
This patch doesn't fix the problem on Camino, and I need to find out
why. But it works fine in Minefield, and a fix is urgently needed for
beta3. So I think I can postpone finding a fix for Camino.
Attachment #300112 -
Flags: review?(joshmoz)
Assignee | ||
Comment 22•17 years ago
|
||
Here's a tryserver build made using this patch (attachment 300112 [details] [diff] [review]).
It urgently needs fairly wide testing.
Everyone here please test whatever you do on Google docs with this
build.
Marcia, could you give this patch what testing you can, and (if need
be) also arrange for others to test it?
https://build.mozilla.org/tryserver-builds/2008-01-29_12:21-smichaud@pobox.com-bugzilla413882/smichaud@pobox.com-bugzilla413882-firefox-try-mac.dmg
Reporter | ||
Comment 23•17 years ago
|
||
This is working great for me!
Assignee | ||
Comment 24•17 years ago
|
||
> This is working great for me!
Glad to hear it! Thanks for letting me know.
Comment 25•17 years ago
|
||
Steven: I gave the build a cursory look on Leopard, using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008012912 Minefield/3.0b3pre. While I see Shawn's issue addressed, using Leopard I can't click the New widget to create a new spreadsheet or any kind of document on the docs home page. My workaround was to go to an existing spreadsheet I had already created and then add a new one from the link there. I recall that link worked when I tested shawn' issue the other day since I am sure I created the spreadsheet from that entry point.
STR:
1. Go to docs.google.com
2. Access the "New" widget on the left hand side of the page.
3. Click to create a new spreadsheet.
Result: Nothing happens and no errors in console.
Next I will try the build on Tiger to see if the same thing occurs.
Assignee | ||
Comment 26•17 years ago
|
||
Here's a minor revision to my patch that makes it work on both
Minefield and Camino.
Yes, the addition is ugly ... but the same ugliness was also needed in
SetFocus() (in my patch for bug 403232).
A tryserver build will follow shortly.
Attachment #300112 -
Attachment is obsolete: true
Attachment #300166 -
Flags: review?(joshmoz)
Attachment #300112 -
Flags: review?(joshmoz)
Assignee | ||
Comment 27•17 years ago
|
||
Comment on attachment 300166 [details] [diff] [review]
Fix rev1 (works on both Minefield and Camino)
Sigh.
You're right, Marcia. I can reproduce what you report on both 10.4.11
and 10.5.1 (with both versions of my patch).
Thanks very much, though, for testing.
Attachment #300166 -
Flags: review?(joshmoz)
Assignee | ||
Comment 28•17 years ago
|
||
This is going to be a bit of a slog.
I'll keep working on it (though probably not very much more tonight).
But I'm almost certainly not going to make the beta3 freeze.
So will this have to go in after the freeze?
Should beta3 be postponed for this?
Comment 29•17 years ago
|
||
Steven: The "New" link and the"More actions" both do not work for me on Tiger using the tryserver build. The rest of the items on that line work for me.
Comment 30•17 years ago
|
||
Moving this to P2 given timing and risk. This'll have to go in after beta3.
Priority: P1 → P2
Assignee | ||
Comment 31•17 years ago
|
||
For what it's worth (probably not much), here's a tryserver build of
rev1 of my patch:
https://build.mozilla.org/tryserver-builds/2008-01-29_15:39-smichaud@pobox.com-bugzilla413882-rev1/smichaud@pobox.com-bugzilla413882-rev1-firefox-try-mac.dmg
Assignee | ||
Updated•17 years ago
|
Attachment #300166 -
Attachment is obsolete: true
Assignee | ||
Comment 32•17 years ago
|
||
I've moved my fix to a new location -- where it works much better.
But I've found that, despite all my efforts, I can't avoid regressing
bug 404433.
This bug (bug 413882) seems much worse than bug 404433. So, if we
have to choose between the two, I think it's more important to fix
this one.
I'll keep looking for a way to fix this bug without regressing bug
404433 ... but I doubt I'll be able to find one in time for the
Firefox 3 release.
Here's a tryserver build made using this patch.
https://build.mozilla.org/tryserver-builds/2008-02-05_11:40-smichaud@pobox.com-bugzilla413882-rev2/smichaud@pobox.com-bugzilla413882-rev2-firefox-try-mac.dmg
Marcia (and anyone else who's willing), please test this build to see
if there's anything I missed.
Attachment #301566 -
Flags: review?(joshmoz)
Comment 33•17 years ago
|
||
smichaud, I can verify that your tryserver build fixes the bug. I am running it on Leopard 10.5.1.
Assignee | ||
Comment 34•17 years ago
|
||
Thanks, pjcabrera, for letting me know.
I particularly want to hear from heavy users of Google documents
(which, for some reason, seems likely to trigger focus problems).
Marcia? :-)
Comment 35•17 years ago
|
||
Steven: As soon as I can finish my B3 testing, I am all over that tryserver build and will report back.
Assignee | ||
Comment 36•17 years ago
|
||
Thanks, Marcia.
If there's a problem with my patch (beyond the regression of bug
404433 that I'm already aware of), I trust you'll find it :-)
Comment 37•17 years ago
|
||
smichaud, I see a weird interaction with Growl notifications with your tryserver build:
e.g. when a Growl message bubble from, say, Twitterrific, floats over the Firefox window, and I click on the Growl message bubble to make it go away, I can't get focus to go back to the Firefox window. I have to command-tab and make focus go to another app, then command-tab again to return to the Firefox window. Same behavior as with this bug, but from an apparent different source of loss of focus.
I've verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 build from today's testing, using another test profile. The weird interaction with Growl doesn't happen there.
Comment 38•17 years ago
|
||
By lack of focus, I mean keyboard and mouse clicking are apparently not received. I can't switch tabs with mouse or keyboard shortcuts, can't get focus back by clicking anywhere. It's not just loss of keyboard focus.
Assignee | ||
Comment 39•17 years ago
|
||
pjcabrera, I don't know how to set myself up to receive Growl
notifications. Please tell me.
Reporter | ||
Comment 40•17 years ago
|
||
Adium uses growl, and it'd probably be the easiest way for you to get some. Just have a friend im you each time you need to test it.
Comment 41•17 years ago
|
||
Once you have installed and set up Growl, restarting Growl-enabled applications
should make the apps "register" themselves for notification of events through
Growl. If that doesn't work, reinstall the app. Adium, Firefox, Twitterrific and
NetNewsWire are Growl-enabled, among others.
To see what apps have registered with Growl, you can open the Growl Systems
Preferences panel, and click on the "Applications" tab. Then you can unregister
and re-register apps with Growl by setting and unsetting the checkboxes.
Once apps have registered with Growl, it's just a matter of waiting for (or
forcing) an app to report an event through Growl while the Firefox window has
focus. For example, use Twitterrific to see tweets from the public timeline
(lots of traffic there, guaranteed to make Twitterrific report an event through
Growl in a very short timeframe.)
Assignee | ||
Comment 42•17 years ago
|
||
So, pjcabrera, Growl and Twitteriffic are seperate applications
... which I didn't know :-)
So, Shawn, could I do this?
1) Run Adium on my target machine (the one I'll test on) and connect
to, say, #macdev.
2) Switch the focus to Minefield, in an open browser window.
3) Connect to #macdev from another computer, under a different
nickname.
4) Do "/msg smichaud" in the im client running on that other computer.
Reporter | ||
Comment 43•17 years ago
|
||
Yes, but I don't know if Adium does irc. I think that colloquy would work in that situation.
Assignee | ||
Comment 44•17 years ago
|
||
(In reply to comment #37 and comment #38)
> smichaud, I see a weird interaction with Growl notifications with
> your tryserver build: [...]
I momentarily saw something like you report (but not as severe) with
today's Minefield nightly (i.e. without my patch).
But now I can't reproduce it, either with today's nightly or with a
build made with my patch on top of source code pulled this morning.
I suspect this problem (whatever it is) is unrelated. Though it would
help if you could give me good "steps to reproduce".
On general principles I'm going to make another tryserver build (with
the same patch). It will supercede the earlier one (after it becomes
available, people should test with the new one).
(By the way, the source of my Growl notifications was Colloquy,
running in the background. I tested on OS X 10.5.1.)
Assignee | ||
Comment 45•17 years ago
|
||
Here's the new tryserver build:
https://build.mozilla.org/tryserver-builds/2008-02-11_12:22-smichaud@pobox.com-bugzilla413882-rev2.1/smichaud@pobox.com-bugzilla413882-rev2.1-firefox-try-mac.dmg
Attachment #301566 -
Attachment is obsolete: true
Attachment #302658 -
Flags: review?(joshmoz)
Attachment #301566 -
Flags: review?(joshmoz)
Comment 46•17 years ago
|
||
Steven: Gave your build a quick whirl and things look good. I tried all the various combinations of document and spreadsheets I could try including creating a new document and spreadsheet, saving, renaming, etc. I did not see an hiccups.
(In reply to comment #45)
> Created an attachment (id=302658) [details]
> Fix rev 2.1 (update for current code)
>
> Here's the new tryserver build:
>
> https://build.mozilla.org/tryserver-builds/2008-02-11_12:22-smichaud@pobox.com-bugzilla413882-rev2.1/smichaud@pobox.com-bugzilla413882-rev2.1-firefox-try-mac.dmg
>
Comment 47•17 years ago
|
||
I am downloading your latest tryserver build at the moment. I will see if I can reproduce the issue. If I can reproduce it, I'll write detailed steps for you to verify. I'll let you know what happens either way. :-)
Comment 48•17 years ago
|
||
I am unable to reproduce the Growl issue with this latest tryserver build. I ran it on 10.5.1 with several Growl notification sources.
You're probably right and the Growl weirdness from Friday was unrelated to your patch.
Good job on the Google Docs fix. :-)
Comment 49•17 years ago
|
||
Comment on attachment 302658 [details] [diff] [review]
Fix rev 2.1 (update for current code)
As you probably guessed, I don't like this swizzling at all.
I think our Cocoa widget focus code has some fundamental issues that aren't gecko bugs (while those exist too). This patch seems to be taking us even further away from a reasonable impl. We had focus working just fine with Carbon widgets and we don't have any less info given to us with which to determine focus behavior in Cocoa widgets, afaict. We need to attack the fundamental issues here instead of going further and further out on a limb. There is time for that (not much, but enough I think). I can help you with this over the next week or two.
For now, please explain little more why swizzling gives us information or capabilities that we couldn't obtain without it. I find it hard to believe that we need to intercept information at the window level in the embedding situation. If you just want to find out that a view lost first responder and other got it, why exactly don't you just use the normal mechanisms for that?
+ NSLog(@"NSWindow sendEvent: view hierarchy above oldFirstResponder:");
Also you left a bunch of active logging code in the patch.
Assignee | ||
Comment 50•17 years ago
|
||
Thank you, Marcia and pjcabrera. I think your results show that my
patch works reasonably well, and that I'm on the right track.
Assignee | ||
Comment 51•17 years ago
|
||
> As you probably guessed, I don't like this swizzling at all.
There's no good reason not to like it (or its cousin poseAsClass:).
As with anything that gets into undocumented parts of the OS, it
should be used with caution, and only when no other, better solution
exists.
But there is no performance penalty (method swizzling and poseAsClass:
basically just swap method pointers inside the Objective-C
architecture).
And if you choose carefully the methods that you "swizzle", there
should be no problems going forward. In this case I'm "swizzling"
[NSWindow sendEvent:], which has existed in the same form since the
earliest days of OS X, and is very likely to do on doing so.
(And in the very unlikely event that Apple _did_ change [NSWindow
sendEvent:], or dropped it altogether, method swizzling and
poseAsClass both fail gracefully -- trying to "hook" an [NSWindow
sendEvent:] would simply have no effect.)
Assignee | ||
Comment 52•17 years ago
|
||
> For now, please explain little more why swizzling gives us
> information or capabilities that we couldn't obtain without it.
I've already answered this question, in my comment above
nsCocoaWindow_NSWindow_sendEvent:
+// For non-embedders (e.g. Firefox and Seamonkey), it would probably only be
+// necessary to add a sendEvent: method to the ToolbarWindow class. But
+// embedders (like Camino) generally create their own NSWindows. So in order
+// to fix this problem everywhere, it's necessary to "hook" NSWindow's own
+// sendEvent: method.
Comment 53•17 years ago
|
||
I don't like it because it appears to be unnecessary and obfuscating, not because I think the technique of swizzling itself is dangerous.
Assignee | ||
Comment 54•17 years ago
|
||
> don't like it because it appears to be unnecessary and obfuscating,
What does this mean, if anything.
> because I think the technique of swizzling itself is dangerous.
Only if used incorrectly.
Comment 55•17 years ago
|
||
That comment doesn't answer my question, I'll clarify. Cocoa has methods, "becomeFirstResponder" and "resignFirstResponder", for handling changes to the first responder. That, combined with window activation information (key/main status notifications), should tell you everything you need to know. Why do you need to use data from sendEvent: to figure that stuff out?
Assignee | ||
Comment 56•17 years ago
|
||
> We need to attack the fundamental issues here instead of going
> further and further out on a limb. There is time for that (not much,
> but enough I think). I can help you with this over the next week or
> two.
I've been working on this (off and on) for several months. You haven't.
Yes, we should tackle the fundamental issues. But, frankly, in all
the time I've spent on this I'm only just beginning to see what they
might be. I'm convince there _isn't_ enough time to come to grips
with these "fundamental issues" before the Firefox 3 release, and that
trying to do so would just waste time better spent elsewhere.
> This patch seems to be taking us even further away from a reasonable
> impl.
This is just flat out wrong. My patch for bug 403232 resolved quite a
few problems. And while it did cause an important regression (notably
this bug), it took quite a while for it to surface -- a sign that even
the patch for bug 403232 was quite _close_ to right.
If you have a solution that's close to right, the reasonable thing to
do is tweak it until it _is_ right. That's what I'm doing with
attachment 302658 [details] [diff] [review].
Assignee | ||
Comment 57•17 years ago
|
||
> Cocoa has methods, "becomeFirstResponder" and
> "resignFirstResponder", for handling changes to the first responder.
Look at my patch for bug 403232.
It was these methods that we used before that patch. Doing so
whenever these methods were called was what caused the problems that
the patch for bug 403232 fixed.
Assignee | ||
Comment 58•17 years ago
|
||
Attachment #302658 -
Attachment is obsolete: true
Attachment #302924 -
Flags: review?(joshmoz)
Attachment #302658 -
Flags: review?(joshmoz)
Comment 59•17 years ago
|
||
+ [self nsCocoaWindow_NSWindow_sendEvent:anEvent];
How does that not hang the browser? Doesn't it just call itself until it runs out of stack space?
Assignee | ||
Comment 60•17 years ago
|
||
>+ [self nsCocoaWindow_NSWindow_sendEvent:anEvent];
>
> Doesn't it just call itself until it runs out of stack space?
Nope. That method swizzling at work.
See http://www.cocoadev.com/index.pl?MethodSwizzling for an
explanation.
Comment 61•17 years ago
|
||
Comment on attachment 302924 [details] [diff] [review]
Fix rev 2.2 (get rid of debug logging)
Since we're sticking with your approach for now this is a win, I'll yield to expediency. However, I stand by what I said before and I hope we have time to investigate further.
Attachment #302924 -
Flags: superreview?(roc)
Attachment #302924 -
Flags: review?(joshmoz)
Attachment #302924 -
Flags: review+
Comment on attachment 302924 [details] [diff] [review]
Fix rev 2.2 (get rid of debug logging)
I feel similar to Josh, we're off in the weeds here. I only hope that we'll be able to rework things significantly post-FF3.
Attachment #302924 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 63•17 years ago
|
||
Landed on trunk.
Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v <-- nsChildView.mm
new revision: 1.306; previous revision: 1.305
done
Checking in widget/src/cocoa/nsCocoaWindow.mm;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v <-- nsCocoaWindow.mm
new revision: 1.129; previous revision: 1.128
done
Checking in widget/src/cocoa/nsToolkit.h;
/cvsroot/mozilla/widget/src/cocoa/nsToolkit.h,v <-- nsToolkit.h
new revision: 1.17; previous revision: 1.16
done
Checking in widget/src/cocoa/nsToolkit.mm;
/cvsroot/mozilla/widget/src/cocoa/nsToolkit.mm,v <-- nsToolkit.mm
new revision: 1.30; previous revision: 1.29
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 417636
Assignee | ||
Comment 64•17 years ago
|
||
It's hard to be sure, but my patch for this bug (bug 413882,
attachment 302924 [details] [diff] [review]) may have caused a crash bug in Camino (bug 417636).
If I'm right about this, sending a focus event to Gecko can cause a
widget (and its ChildView object) to be destroyed. So in
nsCocoaWindow.mm's nsCocoaWindow_NSWindow_sendEvent: I need to retain
and release _both_ oldFirstResponder and newFirstResponder around
calls that send focus events to Gecko. (Previously I only retained
and released oldFirstResponder.)
My new patch implements this change, and also another one suggested by
Stuart Morgan: I only need to retain and release oldFirstResponder
(or newFirstResponder) if they're ChildView objects. So that's what
I'm now doing.
The reason it's hard to be sure is that bug 417636 isn't reproducible.
But a crash log posted there strongly suggests that the crash happened
because a focus event was sent to a ChildView object that had been
destroyed.
Attachment #303612 -
Flags: review?(joshmoz)
Comment 65•17 years ago
|
||
Looks like the changes to mozilla/widget/cocoa/nsChildview.mm rev 1.306 (smichaud%pobox.com 2008-02-13 07:57) caused the same issues as outlined (and previously fixed) in Bug 357535. I reverted to 1.305 and that clears the other bug up for me.
Comment 66•17 years ago
|
||
Bug 418031 filed.
Assignee | ||
Comment 67•17 years ago
|
||
Comment on attachment 303612 [details] [diff] [review]
Fix bug 417636
Another, better fix is coming up.
Attachment #303612 -
Flags: review?(joshmoz)
Assignee | ||
Comment 68•17 years ago
|
||
My patch for this bug regressed bug 403232, bug 404433 and bug 357535
(aka bug 418031).
I've now posted a patch at bug 404433 that fixes all these bugs
without regressing this one.
It also retains the code that was suggested by Stuart Morgan.
To test it, please go to bug 404433 (where I've posted a tryserver
build).
We're just thrashing around uselessly here.
At minimum, before we touch this code again we need mochitests for the entire set of known related bugs.
Assignee | ||
Updated•17 years ago
|
Attachment #303612 -
Attachment is obsolete: true
Assignee | ||
Comment 70•17 years ago
|
||
I just ran the complete set of Mochitests on my test build (built with
my patch). There were no failures:
Passed: 47025
Failed:0
Todo: 114
That's good, but we need mochitests that specifically test these bugs at least:
413882
403232
404433
418031
BTW your patch description in bug 404433 sounds good although I haven't read the patch itself yet.
Assignee | ||
Comment 72•17 years ago
|
||
> we need mochitests that specifically test these bugs at least:
> 413882
> 403232
> 404433
> 418031
I don't know how to write mochitests. Can we ask someone else to do
that?
Mochitests are fairly simple --- although writing tests for these bugs might be tricky. Trying grepping for existing mochitests that manipulate focus. Alternatively, bribe Martijn into helping you :-).
Assignee | ||
Comment 74•17 years ago
|
||
> we need mochitests that specifically test these bugs at least:
> 413882
> 403232
> 404433
> 418031
Two of these bugs' STRs (bug 404433 and bug 413882) require a Google
account. I assume this means that it won't be possible to write
mochitests that use their current STRs (though Martijn can tell me if
I'm wrong). Finding new STRs for these bugs will be a _lot_ of work,
and probably take a long time -- time we (and I) don't have in the
leadup to beta4.
And even for the other two bugs, it'll take me a while (a day or two?)
to get up to speed writing mochitests.
All of these bugs have good (100% effective) STRs, and there aren't
too many of them. Yes, we eventually want mochitests for all of them.
But in the meantime I think we can rely on testing "by hand".
Comment 75•17 years ago
|
||
It absolutely should not take a day or two to get up to speed on Mochitests (I'd say /maybe/ an hour or two); if it does, we're doing something wrong.
The mochitests wouldn't use Google Docs, they'd be reduced testcases. Although since you understand the bugs pretty well, you can probably just write testcases from scratch.
We can't rely on testing "by hand" since it hasn't worked so far.
It won't take you a day or two to get up to speed on Mochitests. It might take a day or two to come up with appropriate testcases. That would be time well spent.
Comment 77•17 years ago
|
||
Unfortunately Attachment 304052 [details] [diff] also breaks the Message Compose WYSIWYG text input box on vBulletin. It fixed my problems with Thunderbird Format Bar, but breaks this. You can click in the box and get a cursor in there, but when you type nothing happens. You have to click in the title box, then back in the WYSIWYG editor for text input to work. It is javascript. You can post at http://www.vbulletin.com/forum/ with the WYSIWYG editor if you need an example. I removed the patch and recompiled and the problem went away. Did another checkout, compile, everything was fine, applied patch once again, and text input broken again.
Assignee | ||
Comment 78•17 years ago
|
||
> You can post at http://www.vbulletin.com/forum/ with the WYSIWYG
> editor if you need an example.
Do you know of a forum there that I can post messages to (or start to)
without having to register?
Comment 79•17 years ago
|
||
Anonymous/Unlogged-In users cannot use WYSIWYG editor on vBulletin forums have to have an account. If you can't create an account there at www.vbulletin.com/forum/ then I can create one on my personal forums and email you username/password.
Assignee | ||
Comment 80•17 years ago
|
||
Don't worry about it. I've registered and am now able to use the post
editor.
But I'm not able to reproduce what you report (using the tryserver
build made with my rev1 patch, attachment 304062 [details] [diff] [review]).
I clicked on "vBulletin Suggestions and Feedback", then on the "New
Thread" button. Then I clicked in the Message box and started typing.
I had no problems.
Assignee | ||
Comment 81•17 years ago
|
||
Comment 82•17 years ago
|
||
Are you using the Full WYSIWYG editor, should have been an option. I'll download the tryserver build.
Comment 83•17 years ago
|
||
tryserver did indeed work just fine.
The patch I applied was:
bugzilla404433-patch-cvs.txt
Index: widget/src/cocoa/nsCocoaWindow.mm
===================================================================
RCS file: /cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v
retrieving revision 1.131
diff -U 10 -r1.131 nsCocoaWindow.mm
--- widget/src/cocoa/nsCocoaWindow.mm 16 Feb 2008 20:33:32 -0000 1.131
+++ widget/src/cocoa/nsCocoaWindow.mm 17 Feb 2008 23:33:04 -0000
Assignee | ||
Comment 84•17 years ago
|
||
> The patch I applied was:
> bugzilla404433-patch-cvs.txt
Wrong patch (not the latest).
The one you should be using is bugzilla404433-patch-rev1-cvs.txt
(attachment 304052 [details] [diff] [review]).
I think I finally got the number right :-)
Comment 85•17 years ago
|
||
Ok, using the correct patch. Everything works for me.
1) vBulletin works, previous comment rescinded. (Comment 77)
2) Google Docs Spreadsheet focus is fine. (Though nasty scroll bar in each cell per Bug 416752)
3) Thunderbird Format Toolbar is functioning. (Bug 418031).
Comment 86•17 years ago
|
||
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre and on the nightly Tiger build as well. I tested Google docs and everything looks good there - I do see the scroll bar in each cell that Jay notes in Comment 85. thanks jay for following up and testing the other items you note in Comment 85.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 87•17 years ago
|
||
I don't know if this was caused by this bug, or if I should file a new bug on this, so I'm just going to comment. Now in gmail and google docs, if I click to select a tab, I have to click in the content area to be able to use the keyboard. So, if I click my gmail tab, I have to click in the content area to use the keyboard shortcuts, and in docs I have to click again in the active cell to be able to start to type there. I don't recall having to do that before.
Assignee | ||
Comment 88•17 years ago
|
||
Shawn, I don't know what this means :-)
Reporter | ||
Comment 89•17 years ago
|
||
What doesn't make sense - I know I've had a long day, and I'm not sure if it's new bug worthy.
Assignee | ||
Comment 90•17 years ago
|
||
I just need more clarity and more detail.
Feel free to wait til tomorrow.
I'm not going to do any more work on this bug tonight :-)
Assignee | ||
Comment 94•17 years ago
|
||
(Following up comment #74)
I've spent the last 4 or 5 days trying to write an automated test for
bug 403232. I've discovered problems that will either make this
impossible or very difficult. See bug 419466.
For other problems I encountered along the way, see bug 418745 and bug
418859.
Reporter | ||
Comment 95•17 years ago
|
||
(In reply to comment #90)
> I just need more clarity and more detail.
>
> Feel free to wait til tomorrow.
>
> I'm not going to do any more work on this bug tonight :-)
Still haven't had time to write this up, but there happens to be a newsgroup post on this very issue!
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/62cf7e4b67e4e041/3c6c00dd27ed3d60#3c6c00dd27ed3d60
Assignee | ||
Comment 96•17 years ago
|
||
(In reply to comment #95)
Neither you nor TJ (who started the Google thread) give specific
examples of problems with keyboard shortcuts (with complete STRs).
These are needed.
When you're ready, please open a new bug and cc me.
Also take a look at bug 413681 comment #18 and following, which may be
relevant. (The earlier comments go all over the map, and aren't very
useful.)
As for TJ's problem with the Esc key, it may be a dup of bug 415437
(and already fixed in the latest nightlies).
Comment 97•16 years ago
|
||
(In reply to comment #61)
> Since we're sticking with your approach for now this is a win, I'll yield to
> expediency. However, I stand by what I said before and I hope we have time to
> investigate further.
(In reply to comment #62)
> I feel similar to Josh, we're off in the weeds here. I only hope that we'll be
> able to rework things significantly post-FF3.
Was a bug filed to rework this? Was any investigation done?
Comment 98•16 years ago
|
||
Erm...
Hello,
Was a bug filed to rework this "significantly post-FF3"? Was any investigation done? I ask because, here we are finishing Firefox 3.1 work and I only see more swizzling being done, not less, despite those comments. I'd like to know if there are plans to change that for 1.9.2, especially given the focus rewrite being done. Hopefully this will no longer be needed.
The focus rewrite should give us a good base to clean up the Cocoa focus handling.
Comment 100•16 years ago
|
||
(In reply to comment #99)
> The focus rewrite should give us a good base to clean up the Cocoa focus
> handling.
What bug number is the Cocoa work?
I don't think we have one.
Comment 102•16 years ago
|
||
So, the answer to comment 98 is "no". Can we get a bug on file from someone who has an understanding of what might/would need to happen after the focus rewrite lands?
Comment 103•16 years ago
|
||
There is cleanup code for Cocoa widget focus in Neil Deakin's patch for bug 178324, at least there was last time I looked. That's a start.
Comment 104•10 years ago
|
||
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Keywords: qawanted
You need to log in
before you can comment on or make changes to this bug.
Description
•