308865 - Spellcatcher X shorthands no longer work in Camino 1.0a1

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[ronald.gold]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050914 Camino/1.0a1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050914 Camino/1.0a1

Spellcatcher X shorthands, which worked fine in all earlier versions of Camino,
no longer work in 1.0a1. Instead of expanding the shorthand after the trigger
(space bar), the last letter of the shorthand is deleted. This problem is unique
to Version 1.0a1 as all features of Spell Catcher X work in other programs.

Is this in any way related to the conflict between 1.0a1 and URL Manager Pro,
reported in Camino Forum?

Reproducible: Always

Steps to Reproduce:
1.Enter Spellcatcher Shorthand in textbox
2.Hit trigger to expand shorthand
3.

Actual Results:  
1. Instead of expanding shorthand, last letter of shorthand is deleted and
cursor stops at that point.
2. For example, “em-space” should expand to my email address. Instead, I get “e”
with no expansion. Same for all shorthands.


Is this a bug of Camino 1.0a1 (if so what changed to cause it?) or is it a
problem with Spell Catcher. Since problems have also emerged with URL Manager
Pro with 1.0a1, something must have changed in Camino.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can you figure out when this broke?

http://ftp.mozilla.org/pub/mozilla.org/camino/nightly/
(for dates after 8-16, the directories ending in -1.0 are what you want to check)

Comment 2

[ronald.gold]

