Closed
Bug 370436
Opened 18 years ago
Closed 15 years ago
Context menu from keyboard for spell checker selects the wrong line
Categories
(Core :: Spelling checker, defect)
Core
Spelling checker
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: bugzille, Assigned: cristiklein)
References
()
Details
(Keywords: polish, regression, verified1.9.2, Whiteboard: [3.6.x])
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
roc
:
review+
beltzner
:
approval1.9.2.2+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
Version: unspecified → Trunk
Comment 1•17 years ago
|
||
This bug is happening in Firefox 2.0.0.4, windows XP SP2 too.
I got this on 2 differents machines.
Comment 2•17 years ago
|
||
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
Comment 3•17 years ago
|
||
looks like dupe of bug 346930
Reporter | ||
Comment 4•17 years ago
|
||
(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.
Comment 5•16 years ago
|
||
(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
Comment 6•16 years ago
|
||
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
Comment 9•16 years ago
|
||
(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
Comment 11•16 years ago
|
||
Maybe the one-line off is a follow-up to bug 337368, which activated the keyboard !?
Comment 12•16 years ago
|
||
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-
Reporter | ||
Comment 13•16 years ago
|
||
(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.
Comment 14•16 years ago
|
||
(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.
Reporter | ||
Comment 15•16 years ago
|
||
(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.
Comment 16•16 years ago
|
||
(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.
Reporter | ||
Comment 17•16 years ago
|
||
(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.
Updated•16 years ago
|
Assignee: mscott → nobody
Comment 20•15 years ago
|
||
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
Reporter | ||
Comment 21•15 years ago
|
||
(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
Comment 22•15 years ago
|
||
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.
Comment 23•15 years ago
|
||
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
Comment 24•15 years ago
|
||
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
Comment 25•15 years ago
|
||
Still affects me, Ubuntu Karmic Koala, Firefox 3.5.5
Updated•15 years ago
|
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
Comment 26•15 years ago
|
||
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?
See Also: → https://launchpad.net/bugs/134813
Comment 27•15 years ago
|
||
(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
Assignee | ||
Comment 28•15 years ago
|
||
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.
Assignee | ||
Comment 29•15 years ago
|
||
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.
Assignee | ||
Comment 30•15 years ago
|
||
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.
Comment 31•15 years ago
|
||
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..)
Comment 32•15 years ago
|
||
(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
Comment 33•15 years ago
|
||
mrbkap, is this covered by your caret experience?
Assignee | ||
Comment 34•15 years ago
|
||
@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
Assignee | ||
Comment 35•15 years ago
|
||
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+
Reporter | ||
Comment 36•15 years ago
|
||
(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]
Updated•15 years ago
|
Assignee: nobody → cristiklein
Comment 40•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a1
Comment 41•15 years ago
|
||
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?
Assignee | ||
Comment 42•15 years ago
|
||
@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?
Assignee | ||
Comment 43•15 years ago
|
||
@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?
Assignee | ||
Comment 44•15 years ago
|
||
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. :(
Comment 45•15 years ago
|
||
(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?
Comment 46•15 years ago
|
||
@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. :)
Comment 47•15 years ago
|
||
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
Comment 48•15 years ago
|
||
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?
Assignee | ||
Comment 49•15 years ago
|
||
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.
Comment 50•15 years ago
|
||
(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.
Comment 51•15 years ago
|
||
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?
Comment 53•15 years ago
|
||
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?
Assignee | ||
Comment 54•15 years ago
|
||
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.
Assignee | ||
Comment 55•15 years ago
|
||
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?
Updated•15 years ago
|
Attachment #420901 -
Flags: approval1.9.2.1?
Updated•15 years ago
|
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 57•15 years ago
|
||
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.
Comment 58•15 years ago
|
||
Will this not reach 3.6, since the RC is already out?
Comment 59•15 years ago
|
||
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.
Assignee | ||
Comment 60•15 years ago
|
||
(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?
Comment 61•15 years ago
|
||
netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
Updated•15 years ago
|
Whiteboard: [3.6.x]?
Assignee | ||
Comment 62•15 years ago
|
||
(In reply to comment #61)
> netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
Awesome. :)
Assignee | ||
Comment 63•15 years ago
|
||
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?
Assignee | ||
Comment 65•15 years ago
|
||
(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. :)
Assignee | ||
Comment 66•15 years ago
|
||
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+
Assignee | ||
Comment 68•15 years ago
|
||
(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;
}
Comment 70•15 years ago
|
||
(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];
Assignee | ||
Comment 71•15 years ago
|
||
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!
Assignee | ||
Comment 73•15 years ago
|
||
With waitForFocus()
Attachment #421424 -
Attachment is obsolete: true
Comment 74•15 years ago
|
||
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?
Assignee | ||
Comment 75•15 years ago
|
||
(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 :-).
Comment 77•15 years ago
|
||
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!
Comment 78•15 years ago
|
||
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 79•15 years ago
|
||
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-
Updated•15 years ago
|
Comment 80•15 years ago
|
||
Will this reach 3.6.1?
Comment 81•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #421013 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #421517 -
Flags: review?(roc)
Attachment #421517 -
Flags: review?(roc) → review+
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [3.6.x]? → [c-n: test to trunk, then both to 1.9.2] [3.6.x]?
Updated•15 years ago
|
Attachment #420901 -
Attachment description: Patch against HEAD which fixes this bug. → Patch against HEAD which fixes this bug
[Checkin: Comment 40]
Comment 82•15 years ago
|
||
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]
Updated•15 years ago
|
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]?
Comment 83•15 years ago
|
||
(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?
Comment 84•15 years ago
|
||
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");
Comment 85•15 years ago
|
||
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
status1.9.2:
--- → .2-fixed
Comment 86•15 years ago
|
||
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)
Comment 87•15 years ago
|
||
(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.
Assignee | ||
Comment 88•15 years ago
|
||
Added needed parameters.
Attachment #421517 -
Attachment is obsolete: true
Comment 89•15 years ago
|
||
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]
Comment 90•15 years ago
|
||
Updated•15 years ago
|
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]
Updated•15 years ago
|
Attachment #431693 -
Attachment description: Mochitest for this bug. → Mochitest for this bug
[Checkin: See comment 82+90 & 89]
Updated•15 years ago
|
Flags: wanted1.9.2?
Comment 91•15 years ago
|
||
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
Comment 92•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•