Closed Bug 602130 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: cers, Assigned: ehsan)

References

()

Details

Attachments

(3 files, 1 obsolete file)

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
Attached image screenshot of bug
Screenshot of the bug as it appears on OSX
Status: UNCONFIRMED → NEW
Ever confirmed: true
Seems likely the fix for bug 536421 will fix this.  Need to retest once that lands...
Depends on: 536421
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?
Depends on: 389321
No longer depends on: 536421
(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?
Also, bug 389321 is about contenteditable elements, and it shouldn't affect caret positioning in input elements at all.
Depends on: 536421
No longer depends on: 389321
(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.
(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...
(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.
Duplicate of this bug: 598842
Duplicate of this bug: 601159
Blocks: 553097
Given that bug 553097 depends on that bug and is a blocker, it would be nice to
have this bug being explicitly a blocker.
blocking2.0: --- → ?
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?
Blocks: 553097
blocking2.0: --- → ?
I can take a look.
(In reply to comment #13)
> I can take a look.

Thanks!
Blocks: 534785
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.
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.
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.
Depends on: 585967
Blocks: 585967
No longer depends on: 585967
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #482868 - Flags: review?(bzbarsky)
I also verified that this patch fixes the duplicates, as well as bug 585967 and bug 594804.
Comment on attachment 482868 [details] [diff] [review]
Patch (v1)

r=me
Attachment #482868 - Flags: review?(bzbarsky) → review+
Attachment #482868 - Flags: approval2.0?
Whiteboard: [needs landing]
Whiteboard: [needs landing]
Whiteboard: [needs landing]
Attached patch For check-inSplinter Review
I added the display:block style to the reference file in order to make sure that the layout is consistent on all platforms.
Attachment #482868 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/a1ee6ca61426
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Version: unspecified → Trunk
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Part 2Splinter Review
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.
Attachment #483339 - Flags: review?(roc)
Status: REOPENED → ASSIGNED
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/26f3aecd3d79
http://hg.mozilla.org/mozilla-central/rev/2ad78af5ae6b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Duplicate of this bug: 574492
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.