611189 - Parchment IF Reader Misbehaves

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Sean Stangl [:sstangl]]

Apologies for the long URL -- every URL shortener rejects it.

Visit the URL linked to load the Parchment Interactive Fiction Reader. Once at the prompt, type "L", and press enter. On the latest nightly, it will display some text, and issue a new prompt with the "L" still typed.

On 3.6.12, the new prompt will not have the "L".

Comment 1

[David Mandelin [:dmandelin]]

There could be something important underneath this, so I'm going to make it block final for now. But if it's just an extra L, we should be ready to unblock it at the end if we need to.

Comment 2

[Torbjörn Andersson]

For a shorter URL, most games on http://parchment.toolness.com/ should work as test cases. (The Parchment source code is hosted on http://code.google.com/p/parchment/ if that's of any help.) Since the problem appears to be that the input buffer isn't cleared between commands, anything you type should do, i.e. it's not just an extra L.

Compiling Firefox to find out when the problem appeared is a bit beyond me, but I tried running a few different releases. The problem appears to have been introduced somewhere between 3.6 and 4.0. More specifically, I could reproduce it with 3.7a5 but not with 3.7a4 (or 3.7a4webm).

Unfortunately, that still leaves a pretty large window for the bug to crawl through.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Torbjörn, if you're willing to do a binary search on nightlies, http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ has builds for every day in the range in question...  You probably want the -tracemonkey builds, if we think this is a JS issue.

Comment 4

[Torbjörn Andersson]

Thanks for the tip. I wouldn't have known which build to use, and when I looked in a directory at random I picked one that had only Windows builds so I didn't look any further.

I can reproduce the bug with this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/06/2010-06-07-03-tracemonkey/firefox-3.7a5pre.en-US.linux-i686.tar.bz2

I can NOT reproduce the bug with this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/06/2010-06-06-03-tracemonkey/firefox-3.7a5pre.en-US.linux-i686.tar.bz2

Comment 5

[Boris Zbarsky [:bzbarsky]]

Excellent.  What are the http URIs in about:buildconfig for those two builds?

Comment 6

[Torbjörn Andersson]

Another trick I didn't know about. Neat! :-)

The one with the bug: http://hg.mozilla.org/tracemonkey/rev/044852a34c7b
The one without the bug: http://hg.mozilla.org/tracemonkey/rev/4bed87d68dab

I'm about to leave for work, but I'll check back again this evening in case there's any other information I can provide.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checkin range: http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=4bed87d68dab&tochange=044852a34c7b

Unfortunately, there's a merge from mozilla-central in the range.

On mozilla-central, the regression checkin range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b219912edfec&tochange=5b3604a3cfbe (2-day range; no June 6 nightly here).  This, of course, includes the TM merge back...

Moving text control editors to content happened in the intersection of the ranges.  Ehsan, I'm betting this is an editor issue.

Comment 8

[(no longer active)]

The input control's .value from DOM is "L" when I type an L in the original input control.  What makes you think that this is an editor issue, Boris?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Mostly the fact that the JS changes in the range don't look relevant, nor do the painting changes, which leaves just the editor changes.

I'll bisect, I guess.

Comment 10

[Boris Zbarsky [:bzbarsky]]

hg bisect on tracemonkey says:

Due to skipped revisions, the first bad revision could be any of:
changeset:   43235:cdbd85281fe2
user:        Ehsan Akhgari <[e-mail]>
date:        Wed Apr 21 16:17:41 2010 -0400
summary:     Bug 534785 - Move the ownership of editor and selection controller from the text frame to the content node; r=roc,jst sr=roc

changeset:   43236:d5a112a3ed92
user:        Ehsan Akhgari <[e-mail]>
date:        Fri Jun 04 13:28:19 2010 -0400
summary:     Bug 567020 - Intermittent timeout in 343889-1.html, leading to the crashtest suite being aborted; r=jst

changeset:   43237:7b7dedca99d4
user:        Ehsan Akhgari <[e-mail]>
date:        Fri Jun 04 13:28:19 2010 -0400
summary:     Bug 563864 - Add some debugging output to figure out what's going on with test_mozPaintCount.html intermittent oranges (temporary test-only change)