(In reply to comment #1)
> Can you figure out when this broke?
> 
> http://ftp.mozilla.org/pub/mozilla.org/camino/nightly/
> (for dates after 8-16, the directories ending in -1.0 are what you want to check)

The version I was using prior to 1.0a1 was 1.0+ Optimized for newer G4, build
9/5/05. I first started using Spell Catcher with this build.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> The version I was using prior to 1.0a1 was 1.0+ Optimized for newer G4, build
> 9/5/05. I first started using Spell Catcher with this build.

That was a trunk build.  Does it work in the branch build from the same day
(i.e., from the directory ending in -1.0/ for that day at the link I mentioned
before)? 

Comment 4

[ronald.gold]

(In reply to comment #3)
> (In reply to comment #2)
> > The version I was using prior to 1.0a1 was 1.0+ Optimized for newer G4, build
> > 9/5/05. I first started using Spell Catcher with this build.
> 
> That was a trunk build.  Does it work in the branch build from the same day
> (i.e., from the directory ending in -1.0/ for that day at the link I mentioned
> before)? 

I first started using Spell Catcher on Sept 6. 2005. The bug only occurs when I
try to enter shorthands in Textbox. The shorthands expand normally if I enter it
into address or google field. Short hands worked fine in all builds prior to
2005-09-11-04-1.0/. The bug first appeared with this build.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Figures it would be when we didn't have branch builds for two days :-(

Regression window (largely Camino-specific): http://tinyurl.com/arnn3

Is it also broken on the trunk (builds in folders not ending with -1.0/)?  If
it's broken on the trunk, too, that would help narrow the regression window.

Mostly Simon's checkins, so cc'ing him.

(Note I don't have the tools to repro this myself, but based on the reporter
finding a regression window, confirming.)

Comment 6

[ronald.gold]

(In reply to comment #5)
> Figures it would be when we didn't have branch builds for two days :-(
> 
> Regression window (largely Camino-specific): http://tinyurl.com/arnn3
> 
> Is it also broken on the trunk (builds in folders not ending with -1.0/)?  If
> it's broken on the trunk, too, that would help narrow the regression window.
> 
> Mostly Simon's checkins, so cc'ing him.
> 
> (Note I don't have the tools to repro this myself, but based on the reporter
> finding a regression window, confirming.)

Spell Catcher works fine in builds from folders not ending with -1/0/ until
build of 11/09/05 at which point the shorthands no longer expand in the text box.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Trunk regression window: http://tinyurl.com/d9kxc

Simon, any ideas?  Is it possibly the bundle identifier switch and SpellCatcher
can't find Camino now (that checkin is just prior to the regression window, though)?

Comment 8

[ronald.gold]

Does the following help in identifying the bug:

in Version 1.0a1, expansion of shorthands does occur normally in the Location
Field and in the Google Field. It does not occur in any text box: instead all
that happens is that the last letter of the shorthand is deleted.

Comment 9

[ronald.gold]

For what it’s worth: while using Version 2005090908 (1.0+)in which Spell Catcher
shorthands do expand normally, Camino crashed unexpectedly when I attempted to
print a page (TalkBack Incident Report: TB9590937E). The curious part is that
Spell Catcher Shorthands did not expand when I tried to use them in the TalkBack
Report fields. On hitting the expansion trigger (Space Bar), nothing happened
other than insertion of a space after the Shorthand. This is somewhat different
than the behavior in Camino 1.0a1 in which the last letter of the shorthand is
deleted.

Comment 10

[timeless]

Incident ID: 9590937 is a plugin crash, please file a new bug w/ the page that 
caused the crash (click on the link, you can get your information from it) and 
the plugin that's on that page.

Comment 11

[ronald.gold]

(In reply to comment #10)
> Incident ID: 9590937 is a plugin crash, please file a new bug w/ the page that 
> caused the crash (click on the link, you can get your information from it) and 
> the plugin that's on that page.

New bug filed: #309539 with all the above details. I don’t not which plugin you
are referring to.

Comment 12

[Simon Fraser [no longer active]]

I've been looking at the Spellcatcher issues; our IME support is basically
broken. I suspect that changing the creator code broke some workarounds that
they had in place. I need to talk to the developers there.

Comment 13

[Evan Gross]

(In reply to comment #12)
> I've been looking at the Spellcatcher issues; our IME support is basically
> broken. I suspect that changing the creator code broke some workarounds that
> they had in place. I need to talk to the developers there.

Hi, right on both accounts. We had a work-around in place for the previous bundle identifier, changing 
it broke that association.

As for the TSM bugs, they've been around for a long, long time. I reported them sometime in 2003, 
they got fixed in Mozilla, but never in Camino. AFAIK, they still exist today.

See bug 198033

Comment 14

[Simon Fraser [no longer active]]

Bugs we have to not regress (from CVS blame):
bug 179308
bug 233288
bug 181198
bug 280894
bug 180589
bug 181947 

Comment 15

[Simon Fraser [no longer active]]

The fix for this would seem to be to always call [super interpretKeyEvents:] in
keyDown: in nsChildView.mm. However, this causes key events to get sent to cocoa
even when Gecko has handled them, for, for example, Control-A both activates the
access key, and causes AppKit to go looking for someone to handle
-moveToBeginningOfParagraph:, fail, and then beep. Similarly, the delete key
both sends the delete into Gecko, and looks for a handler for deleteBackward:
(which, incidentally, we have up on the window controller).

Perhaps our -doCommandBySelector: impl should be the place to filter the event
if it's been handled.

Comment 16

[Simon Fraser [no longer active]]

Attachment: Patch

This patch implements the last suggestion, and cleans up the logic quite a bit.

With the patch we call -interpretKeyEvents for every keyDown (so that input
methods see them), but set a flag if Gecko has already handled the event, and
use that to filter out key binding methods in -doCommandBySelector:.

This seems to work well, and doesn't appear to regress any of the above bugs.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 198890 [details] [diff] [review]
Patch 

+  if (!mIgnoreDoCommand)
+    [super doCommandBySelector:aSelector];

i know you reversed the code, but this check make me think for a minute. Feels
like a double-negative. Would it makes more sense to flip the logic on the bool
to make the if easier to read?

sr=pink either way.

Comment 18

[Simon Fraser [no longer active]]

The basic problem is fixed, but we don't support the more esoteric TSM methods.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Removing obsolete ? flag; sorry for the noise.