Closed Bug 370436 Opened 17 years ago Closed 14 years ago

Context menu from keyboard for spell checker selects the wrong line

Categories

(Core :: Spelling checker, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- .2-fixed
status1.9.1 --- wontfix

People

(Reporter: bugzille, Assigned: cristiklein)

References

()

Details

(Keywords: polish, regression, verified1.9.2, Whiteboard: [3.6.x])

Attachments

(2 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070214 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070214 SeaMonkey/1.5a

If spellchecker underlines a misspelled word you have to place the cursor in the word above the misspelled word to get the contextmenu from the keyboard. Rightclick with mouse works fine.

Reproducible: Always

Steps to Reproduce:
1.Write two lines with a misspelled word in the second line
2.spellchecker underlines the misspelled word.
3.place the cursor on the misspelled word and use the key between the right ALT and Ctrl key (Windows keyboard).
4.no corrections for the word are shown.
5.place the cursor in the word above the misspelled word and hit the key
6.the corrections in the context menu are shown.
Actual Results:  
no corrections are shown.

Expected Results:  
the corrections should be shown.

This bug also affects TB3.0a1.

The bug only affects the keyboard action. Rightclick with mouse on the misspelled word works as expected.

Bug appears first in my SM-Trunk-builds from January the 18.
Version: unspecified → Trunk
This bug is happening in Firefox 2.0.0.4, windows XP SP2 too.
I got this on 2 differents machines.
Reproduced on Ubuntu with Firefox 2.0.0.6 - have previously submitted this as https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/134813 on the Ubuntu bug tracker system, and have put a reproduction example at http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html
looks like dupe of bug 346930
(In reply to comment #3)
> looks like dupe of bug 346930

No, I don't think so. 346930 is much older. This bug appeared first in January 07.
346930 is from August 06.
(In reply to comment #4)
> (In reply to comment #3)
> > looks like dupe of bug 346930

At least, the testcase there can be used to reproduce:

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008050906 Minefield/3.0pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008051002 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

> No, I don't think so. 346930 is much older. This bug appeared first in January
> 07.
> 346930 is from August 06.

Setting as "depends on" to investigate the SeaMonkey regression timeframe.
(Eventually, I "hope" to merge them as a (same) Core / Spelling checker bug.)
Severity: normal → major
Status: UNCONFIRMED → NEW
Depends on: 346930
Ever confirmed: true
Keywords: regression
Michaël, Jeremy,
see bug 346930 for (FF) 1.8.1 branch.

*****

(In reply to comment #0)
> Bug appears first in my SM-Trunk-builds from January the 18.

(In reply to comment #4)
> This bug appeared first in January 07.

Ruediger,
could you confirm any of these two dates ?

***

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070101 SeaMonkey/1.5a] (nightly, 2007-01-01-08-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070106 SeaMonkey/1.5a] (nightly, 2007-01-06-08-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070107 SeaMonkey/1.5a] (nightly, 2007-01-07-09-trunk) (W2Ksp4)

Afaict, all these three builds have this bug.
(I will test with older builds...)
Assignee: mail → mscott
Blocks: 346930
Component: MailNews: Main Mail Window → Spelling checker
No longer depends on: 346930
Product: Mozilla Application Suite → Core
QA Contact: spelling-checker
Summary: contextmenu from keyboard for spellchecker selects the wrong line → [1.9 Trunk] Context menu from keyboard for spell checker selects the wrong line
(In reply to comment #5)
> Setting as "depends on" to investigate the SeaMonkey regression timeframe.

It seems I misinterpreted the comments:
this is (a bug, but) not a regression.

(In reply to comment #6)
> (I will test with older builds...)

No spell checking:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060501 SeaMonkey/1.5a] (nightly, 2006-05-01-15-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060616 SeaMonkey/1.5a] (nightly, 2006-06-16-11-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060708 SeaMonkey/1.5a] (nightly, 2006-07-08-10-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060720 SeaMonkey/1.5a] (nightly, 2006-07-20-10-trunk) (W2Ksp4)

Spell checking, but no (mouse) suggestion:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060801 SeaMonkey/1.5a] (nightly, 2006-08-01-10-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060915 SeaMonkey/1.5a] (nightly, 2006-09-15-11-trunk) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060924 SeaMonkey/1.5a] (nightly, 2006-09-24-11-trunk) (W2Ksp4)

(enhancement) bug 338318 checkin !

(keyboard) One line off:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060925 SeaMonkey/1.5a] (nightly, 2006-09-25-11-trunk) (W2Ksp4)
Blocks: 338318
Keywords: regression
Firefox code came from bug 302050...
Blocks: 302050
Maybe the one-line off is a follow-up to bug 337368, which activated the keyboard !?
Blocks: 337368
Flags: wanted1.9.0.x?
Flags: blocking1.9?
Keywords: polish
If this isn't a regression from Firefox 2, then it's not going to be a hard blocker for Firefox 3, either. Definitely something to look into taking on the branch, though.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
(In reply to comment #6)

> > Bug appears first in my SM-Trunk-builds from January the 18.
> 
> > This bug appeared first in January 07.
> 
> Ruediger,
> could you confirm any of these two dates ?

January the 18 definitely was in January 2007, so I can confirm both dates ;-)


> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070101
> SeaMonkey/1.5a] (nightly, 2007-01-01-08-trunk) (W2Ksp4)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070106
> SeaMonkey/1.5a] (nightly, 2007-01-06-08-trunk) (W2Ksp4)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070107
> SeaMonkey/1.5a] (nightly, 2007-01-07-09-trunk) (W2Ksp4)
> 
> Afaict, all these three builds have this bug.
> (I will test with older builds...)

I cant find that old builds for the moment, so I can not check this out again. But I daily load a new SM-trunk-build, and when I wrote, that the bug first appears on January the 18', I had tested, that former builds don't had this bug on my system.
(In reply to comment #13)
> January the 18 definitely was in January 2007, so I can confirm both dates ;-)

Ah, sure: I read "07th" ;-<

> I cant find that old builds for the moment, so I can not check this out again.

<ftp.mozilla.org : /pub/seamonkey/nightly/>

> But I daily load a new SM-trunk-build, and when I wrote, that the bug first
> appears on January the 18', I had tested, that former builds don't had this bug
> on my system.

I didn't test 2007.01.17-18 builds; but my tests identify a much older date.
It would be interesting if you could test/confirm again.
(In reply to comment #14)

> <ftp.mozilla.org : /pub/seamonkey/nightly/>

Thanks, but this builds don't fit my updater-batch, since some files change their directions. Unfortunately the spell-dictionary is one of them :-(

If it's REALLY necessary, I can install it on Wednesday.

> > But I daily load a new SM-trunk-build, and when I wrote, that the bug first
> > appears on January the 18', I had tested, that former builds don't had this bug
> > on my system.
> 
> I didn't test 2007.01.17-18 builds; but my tests identify a much older date.
> It would be interesting if you could test/confirm again.

As I wrote, I tested this builds in February 2007, before I filed the bug, to  found out, that SM-Trunk-Builds before "January the 18'th" don't have this bug.

My suggestion is, that their are two bugs with nearly the same face.

As I wrote in https://bugzilla.mozilla.org/show_bug.cgi?id=370436#c4, MY bug first appears on January the 18'th 2007 and I never had https://bugzilla.mozilla.org/show_bug.cgi?id=346930 on my system.
(In reply to comment #15)
> If it's REALLY necessary, I can install it on Wednesday.
> As I wrote, I tested this builds in February 2007, before I filed the bug, to 
> found out, that SM-Trunk-Builds before "January the 18'th" don't have this bug.

I think it could help if you could double check and report a precise timeframe (and possibly a bug "culprit").
(To compare with my comment 6 and comment 9.)

> My suggestion is, that their are two bugs with nearly the same face.

I agree: see my bug 346930 comment 29.
That is why I confirmed/differentiated this bug and not resolved it as duplicate of the other one.
(In reply to comment #16)

> I think it could help if you could double check and report a precise timeframe
> (and possibly a bug "culprit").

Okay, its done. SM 2007011809 WFM and SM 2007011909 shows the bug.
Tested with the 'Windows-zip-nightlys' from <ftp.mozilla.org : /pub/seamonkey/nightly/>
So the bug appears after 1809 and before 1909 in trunk.

I hope this window can help you to find out, which check-in cause the bug.
Assignee: mscott → nobody
Platform should be changed to All since this bug affects Linux users also. 
Ubuntu bug: https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/134813

Also, I've noticed that this isn't 100% reproducible. Which makes it really weird. For example, right now (on bugzilla), I'm able to get the menu to work properly.

I copied from http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html and pasted into this bugzilla textarea and it gives me Elephant while the cursor(?) is on Elefant. But when I go to http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html itself, I am able to reproduce this bug (reproduce unwanted behavior).

So, change platform to All.
change reproducible to Not Always.

Thank you!

Umang
(In reply to comment #20)

> Platform should be changed to All since this bug affects Linux users also.

Done.

> Also, I've noticed that this isn't 100% reproducible.

Yes.

> I copied from http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html and pasted into
> this bugzilla textarea and it gives me Elephant while the cursor(?) is on
> Elefant. But when I go to http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html
> itself, I am able to reproduce this bug (reproduce unwanted behavior).

Funny, for me its directly opposed. :-)

> change reproducible to Not Always.

I Don't know how....

Thank you for testing.
OS: Windows XP → All
Right now, its quite stable reproducible ... I even shut down Firefox and reopened it:

In the textarea on http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html I select all text, delete it and just type:

<ctrl>+<a><del>Dog<enter>
Elefant<cursor up><shift>+<F10>

Whereas, when I open this bugreport https://bugzilla.mozilla.org/show_bug.cgi?id=370436 and am logged in by cookie, I cannot reproduce the error in this textbox for comments.

Furthermore I have an add-on "Resizeable Textarea". But changing the aspect ratio or the presence of scrollbars does not affect this error.

To me, it seems as if at creation time the textbox decides, if it "wants" to be one row off. Thereafter any edit won't change it: any error on the second row goes to the contextmenue of the first row.
Still an issue with Shiretoko (firefox-3.5 package from the Ubuntu Mozilla Security PPA). This is still very irritating....

Finally, even with Shiretoko, good behavior is seen on bugzilla and bad (unwanted/bug) behavior on http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html

Umang
I get the same bug also in Thunderbird 3.0 beta 4. It happens always to me in Thunderbird, but not always in Firefox (3.5.4). Perhaps interesting tested for a few month Postbox, and had the same bug also in there.

Thanks
MaX
Still affects me, Ubuntu Karmic Koala, Firefox 3.5.5
status1.9.1: --- → ?
Summary: [1.9 Trunk] Context menu from keyboard for spell checker selects the wrong line → [1.9.1 Trunk] Context menu from keyboard for spell checker selects the wrong line
This is really frustrating me. There seems to be no progress on this. Even Thunderbird 3.0 has this bug.

Can we have an update on whether this is being worked out?
(In reply to comment #17)
> Okay, its done. SM 2007011809 WFM and SM 2007011909 shows the bug.
> Tested with the 'Windows-zip-nightlys' from <ftp.mozilla.org :
> /pub/seamonkey/nightly/>
> So the bug appears after 1809 and before 1909 in trunk.

Ruediger, are you sure that this is the real time frame? I believe you have tested it inside the mail window, right? In your given time frame there is only one check-in which is related to the spell checker and this is bug 355064.

So I believe the underlying issue is inside the toolkit spell checker and has been broken earlier. Would you have time to test builds before that date?
Keywords: regression
Hardware: x86 → All
Hello,

I have been spending some time figuring out this issue and deduced the following. My opinion is that the "simulated right-click" happens a few pixels too low.

The events happen like this:

- The native back-end (e.g. GTK) is receiving a KeyPressEvent. If the "Context Menu" key is pressed, it will convert this into a nsMouseEvent, with coordinates (0,0) and the context type nsMouseEvent::eContextMenuKey (mozilla/widget/src/gtk2/nsWindow.cpp @ 3296)
- As the event propagates, nsEventListenerManager::FixContextMenuEvent() is called, which sets the coordinates to that of the caret, using PrepareToUseCaretPosition (mozilla/content/events/src/nsEventListenerManager.cpp @ 1422)

I doubt that the nsCaret's coordinates are wrong (else this might also create weird drawing artifacts), so I think the problem is related to how PrepareToUseCaretPosition processes this value.

I haven't got any more energy to study this, so it would be really cool if somebody could take over.
This patch has been tested against thunderbird-3.0 3.0.1~hg20091121r4400+nobinonly-0ubuntu1~umd1~karmic from 
the PPA for Ubuntu Mozilla Daily Build Team.
Hello everybody,

It seems I found the extra energy after all. :)

I attached a patch which moves the "simulated right-click" one pixel higher than before. I tested it against Thunderbird 3.0.1pre and it works.

I personally think the fix is ugly and the reason for the required hack should be further studied. However, seeing how many people are frustrated, it is a good temporary solution.

Could anybody see if this patch also works for FF? Thanks.
Why only one pixel? It's off by one full line isn't it? How does it still work? Is this why this bug is not 100% reproducible?

(I hate it when I can only ask more questions without giving any answers..)
(In reply to comment #29)
> Created an attachment (id=420730) [details]
> Ugly, but working proposed patch.
> 
> This patch has been tested against thunderbird-3.0
> 3.0.1~hg20091121r4400+nobinonly-0ubuntu1~umd1~karmic from 
> the PPA for Ubuntu Mozilla Daily Build Team.

Neil, could you have a look at? Can we do that somehow this way?
Summary: [1.9.1 Trunk] Context menu from keyboard for spell checker selects the wrong line → Context menu from keyboard for spell checker selects the wrong line
mrbkap, is this covered by your caret experience?
@Umang: Each text line has a bounding rectangle. It seems to me that FF / TB put the "simulated right-click" one pixel too low. Therefore, instead of "clicking" the lower edge of the expected text line, it click the upper edge of the text line underneath it.


I think I know the root of the problem. The caret position is stored in twips which are a much more precise unit that pixels. The PrepareToUseCaretPosition() first gets the lowest point of the caret (in twips) then converts it into pixels. This is done using round-to-nearest [1][2], which can return a pixel value outside the bounding rectangle of the expected text line.

This would also explain why the bug is not always hit. Different text sizes / scroll positions might determine rounding in the right direction.

I propose one of the following solutions:

1) Don't "click" on the lower edge of the caret. Click at say 80% or 90% of it.
2) Provide a round-down feature for AppUnitsToDevPixels.
3) (What my patch does) subtract 1, just to make sure.
4) Detect if round-up occurred (check if xInPixels * TwipsPerPixel == xInTwips) and compensate.

I could invest some time and implement these solutions, provided they would be accepted and included in the Mozilla Source Code.

[1] mozilla/mozilla/layout/base/nsPresContext.h @ 561
[2] mozilla/mozilla/gfx/public/nsCoord.h @ 330
I attached a patch against HEAD to fix this bug.

Rationale: when a caret exists and the context menu is activated from the keyboard, FF simulates a right-click at the lowest coordinate of the caret. Since the caret's rectangle is stored in twips while mouse coordinates are in pixels, a conversion must happen between these two. AppUnitsToDevPixels does this using round-to-nearest, which however, might return a pixel which belong to the text line below the caret.

Instead of implementing a new conversion function which returns a round-down value, this patch offers a less intrusive solution. Rounding "compensation" takes place in the function that should be concerned with this, right after the conversion takes place.

Tested with Firefox (today's HEAD) on Ubuntu 9.10 with http://wwwb.forbidden.co.uk/~jbj/firefoxbug.html.

Could somebody please help me on how to get someone to review and check this in?

Thanks.
Attachment #420730 - Attachment is obsolete: true
Attachment #420901 - Flags: review+
(In reply to comment #27)

> > So the bug appears after 1809 and before 1909 in trunk.
> 
> Ruediger, are you sure that this is the real time frame?

Yes. Two times double checked.
Comment on attachment 420901 [details] [diff] [review]
Patch against HEAD which fixes this bug
[Checkin: Comment 40 & 85]

you can't set review+, it hasn't been reviewed by someone appropriate
Attachment #420901 - Flags: review+
Comment on attachment 420901 [details] [diff] [review]
Patch against HEAD which fixes this bug
[Checkin: Comment 40 & 85]

But I can. This makes complete sense. Thanks!!!
Attachment #420901 - Flags: review+
We need a mochitest for this using EventUtils.js and synthesizeMouse. However, we can check this in as-is.
Keywords: checkin-needed
Whiteboard: [needs landing]
Assignee: nobody → cristiklein
http://hg.mozilla.org/mozilla-central/rev/e3dd7678ff3f
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a1
Well done! It took so long to find the bug, but in a couple of days of finding the problem you guys have fixed it. Thank you. This was probably the one bug that really irritated me. Can we expect this is 3.5.8?
@roc: Sorry for setting review+, I thought it meant "needs reviewing". Thanks for checking the code so quickly. Also, thanks for giving me the hints on writing the mochitest. I will attach it soon.

@Umang: Since you are on Ubuntu, why don't you use Mozilla Daily PPA?
@roc: Sorry for setting review+, I thought it meant "needs reviewing". Thanks for checking the code so quickly. Also, thanks for giving me the hints on writing the mochitest. I will attach it soon.

@Umang: Since you are on Ubuntu, why don't you use Mozilla Daily PPA?
Attached patch Mochitest for this bug. (obsolete) — Splinter Review
This test contains a textarea with three lines of misspelled words. Using only synthesized keyboard events it tries to correct the word in the middle. At the end, it checks whether the content of the textarea is the expected one.

This test requires focus, or else, it won't even finish. :(
(In reply to comment #41)
> Well done! It took so long to find the bug, but in a couple of days of finding
> the problem you guys have fixed it. Thank you. This was probably the one bug
> that really irritated me. Can we expect this is 3.5.8?

This was checked in to the trunk, which currently is Firefox 3.7 Alpha 1. Is this important enough to be pushed to the 3.5 branch? Should this be checked in for 3.6, or should it start applying from Firefox 3.7 and onwards?
@Cristian: I share the computer with not-so-tech-savy people also. They've "had problems" with beta earlier, like it was called "Shiretoko". But it's not like I'm in a hurry to get this. Not so much that I'll install dailies.

I guess Mozilla will have their own policy on what to include in which release. I'm not interfering. I'm psychologically satisfied that the fix has been committed, even if it will not be released very soon. Thank you for the fix, though. :)
Hi to all an thank you for the efforts. It looks like a solution is finally in sight. For me it is strange since it affects only Thunderbird (running 3.0) but not Firefox (running 3.5.7 right now).  

I think it would be great if this fix could checked in to some previous Gecko (the one used in Thunderbird) as well. 

Sorry if this is totally nontechnical, but i have no clue :-).. i am just a stupid user.

Max
There's no such thing as a stupid user, just very unexperienced ones (which you are not, as you've made it all the way to Bugzilla :) )