changeset:   43238:024121edc745
user:        Ehsan Akhgari <[e-mail]>
date:        Fri Jun 04 13:28:19 2010 -0400
summary:     Bug 569238 - Add some debugging information to determine what causes test_flush_on_paint.html to time out (temporary test-only change)

changeset:   43239:6c41cd3fb116
user:        Ehsan Akhgari <[e-mail]>
date:        Sun May 23 22:13:00 2010 -0400
summary:     Bug 567701 - Don't store a view manager pointer in nsEditor; retrieve it lazily; r=roc

changeset:   43240:0082ced835df
user:        Ehsan Akhgari <[e-mail]>
date:        Sat Jun 05 16:17:42 2010 -0400
summary:     Bug 534785 - fix build bustage

(the missing '}' in that first changeset that wasn't fixed until the last changeset is what makes all the changesets be in range, but that first chagneset is almost certainly the real culprit).

Comment 11

[(no longer active)]

Torbjörn, do you know by any chance what the JavaScript code on that page is doing?  Having a minimized testcase would really help me in fixing this...

Comment 12

[Torbjörn Andersson]

I'm afraid I have only the foggiest ideas about how Parchment (or JavaScript, for that matter) works, and even that may be over-stating my knowledge. I know it implements the Infocom "Z-machine", which was originally used to port a slimmed-down version of "Zork" from the PDP-10 to the home computers of the 1980s, but that's not very helpful.

What I can do is provide a very simple Z-machine program that still shows the bug:

http://parchment.googlecode.com/svn/trunk/parchment.html?story=http://www.update.uu.se/~d91tan/hello.z5.js

You still have the whole Z-machine to deal with, but at least it's not trying to do anything particularly intelligent.

Comment 13

[Torbjörn Andersson]

I don't know if it makes any difference, but after poring over the Z-machine specification (rather than using an assembler that I don't quite understand), I made a shorter Z-machine program that demonstrates the bug:

http://parchment.googlecode.com/svn/trunk/parchment.html?story=http://www.update.uu.se/~d91tan/hello2.z5.js

