396542 - After clicking on readonly text input, caret doesn't blink anymore in caret mode

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, to reproduce the bug:
- Turn on caret browsing (F7)
- Click on the readonly text input
- Click on the text in this page

Expected result:
Caret should blink

Actual result:
Caret does not blink (readonly)

This regressed between 2005-10-03 and 2005-10-04, a regression from bug 193316.
I guess some similar code is needed in nsTextEditorFocusListener::Blur as was added in nsTextEditorFocusListener::Focus.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

The actual cause is that nsEditor::FinalizeSelection() doesn't call nsISelectionController::SetCaretReadOnly(false). The instance of selection controller of <input> or <textarea> is different from the instance of presShell. However, some methods delegate the handling to presShell. Therefore, like this, change for the selection controller of <input> or <textarea> causes the caret behavior of caret browsing.

Similarly, current implementation doesn't reset the nsCaret::SetCaretDOMSelection().

By these reasons, I think that nsFocusManager should have a method which reset the caret and selection for caret browsing mode. And when an editor loses focus, the editor should call it.

Ideally, this patch should be reviewed by enn too, but he isn't available until June 10...

Comment 2

[(no longer active)]

Comment on attachment 752096 [details] [diff] [review]
Patch

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

Can you please add a test for this as well?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Do you have idea for testing if the caret is blinking?

Comment 4

[(no longer active)]

Yes, use http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html?force=1.  That test disables the blinking and renders the caret at all times, to avoid timing issues introduced because of the blinking.

Comment 5

[(no longer active)]

(Note that if your test doesn't require chrome privileges, you can just create a simple reftest.  Reftests have the ui.caretBlinkTime set to -1 as well.)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Yes, use
> http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/
> test_reftests_with_caret.html?force=1.  That test disables the blinking and
> renders the caret at all times, to avoid timing issues introduced because of
> the blinking.

The bug of this is what the caret is NOT blinking after read-only editor loses focus. I.e., we need to check the caret is actually *blinking* at that time. It seems that setting ui.caretBlinkTime to -1 stops blinking the caret. Therefore, I don't think it can test this bug, isn't it?

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > Yes, use
> > http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/
> > test_reftests_with_caret.html?force=1.  That test disables the blinking and
> > renders the caret at all times, to avoid timing issues introduced because of
> > the blinking.
> 
> The bug of this is what the caret is NOT blinking after read-only editor loses
> focus. I.e., we need to check the caret is actually *blinking* at that time. It
> seems that setting ui.caretBlinkTime to -1 stops blinking the caret. Therefore,
> I don't think it can test this bug, isn't it?

Oh, you're right.  Nevermind then, feel free to land this without a test.  :/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, landed without tests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/76d9db6490f4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76d9db6490f4

Comment 10

[Pierre de La Morinerie]

This bug seems to be back: the testcase in comment 0 is broken again.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Pierre de LA MORINERIE from comment #10)
> This bug seems to be back: the testcase in comment 0 is broken again.

Please file a new bug. It's really different bug.

Comment 12

[Bassel]

I think the bug need to be reopened.
The cursor still appears although the input text field is read only.

Comment 13

[Martijn Wargers (dead)]

(In reply to Bassel from comment #12)
> I think the bug need to be reopened.
> The cursor still appears although the input text field is read only.

What do you mean? In the text input, the caret should appear, but it should not blink.