235294 - Text caret appears in wrong element after asynchronous script sets readOnly to false

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Charles Jardine]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

The test page at http://www.cus.cam.ac.uk/~cj10/bugs/gecko-ro.html
contains a form with two text boxes. A JavaScript script puts the
focus into the upper textbox, and uses setTimeout to schedule
a script to set the readOnly propoerty of the lower box to false.

When all this has been done, a text caret appears in the _lower_
box, when the focus is, in fact, still in the upper box. If the
user types stuff, it will apeear in the upper box.

The incorrect position of the caret cam be corrected by clicking
on the background, then on the window titlebar.


Reproducible: Always
Steps to Reproduce:
1.Make sure JavaScript is enabled.
2.Visit the specified URL.
3.

Actual Results:  
Text caret is in lower box.

Expected Results:  
Text caret should be in upper box.

The bug has also been seen using

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 
Firefox/0.8
and
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030821

however the effect is not seen with

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040212 Firebird/0.8.0+

Comment 1

[Boris Zbarsky [:bzbarsky]]

So what you're saying is that this is a bug in all sorts of old version but not
in a current one?  The bug is not present in a current trunk SeaMonkey build
either... (2004-02-18-08).

Sounds to me like this is just something that got fixed in the 1.7a cycle.

Comment 2

[Charles Jardine]

(In reply to comment #1)
> So what you're saying is that this is a bug in all sorts of old version but not
> in a current one?  The bug is not present in a current trunk SeaMonkey build
> either... (2004-02-18-08).
> 
> Sounds to me like this is just something that got fixed in the 1.7a cycle.

It seems you are right. I have just tested the official Windows build of 1.7a
(released yesterday). It does not display the bug.

I am sorry to have wasted your time. 

Comment 3

[Boris Zbarsky [:bzbarsky]]

All good.  ;)  Thanks for the response!

Comment 4

[Charles Jardine]

I am sorry, but I spoke too soon. Neither

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219

nor

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040212 Firebird/0.8.0+

actually fix the bug, though both make it harder to demonstrate.

My original test case assigned false to the readOnly property of a
text box when the property was already false. I think these newer
Gecko builds must optimize this case.

I have a new test page at http://www.cus.cam.ac.uk/~cj10/bugs/gecko-ro2.html
which sets readOnly to true and then later to false.  This page does
demonstrate the bug with the newer builds.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: The second testcase

OK, I see the problem with this one too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The problem seems to be the code at
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFrame.cpp#589
-- similar code is removed in nsPresShell::SetCaretEnabled with comments about
the fact that it makes the caret use the wrong selection...

Why does that chunk of code exist?

Comment 7

[Simon Fraser [no longer active]]

In nsTextInputSelectionImpl::SetCaretEnabled, if the param is PR_FALSE, it's
certainly wrong to reset the selection on the caret. If PR_TRUE, you probably
want to keep resetting the selection but a real fix would be change stuff around
so that there is a 1::1 relationship between carets and selections

Comment 8

[Boris Zbarsky [:bzbarsky]]

Simon, the param is true in this case.  The caller is at
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFrame.cpp#2665

Perhaps it should not reenable the caret, just like unsetting disabled does not?
 Or would that mean the caret would never appear in that control?

> so that there is a 1::1 relationship between carets and selections

Wouldn't that lead to multiple carets all being visible if multiple selections
are?  Eg a textarea on a page in caret browsing mode?

Comment 9

[Simon Fraser [no longer active]]

> Perhaps it should not reenable the caret, just like unsetting disabled does
> not? Or would that mean the caret would never appear in that control?

You'd have to test that; I don't know if something else will cause the caret to
redraw (probably not).

> Wouldn't that lead to multiple carets all being visible if multiple selections
> are?  Eg a textarea on a page in caret browsing mode?

Selections know when they are 'enabled' (they draw a different color), and only
one selection in a window should be enabled at any one time. Obviously, the
caret would only draw when the selection is both enabled and collapsed.

Comment 10

[Kathleen :Brade]

some areas to focus testing on include:
 * IME (including arrowing before committing)
 * clipboard commands: Make sure "copy" is enabled only when there is a selection
 * type ahead find
 * caret browsing
 * accessibility
 * autoCopy
 * xul text fields

