Closed Bug 396542 Opened 13 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Selection, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: martijn.martijn, Assigned: masayuki)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file 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.
Attached patch PatchSplinter Review
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...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #752096 - Flags: review?(ehsan)
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?
Attachment #752096 - Flags: review?(ehsan) → review+
Do you have idea for testing if the caret is blinking?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(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.)
(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?
Flags: needinfo?(ehsan)
(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.  :/
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/76d9db6490f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This bug seems to be back: the testcase in comment 0 is broken again.
(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.
I think the bug need to be reopened.
The cursor still appears although the input text field is read only.
(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.
You need to log in before you can comment on or make changes to this bug.