343296 - Spell-checker requires text cursor to be in word to get context menu

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Chris Lawson (gone)]

I can't seem to trigger the spell-checker context menu unless the text insertion point is inside the word in question, or the word is selectd. A Control-click (or right-click) simply gives the normal textarea context menu.

Worse, if the insertion point is in a misspelled word and I right-click on a *different* misspelled word, I get the context menu for the original misspelled word.

STR:

1) Find any page with a textarea.
2) Type in "mispeled werd ". (The trailing space is important.)
3) Control- or right-click on either underlined word and note lack of spellchecker context menu.
4) Put text insertion point inside or immediately adjacent to "mispeled".
5) Control- or right-click on "werd".
6) Wonder why "misspelled" is an option for spelling "werd" properly.

Comment 1

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

This is Camino-only, btw; both cases work correctly in BE.

Comment 2

[froodian (Ian Leue)]

This is a pretty big deal IMO, as it blocks spell-checking a document as you type it - by the time the word is underlined, the cursor is no longer in it.  Requesting blocking for 1.1.

Comment 3

[Mike Pinkerton (not reading bugmail)]

not sure what we can do here, the click doesn't go to gecko before the context menu is displayed.

Comment 4

[Mark Mentovai]

Cocoafox too?

Comment 5

[Samuel Sidler (old account; do not CC)]

This doesn't happen in a Cocoafox build from a couple weeks ago...

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20060726 Minefield/3.0a1

Comment 6

[Stuart Morgan]

*** Bug 357347 has been marked as a duplicate of this bug. ***

Comment 7

[Stuart Morgan]

Attachment: one possibility

What we'd really like is for the word to actually be selected on context-click, since that's what Cocoa generally does.

This patch will do that.  I'm not wild about synthesizing mouse events, but I couldn't find Gecko APIs that do what's needed, and it does work. I'd say the hack is worth it to avoid the mess of "spell check doesn't work in 1.1" reports we'll get if we ship with this broken.

Pink, what do you think before I toss this at reviewers?

Comment 8

[Håkan Waara]

Comment on attachment 242946 [details] [diff] [review]
one possibility

I really don't think we should introduce this hack. We should really try to to fix nsChildView to allow us to do it in a better way.

Comment 9

[Håkan Waara]

A note from discussing on IRC:

If we can just make gecko textfields more mac-like (ctrl-click / right-click should select the clicked-at word), our spellcheck code can assume the selected word is the interesting one.

Comment 10

[Simon Fraser [no longer active]]

If you need to programmatically select words on click, you should be able to do that either by firing off commands, or using nsIDOMSelection APIs.

Comment 11

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

*** Bug 359631 has been marked as a duplicate of this bug. ***

Comment 12

[Stuart Morgan]

I've looked at this again and I don't see how we can do it without either
a) changing core behavior on Mac to select when right-/control-clicking, or
b) copying about two pages of code that sounds really fragile from nsFrame::HandlePress (plus whatever setup code we need that the event chain would do for us in the mouse-down case).

Certainly a) is the right solution, but even if I had the time to figure it out it before we shipped 1.1 (which I don't), it seems unlikely to land on the 1.8 branch anyway (there are at least one ore two places filtering right-clicks before HandlePress, so it would probably involve surgery to a few parts of nsFrame at least). b) seems a whole lot more fragile than simulating the mouse clicks.

For the short term we could try to instead go the route of mimicking Firefox's behavior, which is to spellcheck the word you click on without moving the caret or the selection, but I'm not sure how much easier that would really be than b), and I'm not wild about doing a bunch more work to get worse behavior.

At this point my feeling is that we should land the simulated double-click and open a core bug to (in the longer term) do the right thing on right-click so we can remove the hack later on trunk (and comment the hack to that effect). I lack the core-fu to find a better option, so probably someone else should take a crack at it.

Comment 13

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

Not blocking a2, but we should decide tomorrow if we're taking Stuart's hack for 1.1 and see if we can't get it reviewed and landed for a2.

Comment 14

[Sean Murphy]

Comment on attachment 242946 [details] [diff] [review]
one possibility

This patch would not apply to an unmodified copy of BrowserWindowController.mm at the current trunk revision.  The actual mouse click simulation is inserted into the following conditional statement:

	else if ((contextMenuFlags & nsIContextMenuListener::CONTEXT_INPUT) != 0 ||
        (contextMenuFlags & nsIContextMenuListener::CONTEXT_TEXT) != 0) {

but the trunk lists this check as:

	else if ((contextMenuFlags & nsIContextMenuListener::CONTEXT_INPUT) != 0 ||
         (contextMenuFlags & nsIContextMenuListener::CONTEXT_TEXT) != 0 ||
         isMidas) {

Assuming this patch is in fact destined to land on the trunk, and not one of the branches from which it may actually apply correctly, I fixed up the problem and updated the patch appropriately.

I also went ahead and made a minor spelling correction in one of the patch's comments ("mispelled/misspelled" again).

I looked over and tested the hack, and code-wise it seems fine to me and does get the job done.  The good thing is that text fields will now exhibit identical right-click behavior throughout our app.  Before this, gecko text fields would not select a word upon right clicking it, while native ones elsewhere in our interface would.

So, r=me for using either this patch (with the comment fixed), or the updated one, as a temporary workaround described in comment 12.

I spent a lot of time today trying as well to come up with another way to accomplish this using mozilla APIs.  Like Stuart, I haven't had any luck.  I will keep trying but wanted to get on with a review so we can include something for it in 1.1a2.

Comment 15

[Sean Murphy]

Attachment: Stuart's patch; updated to apply on trunk + spelling correction.

This is Stuart's patch, modified (if needed) so it will apply correctly to the trunk's most recent revision.  I also made a minor spelling correction to 'mispelled' in this comment:

// doesn't consider to be inside of the mispelled range, so we check one character further

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> trunk's most recent revision.  I also made a minor spelling correction to
> 'mispelled' in this comment:

Yeah, after the first couple of times I got compiler errors trying to remember to do one thing in code and another in comments I just gave up and spelled it wrong everywhere.

Comment 17

[Stuart Morgan]

Comment on attachment 250387 [details] [diff] [review]
Stuart's patch; updated to apply on trunk + spelling correction.

Thanks for respinning the patch

>-           isMidas)
>-  {
>+           isMidas) {

This isn't a correct change though; the way it was is correct style for multi-line conditions.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 250387 [details] [diff] [review]
Stuart's patch; updated to apply on trunk + spelling correction.

sr=pink

does the "up" have to be a double-click as well, or can that be a single click? Is there even such a thing as a double-mouse-up event? Seems rather undefined.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Trunk:
mozilla/camino/src/browser/BrowserWindowController.mm 	1.305

Comment 20

[:Gavin Sharp [email: [e-mail]]]

1.8 Branch:
mozilla/camino/src/browser/BrowserWindowController.mm 	1.201.2.100

Comment 21

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

Thanks, Gavin!  You're a hero to thousands of Camino 1.1a2 users out there ;)

We need to make sure that a Core bug is filed somewhere about fixing ctrl-click behavior in text fields on Mac OS X.

Comment 22

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

(In reply to comment #21)
> We need to make sure that a Core bug is filed somewhere about fixing ctrl-click
> behavior in text fields on Mac OS X.

I didn't find such a bug in my searches, so I filed bug 366008.