Obviously, you'll check to see that the caret doesn't disappear completely in
certain cases or that it shows up multiple times (where it doesn't already).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> You'd have to test that;

Yeah, it's no good.  If I have a readonly control and I click at some point and
then remove the readonly attr, the caret should start flashing at that point if
it has focus.  If I remove that line, that doesn't happen till the window is
refocused. I guess you can't really focus disabled controls, so it's not an
issue there.

> Selections know when they are 'enabled'

Ah, true.

Also, not resetting the selection in nsTextInputSelectionImpl::SetCaretEnabled
doesn't work -- that breaks carets for text inputs in general.

Comment 12

[Boris Zbarsky [:bzbarsky]]

rbs, did you fix this with your selection exclusion changes or something?  It
seems to workforme with the testcase I posted....

Comment 13

[rbs]

Yes, indeed. The old code would have fed the unintended selection in the caret
on your testcase.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So fixed by patch in bug 58305

Comment 15

[Charles Jardine]

Attachment: Third test case

This test case shows that

  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a) Gecko/20040503

does not really fix the bug, though the second test case no longer
demonstrates it.

In this test case the two boxes are both initially readonly.
A script puts the focus into the upper box and then makes
the lower box writable. A text caret appears in the upper box,
despite its still being readonly.

Comment 16

[Charles Jardine]

Attachment: 4th test case

Anothe test for Gecko/20040503.

In this test case both boxes are initially readonly and a
script gives the upper box the focus.
If you click on the "Press Me" button, the lower box is made writable.
Again, a text caret appears in the upper box, despite the fact that
it is readonly, and no longer has the focus; the button has the focus.

Comment 17

[Charles Jardine]

In response to the above, I have tested my real application against the
latest nightly build:

  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a) Gecko/20040503

In the real application, the behaviour of the text caret has changed,
but it is still not correct. It is true that the attachement "The second
test case" no longer demonstrates the bug, but other similar cases do.
See the attached Third and 4th test cases.

Setting readOnly=false can still produce spurious carets, but they
are now in different places.

Comment 18

[rbs]

Attachment: Another testcase: readonly input with onfocus="elsewhere.focus()"

Comment 19

[rbs]

I have a complete fix for these issues. BTW, I curled the above testcase from
this informative page:
http://www.cs.tut.fi/~jkorpela/forms/readonly.html

Comment 20

[rbs]

Attachment: patch to fix the inconsistencies

