Closed
Bug 981281
Opened 11 years ago
Closed 11 years ago
Ctrl + left/right arrow skips words with leading/trailing punctuation
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mozilla3, Assigned: mozilla3)
Details
Attachments
(1 file, 4 obsolete files)
10.36 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
On Linux, pressing Ctrl+Left or Ctrl+Right in a text field will cause the cursor to skip over words with punctuation at the beginning or end of the word, respectively. For example, given the text "this (is a) test", repeatedly pressing Ctrl+Left when the cursor is at the end of the line will move the cursor to immediately before "test", then "a", then "this", skipping the word "is". Similarly, Ctrl+Right from the beginning of the line moves the cursor after "this", "is", and then "test", skipping "a".
This does not occur on Mac OS X using the equivalent key combinations (Option+Left or Option+Right); the cursor properly stops on each word regardless of any leading or trailing punctuation.
Steps to reproduce:
1. Enter "abc (def) ghi" in an empty Location bar.
2. Press Ctrl+Left twice.
Expected behavior:
Cursor is positioned immediately before either the "(" or the "d". (I would consider either to be correct behavior, though I think moving the cursor to before the punctuation is more common.)
Observed behavior:
Cursor is positioned before the "a".
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Philip Chee from comment #1)
> Does the same thing happen with Firefox 27?
No, Firefox 27 seems to work fine. In 27.0.1, Ctrl+Left does this: (| indicates cursor position)
abc (def) ghi|
abc (def) |ghi
abc (|def) ghi
|abc (def) ghi
Flags: needinfo?(mozilla3)
Comment 3•11 years ago
|
||
Isn't this the purpose of the about:config pref layout.word_select.stop_at_punctuation (Boolean, default false)?
After toggling this pref (by double-click), I get the following:
"this (is a) test",
]←←←←←]←←]←←]←←←←[
]→→→→[→→→[→→[→→→→→→[
halting on the right side (in both directions) of a punctuation mark if present.
This makes me think the behaviour in comment #0 (with the pref left at its "false" default) is intentional, which would mean that "t'ain't a bug, it's a feature" (bug INVALID)
Comment 4•11 years ago
|
||
N.B. I see another pref, layout.word_select.eat_space_to_next_word, also default false, which will include the space on one side when double-clicking a word to select it.
Comment 5•11 years ago
|
||
P.S. layout.word_select.stop_at_punctuation defaults to true in Firefox 24.4.0esrpre
Comment 6•11 years ago
|
||
P.P.S.
Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 SeaMonkey/2.27a1 ID:20140214003001 c-c:1ce77c2f9bd0 m-c:d275eebfae04
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20140407 Firefox/24.0 ID:20140407000502 CSet:c7d60c371ec3
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Tony Mechelynck [:tonymec] from comment #3)
> Isn't this the purpose of the about:config pref
> layout.word_select.stop_at_punctuation (Boolean, default false)?
The description of the pref (http://kb.mozillazine.org/Layout.word_select.stop_at_punctuation) says:
"Word" selection includes surrounding punctuation and only stops at whitespace [...]
but the behavior described in comment #0 skips spaces as well, so that would seem to be a bug.
Assignee | ||
Comment 8•11 years ago
|
||
That pref did point me to the relevant part of the code, so here's a patch that fixes the problem. I don't know the code well enough to tell whether it might break anything else.
Assignee | ||
Comment 9•11 years ago
|
||
(Belatedly: the patch is against seamonkey-2.25, but the relevant code appears to be unchanged in the mozilla-central repository.)
Comment 10•11 years ago
|
||
(In reply to Andrew Church from comment #9)
> (Belatedly: the patch is against seamonkey-2.25, but the relevant code
> appears to be unchanged in the mozilla-central repository.)
Hang on while I move this bug to Core::Layout
Updated•11 years ago
|
Component: General → Layout
Product: SeaMonkey → Core
Version: SeaMonkey 2.24 Branch → Trunk
Comment 11•11 years ago
|
||
Comment on attachment 8404696 [details] [diff] [review]
Trivial patch
OK now we need to get the patch reviewed. So I set the review flag to ? and the Requestee to dbaron. You can get some suggestions from the (suggested reviewers) popup
Attachment #8404696 -
Flags: review?(dbaron)
Comment on attachment 8404696 [details] [diff] [review]
Trivial patch
Transferring review request to Mats.
Though I'm curious if this bug occurs only in Seamonkey and not in Firefox -- and if so, if that's because prefs are set differently?
Attachment #8404696 -
Flags: review?(dbaron) → review?(matspal)
Comment 14•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #13)
> Er, I think comment 3 + comment 5 confirm that prefs are different between
> Seamonkey and Firefox -- maybe that should be fixed?
What about comment 7
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 15•11 years ago
|
||
Comment on attachment 8404696 [details] [diff] [review]
Trivial patch
Looks good to me, although the code comment could be more accurate.
I fixed that and submitted it to Try:
https://tbpl.mozilla.org/?tree=Try&rev=d16b284c7aaa
Attachment #8404696 -
Flags: review?(matspal) → review+
Comment 16•11 years ago
|
||
Andrew, do you agree the updated comment is more accurate?
(the space is after the punctuation when aForward is true,
or before the punctuation when aForward is false)
Attachment #8404696 -
Attachment is obsolete: true
Attachment #8407159 -
Flags: review?(mozilla3)
Comment 17•11 years ago
|
||
The Try run looks green so far, so I guess we have no tests for this.
We should add some here: layout/generic/test/test_movement_by_words.html
http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_movement_by_words.html?force=1
It seems we're only testing two combinations:
setPrefs(false, true, test1);
setPrefs(true, true, test2);
where the 1st param is layout.word_select.eat_space_to_next_word
and the 2nd param is layout.word_select.stop_at_punctuation
We should probably just copy-paste the current test1/test2 into test3/test4
and set false for the 2nd param, and then tweak the numbers so that it passes
with the patch applied.
Andrew, would you be interested in doing that?
Flags: needinfo?(mozilla3)
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 8407159 [details] [diff] [review]
patch with update comment and commit message
Review of attachment 8407159 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, thanks for cleaning that up.
Attachment #8407159 -
Flags: review?(mozilla3) → review+
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #17)
> The Try run looks green so far, so I guess we have no tests for this.
> We should add some here: layout/generic/test/test_movement_by_words.html
[...]
> Andrew, would you be interested in doing that?
Sure, I'll do that.
Flags: needinfo?(mozilla3)
Assignee | ||
Comment 20•11 years ago
|
||
It appears that test_movement_by_words.html itself is broken, see bug 916143. (The test appears to be disabled on Linux/GTK+ because Mozilla uses the GTK+ key modifier settings, but my system uses the default of Ctrl+Left/Right, and if I re-enable the test I get the behavior described in that bug.)
I can still write the tests, but I have no way to verify that they're correct.
Assignee | ||
Comment 21•11 years ago
|
||
Submitted bug 996986 regarding the test failure on Linux.
Assignee | ||
Comment 22•11 years ago
|
||
Should be more or less correct, but again, I can't verify this myself.
Attachment #8407285 -
Flags: review?(matspal)
Comment 23•11 years ago
|
||
Comment on attachment 8407285 [details] [diff] [review]
Tests for stop_at_punctuation=false (untested)
Thanks for adding tests!
I pushed this to Try and there were a few failures:
https://tbpl.mozilla.org/?tree=Try&rev=f8de607ecfff
Attachment #8407285 -
Flags: review?(matspal) → review-
Assignee | ||
Comment 24•11 years ago
|
||
Okay, I see what I did wrong: I didn't realize the character positions are independent for each text segment. This version should fix it -- I managed to get an OSX build running and the tests seem to pass now (and fail in the expected way without the nsFrame.cpp patch). PTAL.
Attachment #8407285 -
Attachment is obsolete: true
Attachment #8407984 -
Flags: review?(matspal)
Updated•11 years ago
|
Attachment #8407984 -
Flags: review?(matspal) → review+
Comment 25•11 years ago
|
||
This is the combined patch which I pushed to Try - it looks fine now.
https://tbpl.mozilla.org/?tree=Try&rev=8274f43338c1
Attachment #8407159 -
Attachment is obsolete: true
Attachment #8407984 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8408186 -
Flags: review+
Comment 26•11 years ago
|
||
Andrew, thanks for fixing this!
Assignee: nobody → mozilla3
Keywords: checkin-needed
Comment 27•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 28•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•