(It's basically the same as the last one, but with less decorative text and less space for unnecessary buffers.)

Apart from a mandatory header and a global buffer variable the program just does this:

loop:
        print_char '>'   ; Print the prompt
        read buffer 0    ; Read text into buffer. The 0 means don't tokenize
        jump loop        ; Repeat

I hope I got that right.

Comment 14

[Torbjörn Andersson]

Attachment: The newer/smaller of the encoded Z-Machine files

Comment 15

[(no longer active)]

Attachment: Test case

I got this test case by inspecting what happens when we have the Parchment page open in the browser inside the debugger.  The input control should be empty, but it contains an "L".

Comment 16

[(no longer active)]

Attachment: Patch (v1)

The problem was that nsTextEditorState::GetValue would only check mBoundFrame && mEditor to determine whether it can read the value from the editor.  That condition is true in PrepareEditor before the SetValue call, which means that we can read the wrong value from the editor, and that value would later be passed to SetValue, which causes us to get stuck with it.  The fix was simple: add mEditorInitialized to the check.

The rest of the changes in this patch are made to make sure that we can handle this behavior change correctly in SetValue.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

r=me, but can't the test just be a reftest?

Comment 18

[(no longer active)]

(In reply to comment #17)
> Comment on attachment 490498 [details] [diff] [review]
> Patch (v1)
> 
> r=me, but can't the test just be a reftest?

I'd rather compare both the rendering and the DOM |value| property.  I guess I could dump out the value property somewhere in the document, but that would be a very round-about way for testing this, wouldn't it?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, I missed the check for .value in the test.  OK, then it's fine as-is.

Comment 20

[(no longer active)]

This causes a failure in the accessibility suite:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1289793383.1289798625.23833.gz

I locally tested it, and the last test executed here is <http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/events/test_text.html?force=1#301>.  As far as I can tell, no js exception is being thrown.  This is caused by these hunks:

>@@ -1714,17 +1714,17 @@ nsTextEditorState::SetValue(const nsAStr
>     }
> 
>     nsAutoString currentValue;
>     mBoundFrame->GetText(currentValue);
> 
>     nsWeakFrame weakFrame(mBoundFrame);
> 
>     // this is necessary to avoid infinite recursion
>-    if (!currentValue.Equals(aValue))
>+    if (!mEditorInitialized || !currentValue.Equals(aValue))
>     {
>       nsTextControlFrame::ValueSetter valueSetter(mBoundFrame,
>                                                   mBoundFrame->mFocusedValue.Equals(currentValue));
> 
>       // \r is an illegal character in the dom, but people use them,
>       // so convert windows and mac platform linebreaks to \n:
>       // Unfortunately aValue is declared const, so we have to copy
>       // in order to do this substitution.
>@@ -1754,17 +1754,19 @@ nsTextEditorState::SetValue(const nsAStr
>         if (domSel)
>         {
>           selPriv = do_QueryInterface(domSel);
>           if (selPriv)
>             selPriv->StartBatchChanges();
>         }
> 
>         nsCOMPtr<nsISelectionController> kungFuDeathGrip = mSelCon.get();
>-        PRUint32 currentLength = currentValue.Length();
>+        // While initializing, pretend that the current value is empty, so that
>+        // the full value gets set on the editor.
>+        PRUint32 currentLength = mEditorInitialized ? currentValue.Length() : 0;
>         PRUint32 newlength = newValue.Length();
>         if (!currentLength ||
>             !StringBeginsWith(newValue, currentValue)) {
>           // Replace the whole text.
>           currentLength = 0;
>           mSelCon->SelectAll();
>         } else {
>           // Collapse selection to the end so that we can append data.

Setting currentLength to 0 causes insertValue to be set to newValue, which causes nsIPlaintextEditor::InsertText to be called.  Without this patch, that function doesn't get called.  I think what's going on here is that something inside the accessibility code can't handle this change correctly, but this is basically the behavior that we want, and we need to fix the accessibility side of things.  How to do that is beyond me, though.  Alex, David, can you guys help please?

Comment 21

[alexander :surkov (:asurkov)]

It sounds like a11y tests starts working when editor is uninitialized. We have a input with 'value' string used as a value, then we set selection to 1 to 3 and remove it. We expect to be notified from CharacterDataWillChange/CharacterDataChanged that the characters are removed but we don't. Does it sound correct? What do we get (should) from CharacterDataWillChange/CharacterDataChanged?

Comment 22

[(no longer active)]

(In reply to comment #21)
> It sounds like a11y tests starts working when editor is uninitialized.

I don't believe this is the case.  The test does call focus() on the input element, which makes sure that the editor is initialized (and I've verified that under the debugger).  However, with this patch, an InsertText call gets made during the initialization, which is, I think, what confuses the a11y module.

> We have
> a input with 'value' string used as a value, then we set selection to 1 to 3
> and remove it. We expect to be notified from
> CharacterDataWillChange/CharacterDataChanged that the characters are removed
> but we don't. Does it sound correct? What do we get (should) from
> CharacterDataWillChange/CharacterDataChanged?

I assume that you're talking about those methods of nsDocAccessible, right?  If yes, then I guess we can set a breakpoint there and see what's happening.  Does that make sense?

Comment 23

[(no longer active)]

So, I get two assertions from the accessibility code when nsEditor::InsertTextImpl is executed: http://pastebin.mozilla.org/856036

I also discovered that the second synthesizeKey call in the test (to enter "a" into the text editor is never made, so _something_ should be throwing, causing the test to time out...

Comment 24

[(no longer active)]

OK, I kept banging my head on my desk while trying to debug this, but I didn't have much success.  I guess I'll give up on it for tonight, I'll try to sit down with David to debug this tomorrow.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> (In reply to comment #21)
> > It sounds like a11y tests starts working when editor is uninitialized.
> 
> I don't believe this is the case.  The test does call focus() on the input
> element, which makes sure that the editor is initialized (and I've verified
> that under the debugger).  However, with this patch, an InsertText call gets
> made during the initialization, which is, I think, what confuses the a11y
> module.

perhpas your assumption in bug 613058 that frames aren't ready when we're about to fire a11y text change events is correct.

> I assume that you're talking about those methods of nsDocAccessible, right?  If
> yes, then I guess we can set a breakpoint there and see what's happening.  Does
> that make sense?

correct

Comment 26

[(no longer active)]

(In reply to comment #25)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > It sounds like a11y tests starts working when editor is uninitialized.
> > 
> > I don't believe this is the case.  The test does call focus() on the input
> > element, which makes sure that the editor is initialized (and I've verified
> > that under the debugger).  However, with this patch, an InsertText call gets
> > made during the initialization, which is, I think, what confuses the a11y
> > module.
> 
> perhpas your assumption in bug 613058 that frames aren't ready when we're about
> to fire a11y text change events is correct.

Maybe, but that doesn't have anything to do with the failure here.  nsDocAccessible::CharacterDataWillChange/Changed are not called as a result of nsPlaintextEditor::InsertText.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> Maybe, but that doesn't have anything to do with the failure here. 
> nsDocAccessible::CharacterDataWillChange/Changed are not called as a result of
> nsPlaintextEditor::InsertText.

Ok, perhaps the problem is we get inserted text event while we expect removed text event?

You could check test results visually if you comment all gQueue.push stuffs except interesting one (to avoid noise) and uncomment gA11yEventDumpID variable. Every a11y event is fired will be logged into div on that test page. You could see what's expected and what we get.

Comment 28

[(no longer active)]

(In reply to comment #27)
> (In reply to comment #26)
> 
> > Maybe, but that doesn't have anything to do with the failure here. 
> > nsDocAccessible::CharacterDataWillChange/Changed are not called as a result of
> > nsPlaintextEditor::InsertText.
> 
> Ok, perhaps the problem is we get inserted text event while we expect removed
> text event?
> 
> You could check test results visually if you comment all gQueue.push stuffs
> except interesting one (to avoid noise) and uncomment gA11yEventDumpID
> variable. Every a11y event is fired will be logged into div on that test page.
> You could see what's expected and what we get.

I've already done that.  What happens is that everything proceeds fine until the VK_DELETE key is pressed to remove "al" from the input box's initial value ("value"), and then things stop.  All of the accessibility events up to that point fire just fine.  The test just seems to stop its execution mysteriously for some reason.

Did you get a chance to debug it?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > 
> > > Maybe, but that doesn't have anything to do with the failure here. 
> > > nsDocAccessible::CharacterDataWillChange/Changed are not called as a result of
> > > nsPlaintextEditor::InsertText.
> > 
> > Ok, perhaps the problem is we get inserted text event while we expect removed
> > text event?
> > 
> > You could check test results visually if you comment all gQueue.push stuffs
> > except interesting one (to avoid noise) and uncomment gA11yEventDumpID
> > variable. Every a11y event is fired will be logged into div on that test page.
> > You could see what's expected and what we get.
> 
> I've already done that.  What happens is that everything proceeds fine until
> the VK_DELETE key is pressed to remove "al" from the input box's initial value
> ("value"), and then things stop.  All of the accessibility events up to that
> point fire just fine.  The test just seems to stop its execution mysteriously
> for some reason.

It means we wait for event I think. Did you try to uncomment gA11yEventDumpID, it doesn't clear things?

> Did you get a chance to debug it?

I didn't try, if you need then I can do that tomorrow I think.

Comment 30

[(no longer active)]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > 
> > > > Maybe, but that doesn't have anything to do with the failure here. 
> > > > nsDocAccessible::CharacterDataWillChange/Changed are not called as a result of
> > > > nsPlaintextEditor::InsertText.
> > > 
> > > Ok, perhaps the problem is we get inserted text event while we expect removed
> > > text event?
> > > 
> > > You could check test results visually if you comment all gQueue.push stuffs
> > > except interesting one (to avoid noise) and uncomment gA11yEventDumpID
> > > variable. Every a11y event is fired will be logged into div on that test page.
> > > You could see what's expected and what we get.
> > 
> > I've already done that.  What happens is that everything proceeds fine until
> > the VK_DELETE key is pressed to remove "al" from the input box's initial value
> > ("value"), and then things stop.  All of the accessibility events up to that
> > point fire just fine.  The test just seems to stop its execution mysteriously
> > for some reason.
> 
> It means we wait for event I think. Did you try to uncomment gA11yEventDumpID,
> it doesn't clear things?

Yes, like I said, the events seem to be coming in just fine (well, I don't exactly know what to expect here), but things just stop, and I haven't been able to figure out why.

> > Did you get a chance to debug it?
> 
> I didn't try, if you need then I can do that tomorrow I think.

I would appreciate it, unless David and I can resolve this today.  David?

Comment 31

[alexander :surkov (:asurkov)]

The scenario is we get text inserted event for value specified by @value attribute, then get value change event for that, then we fire text removed event for removed text and fire value change event which is coalesced with the first event. The test fails because it doesn't receive value change after text was removed. The problem is text inserted and first value change events are unwanted I think because we don't want to fire these events for input presented in hierarchy when it gets initialized.

Comment 32

[(no longer active)]

(In reply to comment #31)
> The scenario is we get text inserted event for value specified by @value
> attribute, then get value change event for that, then we fire text removed
> event for removed text and fire value change event which is coalesced with the
> first event. The test fails because it doesn't receive value change after text
> was removed. The problem is text inserted and first value change events are
> unwanted I think because we don't want to fire these events for input presented
> in hierarchy when it gets initialized.

So, what's the solution here?

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (In reply to comment #27)
> > > > (In reply to comment #26)

> I would appreciate it, unless David and I can resolve this today.  David?

Sorry this week was primarily a work week with Marco (focus on html5).

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #32)
> (In reply to comment #31)
> > The scenario is we get text inserted event for value specified by @value
> > attribute, then get value change event for that, then we fire text removed
> > event for removed text and fire value change event which is coalesced with the
> > first event. The test fails because it doesn't receive value change after text
> > was removed. The problem is text inserted and first value change events are
> > unwanted I think because we don't want to fire these events for input presented
> > in hierarchy when it gets initialized.
> 
> So, what's the solution here?

I guess we could check the input and see if it is initialized?

Comment 35

[(no longer active)]

Attachment: Part 2: fix the a11y test

So, here's the stack where we fire the textchange event Alex mentioned in comment 31: http://pastebin.mozilla.org/859234.

We don't really want to not run nsCSSFrameConstructor::ContectAppended, nor do we want to special case the nsAccessibilityService::ContentRangeInserted call, because we don't have any idea that the text node is being appended because of an editor operation.  I think we should just initialize the text control in the accessibility test before the event checkers are run.  If accessibility doesn't want to get text change events for input control initializations, it should probably handle a lot more than just this particular case anyway, so I think this change is safe.

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 491928 [details] [diff] [review]
Part 2: fix the a11y test

please add a ccomment about why you do this. 
r=me since this is holding you up. Thanks for pinging us. Alexander or I will investigate further.

Comment 37

[(no longer active)]

Attachment: Part 2: fix the a11y test

With comment added.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 491999 [details] [diff] [review]
Part 2: fix the a11y test

wrong patch? ;)

Comment 39

[alexander :surkov (:asurkov)]

Comment on attachment 491928 [details] [diff] [review]
Part 2: fix the a11y test

the problem is not in mochitest

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> Created attachment 491928 [details] [diff] [review]
> Part 2: fix the a11y test
> 
> So, here's the stack where we fire the textchange event Alex mentioned in
> comment 31: http://pastebin.mozilla.org/859234.
> 
> We don't really want to not run nsCSSFrameConstructor::ContectAppended, nor do
> we want to special case the nsAccessibilityService::ContentRangeInserted call,
> because we don't have any idea that the text node is being appended because of
> an editor operation.  I think we should just initialize the text control in the
> accessibility test before the event checkers are run.  If accessibility doesn't
> want to get text change events for input control initializations, it should
> probably handle a lot more than just this particular case anyway, so I think
> this change is safe.

That should fix a11y problem. New patch is planned?

Comment 41

[(no longer active)]

Comment on attachment 491999 [details] [diff] [review]
Part 2: fix the a11y test

Yeah, sorry this is the wrong patch...

Comment 42

[(no longer active)]

(In reply to comment #39)
> Comment on attachment 491928 [details] [diff] [review]
> Part 2: fix the a11y test
> 
> the problem is not in mochitest

What do you mean?  If you don't agree to my analysis in comment 35, please share your thoughts.

Comment 43

[(no longer active)]

Attachment: Part 3: correct the click event firing for common parents

This patch also breaks this test:

http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug534833.html?force=1

What's happening is that in the tests performed in nsEventStateManager::SetClickCount, we're not considering the case where the text node has been replaced since the button down event, and only the parents are common (which is what happens after this patch, because of the nsPlaintextEditor::InsertText call which happens when initializing the editor as a resulf of the focus event.)

Check for a common parent should only be made in the case where we're in the anonymous tree, because otherwise it would be possible to press mouse down on an element, move the mouse up to a sibling node in the same level and release the mouse button, which would get misinterpreted as a click on the latter element.

This patch corrects the handling of common parent elements for anonymous content.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> If accessibility doesn't
> want to get text change events for input control initializations, it should
> probably handle a lot more than just this particular case anyway, so I think
> this change is safe.

Ok, if nsCSSFrameConstructor::AppendChild happens when editor is initialized for example when user focus it, then I think we don't need text inserted or value change events because AT should announce this change. Is it real scenario? How does input look when editor is initialized, I mean can the use see the text inside of input? In other words does the input control have text node with this text and does it have rendered text?

Comment 45

[(no longer active)]

(In reply to comment #44)
> (In reply to comment #35)
> > If accessibility doesn't
> > want to get text change events for input control initializations, it should
> > probably handle a lot more than just this particular case anyway, so I think
> > this change is safe.
> 
> Ok, if nsCSSFrameConstructor::AppendChild happens when editor is initialized
> for example when user focus it, then I think we don't need text inserted or
> value change events because AT should announce this change. Is it real
> scenario?

Yes.  That's what happens in the accessibility test case.  To reproduce it, go to a web page with an <input type=text value=foo> tag, and focus the input field.

> How does input look when editor is initialized, I mean can the use
> see the text inside of input?

Yes.  Why wouldn't they?

> In other words does the input control have text
> node with this text and does it have rendered text?

The content tree layout is as usual: an anonymous div with a textnode containing "foo" followed by a br node under it.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #45)

> > How does input look when editor is initialized, I mean can the use
> > see the text inside of input?
> 
> Yes.  Why wouldn't they?
> 
> > In other words does the input control have text
> > node with this text and does it have rendered text?
> 
> The content tree layout is as usual: an anonymous div with a textnode
> containing "foo" followed by a br node under it.

Sorry I should have say "uninitialized" instead "initialized".

Comment 47

[(no longer active)]

(In reply to comment #46)
> (In reply to comment #45)
> 
> > > How does input look when editor is initialized, I mean can the use
> > > see the text inside of input?
> > 
> > Yes.  Why wouldn't they?
> > 
> > > In other words does the input control have text
> > > node with this text and does it have rendered text?
> > 
> > The content tree layout is as usual: an anonymous div with a textnode
> > containing "foo" followed by a br node under it.
> 
> Sorry I should have say "uninitialized" instead "initialized".

Yes, they are the same (without a br node, of course, that was my mistake).  Here's a dump of the frame tree before and after focusing an <input type=text>: http://pastebin.mozilla.org/860441.

Comment 48

[alexander :surkov (:asurkov)]

So why is nsCSSFrameConstructor::AppendContent expected here if text frame is presented in hierarchy already?

Comment 49

[(no longer active)]

(In reply to comment #48)
> So why is nsCSSFrameConstructor::AppendContent expected here if text frame is
> presented in hierarchy already?

A new textnode is injected into the DOM (note that the content pointer values for the two textframes are different).

Comment 50

[alexander :surkov (:asurkov)]

So the text frame isn't changed, but its content is changed (btw, do we get ContentRemoved here?), nothing is changed from user point of view. And therefore the question is why do we need to replace text node?

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 492091 [details] [diff] [review]
Part 3: correct the click event firing for common parents

Have you tested if this affects to scrollbar behavior?
Actually, also video controls have nested anonymous content.

Comment 52

[(no longer active)]

(In reply to comment #50)
> So the text frame isn't changed, but its content is changed (btw, do we get
> ContentRemoved here?), nothing is changed from user point of view. And
> therefore the question is why do we need to replace text node?

That's a good question.  I actually came up with a patch which avoids injecting that new textnode (which is good!), but it fails yet another accessibility test (this time it's accessible/attributes/test_text.html).  I'm investigating the failure...

Comment 53

[(no longer active)]

(In reply to comment #51)
> Comment on attachment 492091 [details] [diff] [review]
> Part 3: correct the click event firing for common parents
> 
> Have you tested if this affects to scrollbar behavior?
> Actually, also video controls have nested anonymous content.

With the new patch that I'm talking about in comment 51, this patch might not be necessary any more.  I'll have more to say when I figure out what's wrong with this other accessibility test.  :-)

Comment 54

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> I actually came up with a patch which avoids injecting
> that new textnode (which is good!), but it fails yet another accessibility test
> (this time it's accessible/attributes/test_text.html).  I'm investigating the
> failure...

Let's investigate the failure since the behavior looks correct.

Comment 55

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan, as per our chat, I hate to say it but I think you need to roll a separate insertText implementation for this case, somehow future proofing as best you can. Swapping nodes between a mouse down and mouse up is fraught with peril.

Comment 56

[(no longer active)]

Attachment: New patch

OK, this new patch should cover all of the cases, and not regress the a11y tests.  As a matter of fact, it seems to be fixing some a11y tests which are broken right now!  I'll attach a separate patch for the a11y test adjustments needed.  I've posted this to the try server to see how it turns out, but it seemed to be passing all of the tests locally.

Comment 57

[(no longer active)]

Attachment: Accessibility test adjustments

Switching from kTodo to kOk for the tests which now pass.

Comment 58

[alexander :surkov (:asurkov)]

Comment on attachment 492505 [details] [diff] [review]
Accessibility test adjustments

great to see, thanks

Comment 59

[(no longer active)]

Comment on attachment 492505 [details] [diff] [review]
Accessibility test adjustments

Hmm, I wish I knew what happens in the a11y tests!  These tests originally failed with TEST-UNEXPECTED-PASS for me locally (on a Mac) with these patches, so I submitted this patch, but they failed on the try server with this patch, which means that the issue is not fixed (it somehow works for me locally though!).

At any rate, this patch is not needed.  Sorry for the noise.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492504 [details] [diff] [review]
New patch

>+      nsCOMPtr<nsIDOMCharacterData> textNode = do_QueryInterface(textContent);
>+      if (textNode) {
>+        textNode->GetData(currentValue);

Can it be a non-text node?  If not, then:

  textContent->GetText()->AppendTo(currentValue);

would work too.  r=me either way.

Comment 61

[(no longer active)]

(In reply to comment #60)
> Comment on attachment 492504 [details] [diff] [review]
> New patch
> 
> >+      nsCOMPtr<nsIDOMCharacterData> textNode = do_QueryInterface(textContent);
> >+      if (textNode) {
> >+        textNode->GetData(currentValue);
> 
> Can it be a non-text node?  If not, then:
> 
>   textContent->GetText()->AppendTo(currentValue);
> 
> would work too.  r=me either way.

In some cases it can actually be a BR node, I guess.

Comment 62

[(no longer active)]

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