1237236 - 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]

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[arni2033]

Attachment: testcase 1 - textarea displays cursor on wrong line.htm

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

Comment 1

[arni2033]

Attachment: screencast 1 - textarea displays cursor on wrong line.webm

Comment 2

[Alice0775 White]

Regression pushlog against A):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b8041e7ee525&tochange=fa1ea174e770

Triggered by: Bug 1048752

Comment 3

[Jorg K (CEST = GMT+2)]

Looks like step 3 (shift right) to select "\n" gets things confused.

Comment 4

[Jorg K (CEST = GMT+2)]

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

Comment 5

[Jorg K (CEST = GMT+2)]

BTW: Searching for "caret" in Bugzilla currently gives more than 300 bugs, more than 50 have the word "position" as well.

Comment 6

[arni2033]

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

Comment 7

[Alice0775 White]

Please file a bug.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

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

Comment 10

[arni2033]

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.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Backing out bug 859088 to see what difference it makes.

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

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Mats, there is an implication that this is your regression. Can you handle it?

Comment 13

[Mats Palmgren (inactive)]

Attachment: 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

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Mats Palmgren (inactive)]

Attachment: fix

Comment 16

[Mats Palmgren (inactive)]

Attachment: tests

All the orange is unrelated afaict,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc07036e3f1

Comment 17

[Jorg K (CEST = GMT+2)]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730940 [details] [diff] [review]
fix

I have a bit long review queue right now. Maybe ehsan can help here.

Comment 19

[(no longer active)]

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

Comment 20

[(no longer active)]

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?

Comment 21

[Mats Palmgren (inactive)]

> Hmm, what changed on this line?

Just a typo: s/platrom/platform/

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f245074b4fb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b6173bf031

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f245074b4fb4
https://hg.mozilla.org/mozilla-central/rev/b3b6173bf031

Comment 24

[arni2033]

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?

Comment 25

[Mats Palmgren (inactive)]

Yes, please file a new bug with testcase + steps to reproduce.