Closed Bug 527306 Opened 10 years ago Closed 10 years ago

A simple text input.value change will no attribute right value if a div container display style is modified

Categories

(Core :: Layout: Form Controls, defect, P2, critical)

1.9.2 Branch
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: nirvn.asia, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2b2pre) Gecko/20091030 Namoroka/3.6b2pre
Build Identifier: 

The summary is probably all wrong but I can't really understand the mechanics behind what happens in the code and why it results in wrong text.value ... The bug revealed itself while using a web application I'm developing on firefox 3.6b1.

I narrowed down the problem and created two test cases.
test-fail.html (bug, div display style change)
test-work.html (no bug, no div display style change)

The problem appears both when jit.content is true or false.

Reproducible: Always

Steps to Reproduce:
1. Open attached 'test-fail.html'
2. Click on edit button next to the first 'Battambang' item
(you can change item text with the edit text box that appeared)
3. Click on edit button next to the second 'Mondolkiri' item
Actual Results:  
When clicking on edit button of a second item, the value in the edit text box is not updated to use the second item's text but keep value of first item.

Expected Results:  
The edit text box value is updated to the content of second item.

(I'm setting severity of bug to critical as the behavior can lead to lose of data if one is not aware of value wrongfully assigned during interface with page)
Attached file testcase showing bug
Version: unspecified → 1.9.2 Branch
Works: http://hg.mozilla.org/mozilla-central/rev/7b06b19e4af6
Fails: http://hg.mozilla.org/mozilla-central/rev/c6692a8f3f27
So if there has been no sneaky fix and re-regression after 3 Aug 2009, the regression window should be: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b06b19e4af6&tochange=c6692a8f3f27

I see no javascript engine in that range, so I'll change it to layout form controls, do you agree?
Assignee: general → nobody
Component: JavaScript Engine → Layout: Form Controls
Flags: blocking1.9.2?
QA Contact: general → layout.form-controls
Keywords: regression, testcase
Hardware: x86 → All
Ria, sounds like a logical thing to do. Could you change bug's status to confirmed?

From the pushlog list you came up with, bug 494546 seems to be a good culprit candidate to start with. Is it possible for someone to back off this change locally and see whether this bug's gone?
Doesn't seem to be caused by bug 494546 as effectively disabling the changes of bug 494546 by never doing the insert asynchronously in RecreateFramesForContent still shows this bug.
Seems to be caused by bug 506874.
Blocks: 506874
Assignee: nobody → roc
Flags: blocking1.9.2? → blocking1.9.2+
OK, three relevant-ish warnings when clicking that second button:

WARNING: NS_ENSURE_TRUE(mShell) failed: file /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp, line 1877
WARNING: Somehow not a plaintext editor?: file /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp, line 2661
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp, line 1906

That first rv is ignored.  But weakFrame.IsAlive() is false at:

2660            if (!plaintextEditor || !weakFrame.IsAlive()) {
2661              NS_WARNING("Somehow not a plaintext editor?");
2662              return NS_ERROR_FAILURE;
2663            }

so we return from nsTextControlFrame::SetValue without setting the value.  nsHTMLInputElement::SetValueInternal ignores the error return from SetFormProperty and returns NS_OK, so there's no exception from the script's point of view.
And the stack to the relevant Destroy() call:

#0  nsTextControlFrame::Destroy (this=0x203eff30) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:1109
....
#12 0x11be902d in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x16731730) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsCSSFrameConstructor.cpp:11601
#13 0x11c61781 in PresShell::FlushPendingNotifications (this=0x167312e0, aType=Flush_Style) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsPresShell.cpp:4879
#14 0x11d0f0af in nsTypedSelection::selectFrames (this=0x16743ba0, aPresContext=0x206c8200, aRange=0x1673f120, aFlags=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:4371
#15 0x11d11ba9 in nsTypedSelection::Clear (this=0x16743ba0, aPresContext=0x206c8200) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:3902
#16 0x11d1657a in nsTypedSelection::Collapse (this=0x16743ba0, aParentNode=0x16743a90, aOffset=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:5241
#17 0x11d17ad1 in nsFrameSelection::TakeFocus (this=0x16743ac0, aNewFocus=0x16743a90, aContentOffset=0, aContentEndOffset=1, aHint=nsFrameSelection::HINTLEFT, aContinueSelection=0, aMultipleSelection=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:1864
#18 0x11d180ef in nsFrameSelection::SelectAll (this=0x16743ac0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:2288
#19 0x11d579cb in nsTextInputSelectionImpl::SelectAll (this=0x16743810) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:930
#20 0x11d59fcc in nsTextControlFrame::SetValue (this=0x203eff30, aValue=@0xbfffc02c) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:2658

That flush in selectFrames was added in bug 506874.  

Note that nsHTMLInputElement::SetValueInternal has:

1105       // No need to flush here, if there's no frame at this point we
1106       // don't need to force creation of one just to tell it about this
1107       // new value.
1108       formControlFrame = GetFormControlFrame(PR_FALSE);

We could solve that by in fact performing a frames flush if there is a form control frame, but that's the common case.... Is there a way to avoid flushing in selectFrames?  Or avoiding selection operations in the value setting codepath?  I suppose we could just toss up a script blocker around the value set, but does selection _really_ need that frames flush?
As far as I recall, that flush is not need to fix bug 506874 specifically. I added it because I was writing tests like

<body style="white-space:pre">0123456789
<script>
var t = document.body.firstChild;
var sel = window.getSelection();
sel.collapse(t, 2);
sel.extend(t, 8);
</script>

and I observed that the tests were failing without the flush. The reason is of course that when that script runs, we don't have a frame for the <body> child, so selectFrames doesn't set selection state on any frame, and frame construction does not consult selection, so the text never looks selected. Flushing frames for selectFrames isn't a complete fix, since reframing still loses selection state, but it fixes this simple case. I would be surprised if no authors have encountered it in the wild. A complete fix would mean re-calling selectFrames after each frame construction episode, which suggests worrying performance implications to me, or better still moving selection state completely out of frames into content.

So we can back out the flush, but we'd have to modify all the testcases from bug 506874 to add explicit flush hacks, which is unappetizing.
Hmm.  Having selection on the frame tree really sucks here....

I suspect the unappetizing thing is the safe thing to do, though we could make window.getSelection flush... would that solve your test issues?
It would, although of course slight variations (e.g., caching getSelection() in a script in <head> and then using it in later scripts) would remain broken.

Another option would be to only flush in selectFrames when this is not the selection for a form control.
That can still lead to weird behavior, since a flush_frames can destroy presshells, no?
Yeah, but we handle that:

  // Re-get shell because the flush might have destroyed it 
  presShell = aPresContext->GetPresShell();
  if (!presShell)
    return NS_OK;
The only way to always behave non-weirdly is to move all selection state into content, I think.
Attached patch fix (obsolete) — Splinter Review
Don't flush in selectFrames when this is the selection for a form control.
Attachment #411265 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
> Yeah, but we handle that:

"handle" in the sense of "don't perform the selection", no?

Will designmode iframes that are trying to delete text while there's a pending reframe of the nsSubDocumentFrame around work right with that last patch?  Or are they ok already for some reason?
Oh, and I absolutely agree with comment 14; it's just that if I have to choose between new web-exposed weirdness and old web-exposed weirdness plus a bit of extra weirdness in our tests, I'd prefer the latter, I think.
Comment on attachment 411265 [details] [diff] [review]
fix

I suppose you're right. But I hate re-breaking this.
Attachment #411265 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Doesn't make me that happy either... :(
Attached patch safe fixSplinter Review
Attachment #411265 - Attachment is obsolete: true
Attachment #411354 - Flags: review?(bzbarsky)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [needs review]
Comment on attachment 411354 [details] [diff] [review]
safe fix

Looks fine, but why the changes to dynamic-text-1-*?
Attachment #411354 - Flags: review?(bzbarsky) → review+
To work around the problem that Quartz antialiasing causes the test to fail --- antialiased pixels aren't repainted. Bug 476927.
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/9c362934e378
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 192 landing]
Depends on: 554806
Depends on: 602331
You need to log in before you can comment on or make changes to this bug.