PART: @@ -2080,7 +2081,16 @@ void    nsTextControlFrame::SetFocus(PRB

The purpose is as ducumented there Without this, the caret in attachment 147687 [details]
is located in the readonly input, while the typed text appears in the real
focused input.

PART: @@ -2667,14 +2677,10 @@ nsTextControlFrame::AttributeChanged(nsI

That old code was evidently wrong. There is only one caret, and there is no
reason for any element to disable/enable the caret when its readonly attribute
is changed. Leave that business to the active/focused element. Note: this part
of the patch alone fixed the 3rd and 4th testcases.

Comment 21

[rbs]

Comment on attachment 147687 [details]
Another testcase: readonly input with onfocus="elsewhere.focus()"

bz, care to take a look? you dragged me into this...

Comment 22

[rbs]

Comment on attachment 147689 [details] [diff] [review]
patch to fix the inconsistencies

...correct attachment for the patch

Comment 23

[Boris Zbarsky [:bzbarsky]]

rbs, what happens if I focus a readonly text control (which I can do) and then
it's set to not be readonly anymore in script?  See comment 11.  That's the
problem I ran into when removing those calls....

Comment 24

[rbs]

Attachment: testcase - to focus then set readonly=false

As is, the patch doesn't solve that problem, as illustrated in this testcase. I
will iterate an idea that I have about that.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Note that clicking to set readonly=false should NOT cause the caret to appear,
since the focus is then not in the control.  The problems start if the control
is focused and then something else causes it to become non-readonly without
taking focus....

Comment 26

[rbs]

Attachment: better testcase for focus() and readonly=false

Comment 27

[rbs]

Attachment: patch - take 2

Comment 28

[rbs]

Comment on attachment 147704 [details] [diff] [review]
patch - take 2

Rationale of the patch:
- either the node |has| focus, |will have| focus, or it |won't have| focus.
- if the node |has focus|, then disable/enable straightway.
- if it |will have| focus, then SetFocus() will take care.
- if it |won't have| focus, then don't mess with what belongs to somebody else.
Leave the caret alone.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147704 [details] [diff] [review]
patch - take 2

r+sr=bzbarsky.

Comment 30

[rbs]

Fix checked in on the 1.8 trunk.

Reporter, if this bug is important for that (secret) application of yours, feel
free to ask for approval for 1.7f (using Edit -> approval ?), explaining in the
comment field to drivers why it is important to you. The new behavior is more
logical and superior to IE. IE doesn't pass the testcase in attachment 147703 [details]
for focus() and readonly=false. Its caret doesn't re-appear.

Comment 31

[[e-mail]]

(In reply to comment #30)
>IE doesn't pass the testcase in attachment 147703 [details] for focus() and
>readonly=false. Its caret doesn't re-appear.
Bah, if only we followed IE3's example and did't hide the caret in readonly
fields we wouldn't have this issue. So since when did IE start hiding the caret
in readonly fields, and why did we copy it?

Comment 32

[rbs]

> IE3
You still know what that did? Anyway, I don't see much benefit with that old
behavior since showing the caret misleads users into thinking that they can
type/enter some data. That seems more confusing than useful.

> we wouldn't have this issue
Not anymore with the fix.

Comment 33

[[e-mail]]

(In reply to comment #32)
>>IE3
>You still know what that did?
I've still got both IE2 and IE3 installed. IE2 was great for reading Perl 4
documentation, but I had to upgrade to IE3 to read Perl 5...
>Anyway, I don't see much benefit with that old behavior since showing the
>caret misleads users into thinking that they can type/enter some data. That
>seems more confusing than useful.
Personally I find that having no indication of focus is more confusing than useful.

Comment 34

[rbs]

:focus { background-color: gray; } or something, e.g., a distinctive readonly
type of caret might be okay -- not the current ones
(eMetric_SingleLineCaretWidth and eMetric_MultiLineCaretWidth) that are
invariably associated to typing. But that's another story/bug.

Comment 35

[Charles Jardine]

(In reply to comment #30)
> Fix checked in on the 1.8 trunk.

I have tested last night's build (2004050609). The text caret now
behaves correctly on my application. Thank you.

While testing, I noticed an (I hope independant) problem with this build.
It is not possible byt tabbing to move the fixcus from the location
toolbar widget to a form element. This maked use of forms without a mouse
impossible. I would value advice as to whether an outside like me should
report a bug like this in a nightly build.

> Reporter, if this bug is important for that (secret) application of yours, feel
> free to ask for approval for 1.7f (using Edit -> approval ?), explaining in the
> comment field to drivers why it is important to you. The new behavior is more
> logical and superior to IE. IE doesn't pass the testcase in attachment 147703 [details]
> for focus() and readonly=false. Its caret doesn't re-appear.

It ouls be very useful if the patch got into 1.7. I don't seem to have
authority to edit the patch, so here is my pleading.

I am developing an application which provides
infrastructure to allow others to develop database
applications. These applications use Javascript
ot maintain a view of a portion of the database in
a form in one frame of a frameset while communicating
with a database server (via an Apache middle tier)
by posting an invisible form whose target is another
frame in the frameset.

The Javascript dynamically modifies the 'value' and
'readOnly' properties of the text elements in the
display form. The effect of this bug is that the
text caret is very rarely displayed in the correct
element.

I am therefore very grateful for the fix. Thank you all.

I am expecting to release the first of these applications
for use by real end users within the next few weeks.
I would like, from the start, to recommend Mozilla
as the preferred browser for these applications.
However, if this patch does not make it into 1.7,
I will have, reluctantly, initially to recommend IE.
This would be sad.

Comment 36

[[e-mail]]

(In reply to comment #35)
>While testing, I noticed an (I hope independant) problem with this build.
>It is not possible byt tabbing to move the fixcus from the location
>toolbar widget to a form element. This maked use of forms without a mouse
>impossible. I would value advice as to whether an outside like me should
>report a bug like this in a nightly build.
Don't worry, this has already been reported and fixed, see bug 242790.

Comment 37

[rbs]

Comment on attachment 147704 [details] [diff] [review]
patch - take 2

Asking a= for 1.7f based on the plea on the reporter (drivers, please see
comment 35) and his satisfactory testing.

Comment 38

[chris hofmann]

Comment on attachment 147704 [details] [diff] [review]
patch - take 2

ok, lets get this in for 1.7 RC2   

bob clary also suggests it would be good to get contacts at Siebel, Oracle,
SAP, IBM, Sun and Macromedia actively testing their applications with 1.7 RC2
with this change in focus/caret processing explicitly mentioned as a new change
which needs their attention.

Comment 39

[rbs]

Checked in the 1.7f branch.