Closed Bug 314160 Opened 16 years ago Closed 13 years ago

Typing focus lost when using any Midas buttons/selects except text color/hightlight color

Categories

(Core :: Widget: Cocoa, defect, P2)

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: alqahira, Assigned: smichaud)

References

()

Details

(Keywords: access, regression, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

In bug 197253 comment 77:

------- Comment #77 From sairuh  2005-01-20 13:53 PDT  [reply] -------

tested with 2005011808-trunk camino bits on 10.3.7.

overall, this works, but found some issues --are they already filed?

b. after trying the commands in the 2nd formatting toolbar (same sample url
above), the caret (and focus) within the midas editing area is lost. need to hit
Tab or reclick in the editing area again.


No one ever replied to sairuh and so this bug was never filed.  In Fx, all the toolbar buttons and selects preserve focus of the Midas area/selection.  (Whee, another focus bug!)
This isn't the same as our "lose focus when tabbing through plugin" bug, is it?
(In reply to comment #1)
> This isn't the same as our "lose focus when tabbing through plugin" bug, is it?

I don't think so (but what bug is that?); this is about pushing buttons in Midas (which is not a plugin).  And using selects, but they have focus issues already in Camino.
Assignee: mikepinkerton → nobody
Component: General → Accessibility
Keywords: access
QA Contact: accessibility
Remind me to look at this with Steven's focus patch.
And the answer is no, at least for everything except the colorpicker buttons.
Trunk FTW! This works correctly in 20080331
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Really?  In a build from last night, pressing a button doesn't eat the caret anymore, but it still sends focus somewhere else such that I have to click back into the midas area to start typing again....

Actually, I can move the caret with the cursor; I just can't type.

Text/highlight color do work properly.
Wow, that's crazy. I was just looking at the highlighting and cursor; I didn't realize the current behavior could be possible.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Tweaking summary, and sending to core since today's Minefield has the same bug.
Assignee: nobody → joshmoz
Status: REOPENED → NEW
Component: Accessibility → Widget: Cocoa
Product: Camino → Core
QA Contact: accessibility → cocoa
Summary: Focus lost when using any Midas buttons/selects except text color/hightlight color → Typing focus lost when using any Midas buttons/selects except text color/hightlight color
This isn't really a focus problem, as far as I can tell.  But it does
seem to be a Cocoa widgets problem (I can't reproduce it in today's
Windows or Linux nightlies).

Here's a clear description of the problems I saw with today's
Minefield nightly:

1) Load the Midas demo URL (http://www.mozilla.org/editor/midasdemo/),
   click in the edit box (below the two menu bars), and type some
   text.

2) Click on the Bold button (or the Italic button or the Underline
   button).

   The text-input cursor still blinks in the correct location, and the
   arrow keys move it.

   But no new text gets entered as you type.

   And even more strangely, pressing the Backspace key triggers the
   browser's Back button!

This isn't a recent regression.  Before my focus patches (for bug
403232 and bug 404433) landed, the text-input cursor would stop
blinking when you pressed any of these buttons.  But the Backspace key
still triggers the Back button in these earlier nightlies.
(Following up comment #9)

> Backspace key

I should have said "Delete key".

(The keyboard I first tested with really does have a Backspace key --
it's an old Dell keyboard.  But I've now retested with an Apple
keyboard and got the same results.)
The Delete key wierdness no longer happens with today's nightly.  Could have been fixed by the patch for bug 398514.
So for mail-client type folks (and probably HTML editor-type folks), this is a regression from the switch to Cocoa.  For them, it's also not minor, either.
Severity: minor → normal
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Keywords: regression
Assignee: joshmoz → smichaud
Priority: -- → P2
(In reply to comment #13)
> So for mail-client type folks (and probably HTML editor-type folks), this is a
> regression from the switch to Cocoa.  For them, it's also not minor, either.

For Thunderbird/SeaMonkey, anyone attempting to compose an HTML email on Mac will likely run across this. Steps to repeat:

- Type some characters
- Click the bold/italic/underline button
- Try to type some more characters - nothing happens, and it is very hard to get the text bold or something without first typing your text and then going back and selecting it.

This will be found easily - I expect it hasn't been in our releases so far because I expect that most of our nightly testers probably use plain text, or just haven't reported it.

Although this bug is old, TB/SM worked fine on the 1.8 branch. I think it is only that we didn't ship off 1.9 that we haven't found it until now - we'd have most likely had some reports if we'd been shipping betas.

Therefore requesting blocking as David, Dan and myself think this will be reasonably high profile for TB.
Flags: blocking1.9.1?
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Attached patch Fix (obsolete) — Splinter Review
Here's a patch that fixes this bug without regressing bug 403232, bug
404433, bug 357535, bug 413882 or bug 418031.  It also doesn't regress
the (separate) problem reported in bug 413882 comment #77.

I tested in Minefield, Shredder and Seamonkey, on OS X 10.5.4 and
10.4.11.  I also tested a cvs variant of this patch in Camino (again
on OS X 10.5.4 and 10.4.11).

I surprised myself by finding that I could accomplish all this while
substantially simplifying the previous code.

I've done a lot of testing.  But I'd really like to see others test,
too.  Marcia? :-)  And whoever else you can persuade to participate
from the QA team.

The STRs of all the bugs I mentioned above should be tested.  I'd also
like to see someone who knows Google docs well (and particularly
Google spreadsheets) bang away hard on this patch.

A tryserver build (of Minefield) will follow in an hour or two.
Attachment #340418 - Flags: review?
Attachment #340418 - Flags: review? → review?(joshmoz)
Mark:  Needless to say, I'll expect you to test this patch, too.
(In reply to comment #16)
> Mark:  Needless to say, I'll expect you to test this patch, too.

I've done a short test on Thunderbird with this patch and it appears to be working correctly. I'll do some more testing tomorrow and maybe check out those other bugs you referenced (i.e. check they haven't regressed) if I get time.
ere's a more precise list of things to test (STRs) to vet my patch
for this bug (including some that I didn't mention before):

A) Tests in Minefield/Firefox:

1) Bug 314160 comment #9.

2) Bug 399471 comment #0 and comment #2.

3) Bug 403232 comment #0.

