Context menu key to fix spellings is on wrong line of textarea

Seems to work for me on my Linux branch build.

I can reproduce in a tinderbox build and my own branch build from this morning. I created a new profile just to test this.

This regressed on branch Linux and Mac between 2006-05-15-04 and 2006-05-16-04. Incidentally, this time period is when the fix for bug 337368 landed on branch!

Check-ins: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-05-15+03&maxdate=2006-05-16+05&cvsroot=%2Fcvsroot

I'm going to back out the patch for bug 337368 and see if this fixes things.

WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060731 BonEcho/2.0b1

Backing out the patch for bug 337368 did not fix this for me.

Confirming previous comments that this does not affect Windows builds (tested 2006-08-02 branch).

I just verified that this is broken on current tinderbox builds for both branch and trunk on both Mac OS X and Linux (FC5).

I also verified that the regression window I mentioned in comment 3 is the same for Mac OS X (on the 1.8 branch).

Created an attachment (id=232021)
testcase

I can reliably reproduce this bug on Windows trunk using this testcase.

(In reply to comment #8)
> I can reliably reproduce this bug on Windows trunk using this testcase.

Yeah, I can reproduce this too on current windows trunk build.

(In reply to comment #9)
> Yeah, I can reproduce this too on current windows trunk build.

However, on current 1.8.1 branch builds, it seems to work just fine.

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

I've just gotten a user report of this on:
Mozilla/5.0 (Windows; I; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061003 Firefox/2.0
If this is reproducibly broken in the 2.0 RCs, that seems bad, though certainly not a blocker -- we'd probably want to try for a .1 fix though.

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

(In reply to comment #13)
> *** Bug 358337 has been marked as a duplicate of this bug. ***

Sorry for the duplicate bug, I'm hopeless at searching in Bugzilla.
What's the hell ?! I don't understand, I can't reproduce the bug anymore... And I don't have any explanation !

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

I can confirm I've seen this problem on the branch (I thought it was only on trunk). It seems not to happen very often though, but it seems like a big problem if it happens at all.

Related is that the context menu doesn't appear if you have the cursor right before the word. This can be annoying for users who select the whole word moving backwards and use the context menu key.

Possibly the fix is if the current pixel does not have a misspelling under it, to check the pixel above and to the right. This might fix both of the problems. Unfortunately, I don't have time to work on this right now.

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

Please note that Windows users can use one of the two Windows key on their keyboard : the Windows application key. Use of this key is very common for people used to keyboard shortcuts. On Windows it was Word that made inline spell checking popular, and the windows application key was created I believe having this particularly in mind.

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.

Spelling suggestions in context-menu depend on cursor-position!

In a fresh profile with only the en-US dictionary open a page with a textarea and paste everything between the quotation marks:

"A nice english text. hjtr
with alot of errors
f gf< djg fxgh xdgt bydt
sdfbgty xt shtdbgtad gteaah tdyxgd he
ADHT 5HZ RYDGRDG Whtsd hrhxf hjtzjs
 hjzfdhj fzcj zdfhj fxchj ztd
gfg jzfjfxth jjdz7 7te gtd dfsg aevgyd gftst hsetrfagtydr earAt rsg xydthzdY 4g
rsaz ysrgt ysgtrrydgt xdth xfhdxyh zeraz hydgysrzt ajhydg Sgt ydxhz zgju <ct hxt
hz chg ctrhz xtch t tfj tfugik zfik ikufzt ted trsht dhfdhj zj rsd hujdtrj hjtr
 hjtr "

- place cursor into last word "hjtr" and invoke context menu via context-menu-key or shift+F10
- notice the first line reads "no suggestions available"
- place cursor into second last word "hjtr"
- spelling is not triggered at all
- jump to first line last word "hjtr" and see the real list of suggestions
- rightclick on any of the "hjtr" and see the expected list of suggestions

This example is highly instable. Back in my normal profile it does not work anymore, but in a fresh profile I could reproduce it. Notice also, that it depends on the direction / location, the cursor is coming from. In addition the lines of garbage prior to the miss-spelling have a major influence. Try adding or deleting some of it.

In addition, I sometimes managed to get suggestions, that clearly belong to the word before or after. In no case I could observe an error on the very first miss-spelling in a textarea. To me it seems, as if the "spelling-cursor" is some 10 characters off from the visibile "text-insertion-character".

Sorry ... forgot to append the version numbers:

Linux 2.6.18.8-0.1-default
openSUSE 10.2
KDE 3.5.5 "release 45.2"
GTK gtk2-2.10.6-24.2
Firefox/2.0.0.2
Gecko/20061023
Java 1.6.0_01-b06

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

immediately fails attachment (id=232021). if backout of bug 337368 didn't fix this, do we know if build of 2006-05-16-04 fails?

last word of comment 19 doesn't fail until removing the trailing space - but in that case the word isn't flagged as mispelled, so we shouldn't expect spell correction recommendations. This behavior is similar to bug 296366 IMO.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre

improve summary

looks like dupe of bug 346930

When I try pressing Shift-F10 on a redlined word using

MS Windows XP, with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9

on a multiline edit box that is several screenfulls down the page,
then the screen flickers like mad for a ~10 seconds.

It looks like the page is jumping repeatedly between the edit box and approx the middle of the screen.

The additional comment box at the bottom of the this page triggers it.

(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.

within text NoWord within text

I could not trigger spell checking by Shift+F10 for the first error above, though it is working on right click. It seems on this system, the spell checking never recognizes a keyboard activation of the context menu.

I have no flicker as described in comment 23.

I cannot reproduce the vanishing red underline as in bug 296366.

System:
Kubuntu Gutsy Gibbon 7.10
uname -a
Linux samson 2.6.22-14-generic #1 SMP Sun Oct 14 23:05:12 GMT 2007 i686 GNU/Linux
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071022 Ubuntu/7.10 (gutsy) Firefox/2.0.0.8
Dictionary en-US

ok, i tested, it seems to be looking one line down:

Test line
dfjlkfsdalk
asdf

when cursor is on test i get "(no spelling suggestions)" when on dfjlkfsdalk i get "asked aside, etc" and i dont get any spelling suggestions on asdf

(In reply to comment #25)
> ok, i tested, it seems to be looking one line down:
>
>
> Test line
> dfjlkfsdalk
> asdf
>
> when cursor is on test i get "(no spelling suggestions)" when on dfjlkfsdalk i
> get "asked aside, etc" and i dont get any spelling suggestions on asdf
>

above only applys to ff3b1. on ff2 (version info below), i cant get any to show with context menu, and cant seem to figure out any pattern

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20061201 Firefox/2.0.0.8 (Ubuntu-feisty)

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

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.

(From update of attachment 420945)
>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');

(In reply to comment #61)
> netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

Awesome. :)

Created an attachment (id=421217)
Mochitest for this bug.

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.

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. :)

Created an attachment (id=421240)
Mochitest for this bug.

(From update of attachment 421240)
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.

(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];

Created an attachment (id=421424)
Mochitest for this bug.

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.

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!

Created an attachment (id=421517)
Mochitest for this bug.

With waitForFocus()

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.

(From update of attachment 421013)
Not approved for the older security branches (1.9.1.8-minus specifically), this does not meet the branch criteria.

Will this reach 3.6.1?

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

Crashes out of MINEFIELD when you envoke the inline spellchecker. (Windows 7, 64bit)

(From update of attachment 420901)
a1922=beltzner, please land this and the mochitest

(From update of attachment 421517)

http://hg.mozilla.org/mozilla-central/rev/d50a6e09b8d0

(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.

Created an attachment (id=431693)
Mochitest for this bug.

Added needed parameters.

test pushed on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c943463925b3
i'll sync test on central now.

synced test: http://hg.mozilla.org/mozilla-central/rev/8dc56c4c00b9

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)