Closed Bug 1237236 Opened 4 years ago Closed 4 years ago

In some cases <textarea> displays caret on wrong line, and when I start typing, caret teleports to the next line (caret out of sync with collapsed selection) [1/2]

Categories

(Core :: Selection, defect, minor)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 --- affected
firefox48 --- fixed

People

(Reporter: arni2033, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(6 files)

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160105030211
STR:
1. Open attached "testcase 1", make sure that cursor is blinking at the start of 1st line
2. Press End to move cursor to the end of the line
3. Press Shift+Right to select "\n"
4. Press Right key to move cursor to the end of text selection
5. Type "x"

Result:       
 After Step 4 cursor is displayed at the end of the 1st line
 After Step 5 the typed letter ("x") appears in the beginning of the 2nd line

Expectations (either A or B): 
 A) After Step 4 cursor should be displayed at the start of the 2nd line
 B) After Step 5 the typed letter ("x") should appear in the end of the 1st line
See Also: → 1237286
Summary: In some cases <textarea> displays cursor on wrong line, and when I start typing, cursor teleports to the next line → In some cases <textarea> displays cursor on wrong line, and when I start typing, cursor teleports to the next line [1/2]
Looks like step 3 (shift right) to select "\n" gets things confused.
Blocks: 1237286
Sorry, the regression range isn't correct. I tried the following:
34.0a1 (2014-08-01)
34.0a1 (2014-08-15)
34.0a1 (2014-08-16)
34.0a1 (2014-08-17)
They *all* show the bug: Caret displayed at the end of the first line, x inserted on the second line.

I noticed a small change in behaviour between the 16th and the 17th. The versions of the 1st, 15th and 16th all place the caret onto the second line for a split second, but then it moves to the end of the first line. The version of the 17th doesn't show that effect. There the caret is put at the end of the first line to start with.

BTW, A) is the correct expected outcome to be consistent with IE and Chrome. BTW, those both show a non-collapsed selected (looks like a selected blank) when doing the "shift right". In FF the caret disappears and no selection is shown which isn't so nice either.

I took the liberty do adjust the summary and added:
Caret out of sync with collapsed selection.
(Collapsed selection means that the start and the end of the selection are the same).
No longer blocks: 1048752
Flags: needinfo?(alice0775)
Summary: In some cases <textarea> displays cursor on wrong line, and when I start typing, cursor teleports to the next line [1/2] → In some cases <textarea> displays cursor on wrong line, and when I start typing, cursor teleports to the next line (caret out of sync with collapsed selection) [1/2]
BTW: Searching for "caret" in Bugzilla currently gives more than 300 bugs, more than 50 have the word "position" as well.
Summary: In some cases <textarea> displays cursor on wrong line, and when I start typing, cursor teleports to the next line (caret out of sync with collapsed selection) [1/2] → In some cases <textarea> displays caret on wrong line, and when I start typing, caret teleports to the next line (caret out of sync with collapsed selection) [1/2]
Flags: needinfo?(alice0775)
(In reply to Jorg K (GMT+1) from comment #4)
> I noticed a small change in behaviour between the 16th and the 17th.
I'm pretty sure that's what Alice meant: it became worse (a little). Anyway, I just tested Steps 1-4 with "Caret browsing" on a normal text, and it shows the same bug. It's reproducible since bug 859088
So here are my options (if nobody stops me):
1) To set "Component" -> "Editor" and set this as blocking bug for 859088   [OR]
2) To file new bug "Core -> Editor" with all info above, and set as blocking for 859088 and this bug
Do you have other suggestions?
Please file a bug.
I did the changes you suggested.

Yes, this seem to have something to do with bug 859088. I will back out that change locally and see what happens.

The behaviour of the system depends on the setting of layout.selection.caret_style, for 0 and 2 it's bad as described here, for 1 and 3 it's different (also no good, since no cursor is displayed).

My suggestion is to *not* file a new bug (why should we?).
I will confirm the regression of bug 859088 once my compile is done. I will also test bug 1237286.