4) Bug 404433 comment #0 (need a Gmail account).

5) Bug 408266 comment #0.

   This STR is already in litmus (bug 408266 comment #7).

6) Bug 413882 comment #18 (need a Gmail account).

   Note that the cell in step 2 needs to be above the "bar", and the
   cell in step 3 needs to be below the "bar".

7) Bug 413882 comment #25 (need a Gmail account).

8) Bug 413882 comment #77 and bug 413882 comment #80 (need a free
   vBulletin account).

   Here's a more precise STR:

   a) Visit http://www.vbulletin.com/forum/ and log in.

   b) Click on "vBulletin Suggestions and Feedback" (under "vBulletin
      3.7").

   c) Click on the "New Thread" button.

   d) Click in the message body and try typing.  (For extra credit
      also fiddle with the formatting buttons and keep typing.)

   e) Log out without submitting your post :-)

B) Tests in Thunderbird/Shredder

1) Bug 357535 comment #0.

2) Bug 418031 comment #0.
For what it's worth, here's a Shredder build (universal) that I did
myself and uploaded to my people.mozilla.com account.  It was built
using code pulled the morning of 2008-09-22 -- plus my patch, of
course.

http://people.mozilla.com/~stmichaud/bmo/thunderbird-3.0b1pre.en-US.mac.bugzilla314160-smichaud.dmg
I've just tested all the bugs that don't require logins on my local built FF and TB (pulled today) with the patch applied. I couldn't see any regressions.
Thanks, Mark!
Comment on attachment 340418 [details] [diff] [review]
Fix

+  ChildView *mShouldFocusView; // Strong

Why did you choose to make this a strong reference instead of just clearing the value when a view gets deleted via the view's destructor or destroy call, something like that?
I can't make mShouldFocusView a weak pointer:  This variable needs to
be associated with a NSWindow object.  But by the time [ChildView
widgetDestroyed] is called the ChildView ("self") may already have
been detached from its NSWindow.  In this case it won't be possible to
null-out this variable (still associated with the NSWindow to which
"self" once belonged), and there's a real possibility that
mShouldFocusView will become an invalid pointer (that it will point to
a deleted object).

But I've made other changes to neaten up my patch.  In particular,
I've added code to [TopLevelWindowData dealloc] to make sure
mShouldFocusView is never leaked.  And I've changed
[TopLevelWindowData getShouldFocusView] to make sure it never returns
an object that's been detached from its window (and that such an
object is never made a first reponder in
nsCocoaWindow_NSWindow_sendEvent:).

A new tryserver build and Shredder build will follow in a while.

I've tested my new patch (in my own builds) with all the STRs from
comment #19, on both OS X 10.5.5 and 10.4.11.
Attachment #340418 - Attachment is obsolete: true
Attachment #341339 - Flags: review?(joshmoz)
Attachment #340418 - Flags: review?(joshmoz)
Here's a tryserver build (of Firefox/Minefield) made with my rev1 patch:

https://build.mozilla.org/tryserver-builds/2008-10-01_13:33-smichaud@pobox.com-bugzilla314160-rev1/smichaud@pobox.com-bugzilla314160-rev1-firefox-try-mac.dmg

And here's a custom build of Thunderbird/Shredder made with my rev1 patch.  As before, the base code was pulled on 2008-09-22.

http://people.mozilla.com/~stmichaud/bmo/thunderbird-3.0b1pre.en-US.mac.bugzilla314160-rev1-smichaud.dmg
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Attachment #341339 - Flags: review?(joshmoz) → review+
Attachment #341339 - Flags: superreview?(vladimir)
Comment on attachment 341339 [details] [diff] [review]
Fix rev1 (clean up)

Vlad, do you want to sr this?  Or should I ask someone else?
Attachment #341339 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 341339 [details] [diff] [review]
Fix rev1 (clean up)

This looks fine; sorry I missed this review request earlier.
Landed on mozilla-central (changeset 83cb01024590).
Status: NEW → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Landed before 1.9.1 branching:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/83cb01024590

Verified fixed on recent Shiretoko build:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090220 Shiretoko/3.1b3pre

(Also confirmed present on Firefox 3.0 on Intel Mac.)
Status: RESOLVED → VERIFIED
Hardware: PowerPC → All
It looks like that it's worth having an automated test for this issue.
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.1b3
Version: unspecified → Trunk
Automated testing of widget-level focus problems won't really be
feasible until bug 470845 is resolved.
Ok, so it should depend on bug 470845. Means we don't miss it. Thanks Steven.
Depends on: 470845
You need to log in before you can comment on or make changes to this bug.