I am not sure about if this change risks breaking something, or if it relies on something that is only implemented in Firefox 3.6 and 3.7. The release of Firefox 3.6 is very close, which means two things.

1. As the release of 3.6 is so close, checking it in to 3.5 is a bit pointless.
2. As the release of 3.6 is so close, a fix like this might not be allowed to be checked in to 3.6 either as it might break features.

As it is not up to me what gets pushed, I'd like to ask those with the authority, is this going to be checked in for 1.9.2/3.6?
I would offer to create similar patches against FF 3.5, FF 3.6, TB 3.0 and TB 3.1, if roc would be kind enough to review and check them in.
(In reply to comment #49)
> I would offer to create similar patches against FF 3.5, FF 3.6, TB 3.0 and TB
> 3.1, if roc would be kind enough to review and check them in.

TB shares parts of its code with FF, so only need to patch FF.
For those based on 1.9.2, the current patch probably just needs an approval request submitting as the code in 1.9.3 is very similar.
For those based on 1.9.1, the code is very different to 1.9.3, so, if the bug exists there, would need a new patch and review and approval.
Sounds like a lot of work to support on 1.9.1 then. My opinion is that checking it in for 1.9.2 is enough, which can't be more than a month away, even in a worst case scenario (right?).
(In reply to comment #44)
> Created an attachment (id=420945) [details]
> Mochitest for this bug.
> 
> This test contains a textarea with three lines of misspelled words. Using only
> synthesized keyboard events it tries to correct the word in the middle. At the
> end, it checks whether the content of the textarea is the expected one.
> 
> This test requires focus, or else, it won't even finish. :(

EventUtils has "waitForFocus" for that.

+	/* Correct "hellow" */
+	synthesizeMouse(ta, 0, 0, { type : "contextmenu" });
+	synthesizeKey("VK_DOWN", {})
+	synthesizeKey("VK_ENTER", {})

This is a bit of a problem, it will break if someone changes the layout of the context menu.

I think what you really care about here is the coordinates in the contextmenu event. I think the right thing to do is to check the clientX/clientY coordinates in the event. The tricky part is to make sure that your test fails without the patch, and passes with the patch.

+	/* Verify result */
+	synthesizeKey("VK_TAB", {})
+	synthesizeKey("VK_SPACE", {})

Instead of doing that, why not just call verifyResult directly?
There is already wanted1.9.0.x+ I thought it should take care.
Just in case it dont, I have requested a wanted1.9.2
Flags: wanted1.9.2?
Attached patch Patch against 1.9.1. (obsolete) — Splinter Review
This patch is functionally identical to the on against HEAD. The only difference is that the PrepareToUseCaretPosition() function moved from one file to the other.
I just tested the patch I submitted against HEAD and it cleanly applied to 1.9.2.

@roc: Would you please be so kind and fix this bug in both 1.9.1 and 1.9.2, so that everybody is happy?
Attachment #420901 - Flags: approval1.9.2.1?
Attachment #421013 - Flags: approval1.9.1.8?
That's not actually my decision. Let's wait for branch drivers to respond to your approval request. Make sure this bug contains an explanation of why this bug is important enough to land on a stable branch. I can testify that the patch is low risk.
Comment on attachment 420945 [details] [diff] [review]
Mochitest for this bug.

>diff -r 6a7294d0f305 content/events/test/Makefile.in
>@@ -77,16 +77,17 @@
> 		test_bug517851.html \
>+		test_bug370436.html \

Please, keep it sorted.
Will this not reach 3.6, since the RC is already out?
There will be one more RC, but taking this in might be considered risky. It could come with 3.6.1 if it doesn't make 3.6RC2.
(In reply to comment #52)
> (In reply to comment #44)
> > Created an attachment (id=420945) [details] [details]
> > Mochitest for this bug.
> > 
> > This test contains a textarea with three lines of misspelled words. Using only
> > synthesized keyboard events it tries to correct the word in the middle. At the
> > end, it checks whether the content of the textarea is the expected one.
> > 
> > This test requires focus, or else, it won't even finish. :(
> 
> EventUtils has "waitForFocus" for that.
> 
> +    /* Correct "hellow" */
> +    synthesizeMouse(ta, 0, 0, { type : "contextmenu" });
> +    synthesizeKey("VK_DOWN", {})
> +    synthesizeKey("VK_ENTER", {})
> 
> This is a bit of a problem, it will break if someone changes the layout of the
> context menu.
> 
> I think what you really care about here is the coordinates in the contextmenu
> event. I think the right thing to do is to check the clientX/clientY
> coordinates in the event. The tricky part is to make sure that your test fails
> without the patch, and passes with the patch.

Hello,

I think that what I really care about is the word on which the oncontextmenu occurred, as can be deduced from rangeParent and rangeOffset (this is what inlineSpellChecking uses). Unfortunately I have been unable to access this information. No matter what attribute I try to access on rangeParent, I get errors like:

Error: Permission denied to access property 'data' from a non-chrome context

Any pointers?
netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
Whiteboard: [3.6.x]?
(In reply to comment #61)
> netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

Awesome. :)
Attached patch Mochitest for this bug. (obsolete) — Splinter Review
This test moves the cursor over a textarea and synthesizes a keyboard-style oncontextmenu. It then checks that the words returned by rangeParent are the correct ones.

Fails without the fix, succeeds with the fix.
Attachment #420945 - Attachment is obsolete: true
We're getting close but not quite there yet :-)

+	var node = e.rangeParent;
+	var offset = e.rangeOffset;
+	words.push(node.data);

You're relying on the fact that the textarea breaks up each line into independent text nodes. That's not guaranteed, and in fact we're likely to change this soon. Here, you really should use 'offset' and check that the characters at 'offset' are the word you're expecting. Make sense?
(In reply to comment #64)
> We're getting close but not quite there yet :-)
> 
> +    var node = e.rangeParent;
> +    var offset = e.rangeOffset;
> +    words.push(node.data);
> 
> You're relying on the fact that the textarea breaks up each line into
> independent text nodes. That's not guaranteed, and in fact we're likely to
> change this soon. Here, you really should use 'offset' and check that the
> characters at 'offset' are the word you're expecting. Make sense?

Yes. Deep down inside I was afraid you might say that. :)
Attached patch Mochitest for this bug. (obsolete) — Splinter Review
Attachment #421217 - Attachment is obsolete: true
Comment on attachment 421240 [details] [diff] [review]
Mochitest for this bug.

OK, but you can vastly simplify the code by using a better regular expression. E.g.:

<script>
function offsetToWord(data, offset) {
  var rest = data.substr(offset);
  var m = rest.match(/^\w+/);
  return m ? m : "";
}
function test(data, offset) {
  document.write("offsetToWord(\"" + data + "\"," + offset + ") = \"" + offsetToWord(data, offset) + "\"<br>");
}
test("hello kitty", 0);
test("hello kitty", 2);
test("hello kitty", 5);
test("hello kitty", 6);
test("hello kitty", 8);
</script>

Please revise the patch like that and then we can get this landed.
Attachment #421240 - Flags: review+
(In reply to comment #67)
Hello,

The code you wrote does not exactly do what I intended. More precisely, it does not extend the range to the left, so as to include the whole word. I would expect offsetToWord('hello kitty', 2) to return 'hello'.

I intended my code like this, because in future I might want to test whether the contextmenu works correctly in the middle of the word or between words ('hello| world' + contextmenu should return 'hello' and not 'world').

However, for the current bug, the mochitest might seem a little over-engineered. Do you still think I should simplify the code?
OK then, just use this:

function offsetToWord(data, offset) {
  var m1 = data.substr(0, offset).match(/\w+$/) || "";
  var m2 = data.substr(offset).match(/^\w+/) || "";
  return m1 + m2;
}
(In reply to comment #69)
> function offsetToWord(data, offset) {
>   var m1 = data.substr(0, offset).match(/\w+$/) || "";
>   var m2 = data.substr(offset).match(/^\w+/) || "";
Unfortunately all of the regexp methods return an array of matches (including any parenthetical captures) rather than the matching string itself. You could however use \w* instead of \w+ to force a match to exist, e.g.
var m1 = data.substr(0, offset).match(/\w*$/)[0];
Attached patch Mochitest for this bug. (obsolete) — Splinter Review
Nice way of selecting the closest word.

BTW, while writing the test case I observed that there is another bug in FF. Opening the context menu at the end of the line returns rangeParent.data "undefined". Since this is out of the scope of the current bug and since it isn't as annoying, I did not include tests for it in the mochitest.
Attachment #421240 - Attachment is obsolete: true
Looks good, but one more thing! You should use SimpleTest.waitForFocus to start the test.

(In reply to comment #71)
> BTW, while writing the test case I observed that there is another bug in FF.
> Opening the context menu at the end of the line returns rangeParent.data
> "undefined". Since this is out of the scope of the current bug and since it
> isn't as annoying, I did not include tests for it in the mochitest.

I suspect this is not actually a bug. I suspect the rangeParent is a <br> element. This probably isn't a good thing to return, but it doesn't matter too much since the details of what a textarea contains are not visible to Web content.

Thanks!
With waitForFocus()
Attachment #421424 - Attachment is obsolete: true
I don't know too much about all this so don't even take my comments as suggestions - just take them as questions.

Why does Firefox chose the bottom most pixel to right click on? When I right click a word to get suggestions by spell check, I click in the middle of the character. So shouldn't you actually subtract 50% of the line height or the text size (I really don't know which one I mean) from the bottom most pixel? I guess -1 pixel is also fine, because no one will type with a font size of 1 pixel and we will almost never jump _up_ a line, but -(0.5*line-height) would make more sense to me. What's the reasoning behind using the bottom of the Caret rather than the middle?
(In reply to comment #74)
> What's the reasoning behind using the bottom of the
> Caret rather than the middle?

I suppose the original designer had UI usability in mind. If the context menu opens downwards and its origin is at the bottom of the caret, you can still see the whole word / line to which the context menu applies. If, for example, you misspelled a word, you might want to re-read that line with one of the suggestions, to make sure it is the right one.

Of course, it would be totally cool if the top of the caret was taken as the origin when the context menu opens upwards. :D
(In reply to comment #75)
> I suppose the original designer had UI usability in mind. If the context menu
> opens downwards and its origin is at the bottom of the caret, you can still see
> the whole word / line to which the context menu applies. If, for example, you
> misspelled a word, you might want to re-read that line with one of the
> suggestions, to make sure it is the right one.

Yeah, absolutely.

> Of course, it would be totally cool if the top of the caret was taken as the
> origin when the context menu opens upwards. :D

Our popup menus do have support for an "anchor rect" which will do this. Patches accepted :-).
Well that problem too isn't really solved properly, because if you scroll up a little, so that this textarea is at the bottom of your screen and the open the context menu, the menu pops up from the bottom of the text and covers the word anyway. Textareas like these also tend to be at the bottom of the page, so that logic (that the word shouldn't get covered) is still not implemented properly. But yes, a popup menu that actually pops up and not down would be awesome!
I know this has been pretty much rapped up now, but I have found that increasing the text by a size on the pages that give trouble, will get things working as they should. I have Firefox set to "only zoom text" I'm not sure if this makes a difference. Also you can do this while typing, say, if this was one of the areas of text that wasn't working properly and suggestions were not being brought up with the menu key. Then I could increase the font by a size to get it to work and without having to navigate away from the page or restart Firefox. If you change the font back to a size smaller, it will stop working again. 

It's not ideal, but it works, and we have a proper fix for this already. I just thought I'd mention it as a temporary solution for those of us who are eagerly waiting the proper fix.
Comment on attachment 421013 [details] [diff] [review]
Patch against 1.9.1.

Not approved for the older security branches (1.9.1.8-minus specifically), this does not meet the branch criteria.
Attachment #421013 - Flags: approval1.9.1.8? → approval1.9.1.8-
Will this reach 3.6.1?
Comment on attachment 420901 [details] [diff] [review]
Patch against HEAD which fixes this bug
[Checkin: Comment 40 & 85]

a1922=beltzner, please land this and the mochitest
Attachment #420901 - Flags: approval1.9.2.2? → approval1.9.2.2+
Attachment #421013 - Attachment is obsolete: true
Attachment #421517 - Flags: review?(roc)
Keywords: checkin-needed
Whiteboard: [3.6.x]? → [c-n: test to trunk, then both to 1.9.2] [3.6.x]?
Attachment #420901 - Attachment description: Patch against HEAD which fixes this bug. → Patch against HEAD which fixes this bug [Checkin: Comment 40]
Comment on attachment 421517 [details] [diff] [review]
Mochitest for this bug
[Checkin: Comment 82]


http://hg.mozilla.org/mozilla-central/rev/d50a6e09b8d0
Attachment #421517 - Attachment description: Mochitest for this bug. → Mochitest for this bug [Checkin: Comment 82]
Flags: in-testsuite? → in-testsuite+
Whiteboard: [c-n: test to trunk, then both to 1.9.2] [3.6.x]? → [c-n: both to 1.9.2] [3.6.x]?
(In reply to comment #82)
> http://hg.mozilla.org/mozilla-central/rev/d50a6e09b8d0

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267621810.1267624217.16711.gz&fulltext=1
WINNT 5.2 mozilla-central opt test mochitests-4/5 on 2010/03/03 05:10:10

7 times:
... INFO TEST-PASS | .../test_bug370436.html | undefined
}

Something is wrong, no?
these should either be ok(), or have a message.

    2.70 +	is(words.pop(), "welcome");	
    2.71 +	is(words.pop(), "world");	
    2.72 +	is(words.pop(), "hello");	
    2.73 +	is(words.pop(), "hello");	
    2.74 +	is(words.pop(), "sheep");	
    2.75 +	is(words.pop(), "sheep");	
    2.76 +	is(words.pop(), "sheep");
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f4c287243458

i skipped the mochitest because the chatter in comment 83 / comment 84 is scary
Thank you! Wow, three years and one month! 

(This might make 3.6.2 one of the most exciting releases for me, this bug has irritated me for a long time). I guess it will take a while before it is fixed in TB right? (Guessing from https://developer.mozilla.org/en/Gecko)
(In reply to comment #85)
> i skipped the mochitest because the chatter in comment 83 / comment 84 is scary

We should fix comment 84 (by adding comments), but it shouldn't block landing the test on the branch.
Added needed parameters.
Attachment #421517 - Attachment is obsolete: true
test pushed on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c943463925b3
i'll sync test on central now.
Keywords: checkin-needed
Whiteboard: [c-n: both to 1.9.2] [3.6.x]? → [3.6.x]
Attachment #420901 - Attachment description: Patch against HEAD which fixes this bug [Checkin: Comment 40] → Patch against HEAD which fixes this bug [Checkin: Comment 40 & 85]
Attachment #431693 - Attachment description: Mochitest for this bug. → Mochitest for this bug [Checkin: See comment 82+90 & 89]
Flags: wanted1.9.2?
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 (.NET CLR 3.5.30729)
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.