Closed Bug 981281 Opened 10 years ago Closed 10 years ago

Ctrl + left/right arrow skips words with leading/trailing punctuation

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mozilla3, Assigned: mozilla3)

Details

Attachments

(1 file, 4 obsolete files)

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".
Does the same thing happen with Firefox 27?
Flags: needinfo?(mozilla3)
(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)
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)
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.
P.S. layout.word_select.stop_at_punctuation defaults to true in Firefox 24.4.0esrpre
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
(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.
Attached patch Trivial patch (obsolete) — Splinter Review
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.
(Belatedly: the patch is against seamonkey-2.25, but the relevant code appears to be unchanged in the mozilla-central repository.)
(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
Component: General → Layout
Product: SeaMonkey → Core
Version: SeaMonkey 2.24 Branch → Trunk
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)
Er, I think comment 3 + comment 5 confirm that prefs are different between Seamonkey and Firefox -- maybe that should be fixed?
(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 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+
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)
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)
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+
(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)
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.
Submitted bug 996986 regarding the test failure on Linux.
Should be more or less correct, but again, I can't verify this myself.
Attachment #8407285 - Flags: review?(matspal)
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-
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)
Attachment #8407984 - Flags: review?(matspal) → review+
Attached patch fix+testSplinter Review
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
Attachment #8408186 - Flags: review+
Andrew, thanks for fixing this!
Assignee: nobody → mozilla3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7dd1a0534c38
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.