Core::Editor is an unowned module, so the chances are slim that this will get fixed.
Blocks: 859088
Component: Layout: Form Controls → Editor
(In reply to Alice0775 White from comment #7)
> Please file a bug.
Please don't. We have enough bugs in the system. Unless someone who analyses this decides that there is a second issue that needs to be treated elsewhere, there is no need for two independent bugs.
Looking at bug 1153237, bug 1077515, I think "Component" should be set to "Selection".
I asked this because I can never be sure about component.
OK, when I take the relevant change from bug 859088 out
(https://hg.mozilla.org/mozilla-central/rev/9a09523eb5dd)
which is a one line change, the behaviour indeed changes for
layout.selection.caret_style = 0. This then starts behaving like setting 1 or 3, which is still incorrect, since it should place the caret at the start of the line, not behind the first character.

In any case, the bad behaviour can still be observed for setting 2.

IMHO, this is just another one of the 300+ caret problems ;-(
Mats, there is an implication that this is your regression. Can you handle it?
Assignee: nobody → mats
Attached file testcase
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #12)
> Mats, there is an implication that this is your regression.

Not really, the bug was already there as you can see from this testcase.
(the bug is in Selection.collapse())

Bug 1237236 made us take a path that leads to a collapse() call here:
http://hg.mozilla.org/mozilla-central/annotate/f0c0480732d3/layout/generic/nsSelection.cpp#l944
Severity: normal → minor
Component: Editor → Selection
Keywords: regressiontestcase
OS: Unspecified → All
Hardware: Unspecified → All
All that bug 859088 did was to use caret style 2 instead of caret style 0 in one case:
https://hg.mozilla.org/mozilla-central/rev/9a09523eb5dd#l1.21
Personally I don't understand what the different caret styles are about and the description is hard to understand, if at all:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2271
As per my comment #11, *all* the caret styles have a problem in this particular testcase, bug 859088 only shifted the problem from one problematic behaviour to another.

IMHO the major problem is that when selecting the "\n" at the end of the line, the cursor disappears altogether. This is not how other browsers behave. They display a selected space at the end of the line to show the user that some white-space was selected.

Also, the caret style behaviour seems to be in need of a cleanup.

Looking at https://wiki.mozilla.org/Modules/Core, Core::Selection is also unowned, and even writing to Mitchell Baker hasn't fixed that yet:
https://groups.google.com/forum/#!topic/mozilla.governance/-0sAFE-_Us4
Attached patch fixSplinter Review
Attachment #8730940 - Flags: review?(bugs)
Attached patch testsSplinter Review
All the orange is unrelated afaict,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc07036e3f1
Comment on attachment 8730940 [details] [diff] [review]
fix

This works for me.

Would you like to have a look at bug 1237286 while you have the box open ;-)

Looks like a change I made in bug 756984 exposed another problem. There is some analysis in bug 1237286 comment #10.

The problem looks similar to this bug. After some action, the caret is placed on the previous line where it should go onto the next line.
Attachment #8730940 - Flags: feedback+
Comment on attachment 8730940 [details] [diff] [review]
fix

I have a bit long review queue right now. Maybe ehsan can help here.
Attachment #8730940 - Flags: review?(bugs) → review?(ehsan)
Comment on attachment 8730940 [details] [diff] [review]
fix

Review of attachment 8730940 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSelection.cpp
@@ +4940,5 @@
> +  if (aParentNode.IsContent()) {
> +    nsTextFrame* f = do_QueryFrame(aParentNode.AsContent()->GetPrimaryFrame());
> +    if (f && f->IsAtEndOfLine() && f->HasSignificantTerminalNewline()) {
> +      int32_t start, end;
> +      f->GetOffsets(start, end);

Nit: you can use GetContentEnd() instead, no need to also retrieve the start offset.

@@ +4942,5 @@
> +    if (f && f->IsAtEndOfLine() && f->HasSignificantTerminalNewline()) {
> +      int32_t start, end;
> +      f->GetOffsets(start, end);
> +      if (end == int32_t(aOffset)) {
> +        mFrameSelection->mHint = CARET_ASSOCIATE_AFTER;

Nit: please use SetHint().
Attachment #8730940 - Flags: review?(ehsan) → review+
Comment on attachment 8730941 [details] [diff] [review]
tests

Review of attachment 8730941 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/test_reftests_with_caret.html
@@ +170,5 @@
>    tests.push([ 'bug966992-2.html' , 'bug966992-2-ref.html' ]);
>    tests.push([ 'bug966992-3.html' , 'bug966992-3-ref.html' ]);
>    tests.push(function() {SpecialPowers.pushPrefEnv({'clear': [['layout.css.overflow-clip-box.enabled']]}, nextTest);});
>  } else {
> +  is(SpecialPowers.getIntPref("layout.spellcheckDefault"), 0, "Spellcheck should be turned off for this platform or this if..else check removed");

Hmm, what changed on this line?
Attachment #8730941 - Flags: review+
> Hmm, what changed on this line?

Just a typo: s/platrom/platform/
https://hg.mozilla.org/mozilla-central/rev/f245074b4fb4
https://hg.mozilla.org/mozilla-central/rev/b3b6173bf031
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Scenario in comment 0 looks fixed on:   Win7_64, Nightly 48, 32bit, ID 20160319030558

But I still can reproduce the same issue. Should I file new bug?
Flags: needinfo?(mats)
Yes, please file a new bug with testcase + steps to reproduce.
Flags: needinfo?(mats)
See Also: → 1258308
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.