602130 - Caret positioned incorrectly in input[type=text] when hidden and redisplayed

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Christian Sonne [:cers]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101005 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101005 Firefox/4.0b7pre

If an input[type=text] is hidden via display="none" and redisplayed via display="block" or display="inline-block" (and possibly other values), the caret is displayed too low in the input. This only occurs if the input has been focused before the first time it is hidden, and the bug disappears as soon as text is entered.

It is not triggered if the input is redisplayed via display="" or display="inline". If the input is wrapped in a div, and the div is hidden, display="block" no longer triggers the bug, yet display="inline-block" still does.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.geeksbynature.dk/bugs/cursorPosition
2. focus topmost text input
3. click trigger next to it
Actual Results:  
caret positioned about half a line down, showing only top part

Expected Results:  
caret positioned as before input was hidden

Comment 1

[Christian Sonne [:cers]]

Attachment: screenshot of bug

Screenshot of the bug as it appears on OSX

Comment 2

[Boris Zbarsky [:bzbarsky]]

Seems likely the fix for bug 536421 will fix this.  Need to retest once that lands...

Comment 3

[Mats Palmgren (inactive)]

Bug 536421 will not fix this.  This bug only seems to occur when the
text input is empty.  Maybe Ehsan's work on caret placement in bug 389321
will help?

Comment 4

[(no longer active)]

(In reply to comment #3)
> Bug 536421 will not fix this.

Why?  It seems to me that this is actually a dupe of that bug, isn't it?

Comment 5

[(no longer active)]

Also, bug 389321 is about contenteditable elements, and it shouldn't affect caret positioning in input elements at all.

Comment 6

[Christian Sonne [:cers]]

(In reply to comment #4)
> (In reply to comment #3)
> > Bug 536421 will not fix this.
> 
> Why?  It seems to me that this is actually a dupe of that bug, isn't it?

In bug 536421 the caret is not displayed at all, here it is displayed in a weird position (and this bug only affects empty elements). While it is a reasonable guess that the fix for that bug might also solve this problem, I do not think it is a dupe.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Bug 536421 will not fix this.
> > 
> > Why?  It seems to me that this is actually a dupe of that bug, isn't it?
> 
> In bug 536421 the caret is not displayed at all, here it is displayed in a
> weird position (and this bug only affects empty elements). While it is a
> reasonable guess that the fix for that bug might also solve this problem, I do
> not think it is a dupe.

The cause of the problem (reframing of the text control) is the same, however...

Comment 8

[Christian Sonne [:cers]]

(In reply to comment #7)
> 
> The cause of the problem (reframing of the text control) is the same,
> however...

On that note, I've tested the patch from bug 536421, but it does not appear to
affect this bug. I do agree that the over all cause for the two are the same
though.

Additionally, I've tried replicating what happens in Panorama (bug 5948044),
but it doesn't seem to trigger with those circumstances in HTML. iQ hides and
shows by setting "opacity:0;display:none" and "opacity:1;display:''"
respectively, and Panorama applies this to a wrapper div. It seems that the bug
behaves slightly differently in chrome context, though it might just be other
contributing factors in the Panorama code.

I've added a test for the iQ way of hiding/showing to the HTML testcase.

Comment 11

[Mounir Lamouri (:mounir)]

Given that bug 553097 depends on that bug and is a blocker, it would be nice to
have this bug being explicitly a blocker.

Comment 12

[(no longer active)]

I set a breakpoint on nsCaret::DisplayAtPositionWithHint, and verified that the caret rectangle is correctly calculated (a 1x14 rectangle).  I think this should be debugged by someone familiar with display list stuff.  roc: do we have anybody to take a look at this?

Comment 13

[Timothy Nikkel (:tnikkel)]

I can take a look.

Comment 14

[(no longer active)]

(In reply to comment #13)
> I can take a look.

Thanks!

Comment 15

[Timothy Nikkel (:tnikkel)]

So the caret is ok. The textframe inside the anonymous div is shifted down, and the caret is drawn at the correct position inside the textframe and is just clipped. So the textframe being at the wrong place is the problem.

Comment 16

[Timothy Nikkel (:tnikkel)]

Hmm, so '<input>' and '<input style="display: block;">' both create an anonymous div that contains only a br node, but setting display block when onfocus the anonymous div only contains a text node.

Comment 17

[Timothy Nikkel (:tnikkel)]

Setting to display: block causes us to reconstruct the frame. We follow this path
nsCSSFrameConstructor::ProcessChildren
nsCSSFrameConstructor::GetAnonymousContent
nsTextControlFrame::CreateAnonymousContent
nsHTMLInputElement::BindToFrame
nsTextEditorState::BindToFrame
nsTextEditorState::GetRootNode
nsTextEditorState::CreateRootNode
nsTextControlFrame::UpdateValueDisplay
and in UpdateValueDisplay we create the text node under the anonymous div. Then the next time it is safe to run script the PrepareEditorEvent runs and when it tries to nsTextEditRules::CreateBogusNodeIfNeeded it does not create the br node because it finds the textnode already there.

So I'm guessing we still want to create a br node in this case? And UpdateValueDisplay should handle this? I'm not sure here.

Comment 18

[(no longer active)]

Attachment: Patch (v1)

Good catch, Timothy.

The problem here is that UpdateValueDisplay can be potentially called before editor init with aBeforeEditorInit equal to false (or not called at all, which is even worse).

This patch should fix this issue.

Comment 19

[(no longer active)]

I also verified that this patch fixes the duplicates, as well as bug 585967 and bug 594804.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 482868 [details] [diff] [review]
Patch (v1)

r=me

Comment 21

[(no longer active)]

Attachment: For check-in

I added the display:block style to the reference file in order to make sure that the layout is consistent on all platforms.

Comment 22

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a1ee6ca61426

Comment 23

[(no longer active)]

I backed this out because of test failures:

http://hg.mozilla.org/mozilla-central/rev/8567b73400e8

Sample failure log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287087223.1287088014.28248.gz

Comment 24

[(no longer active)]

Attachment: Part 2

The test failure happened because under some situations, the transaction manager was not correctly cleared when we re-initialized the editor for a text control.  This patch fixes this problem as well.

Comment 25

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/26f3aecd3d79
http://hg.mozilla.org/mozilla-central/rev/2ad78af5ae6b