821307 - password input field doesn't work correctly in the particular scenario

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[valebtin]

Attachment: Page with the issue

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

1) Create a simple html page with 2 inputs:
<input type="password" id='disabledPassword' disabled></input>
<input type="password" id='password' value='11111111111111111'></input>​

2) Add focus event listener to the second input:
(function(){
    var passwordField = document.getElementById('password');
    var focusHandler = function(event){
        passwordField.value= '';
    };
    password.addEventListener('focus', focusHandler);
})();​

3) Open this page
4) Click on the second password input (on the area with dots)
5) Click on the second password input (on the area with dots) again

Note:
1) it's important to have disabled input field
2) when clicking not on the area with dots I have expected behavior

Please find page with the bug: http://jsfiddle.net/9AeLK/

Tested in FireFox 16, FireFox 17, FireFox Beta 18.0b3


Actual results:

When I click first time on the password input nothing happens.
For the second click I have event handler executed.


Expected results:

Focus event handler should be executed when clicking first time on the password field.

Comment 1

[Loic]

It works if the user clicks in the imput field just after the dots.

Comment 2

[Loic]

Not sure if it's a real regression, but in the past, it was possible to execute the event handler in the input field on the 1st click between the dots (even if the dots weren't erased).

m-c
good=2010-11-07
bad=2010-11-08
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8cbe83542596&tochange=a0826dcd6228

CC'ing Alice.

Comment 3

[Alice0775 White]

I cannot reproduce the problem in Firefox17 on Windows7.
The black dots disappears when i click on the dots at first time.
http://hg.mozilla.org/releases/mozilla-release/rev/c23c45132139
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0 ID:20121128204232

Comment 4

[Alice0775 White]

Attachment: testcase html

Ok
It depend on whether the password field is overflowed or not.
If the password field had been overflowed, the problem does not occur.

Comment 5

[Alice0775 White]

Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/9821a96e7230
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre ID:20100411172027
Bad:
http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100411 Minefield/3.7a5pre ID:20100411174811
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9821a96e7230&tochange=d3e7c18f58fc

Suspected: Bug 221820

Comment 6

[Loic]

(In reply to Alice0775 White from comment #4)
> Created attachment 692252 [details]
> testcase html

In the 1st input field, if I click after the dots, it's OK. If I click between the dots, I need to click twice to have the focus on the input.

Comment 7

[(no longer active)]

Aryeh, can you please take a look?  Thanks!

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I'm not so familiar with this code, and don't really know where to start.  Could you point me to some likely functions to examine?

Comment 9

[(no longer active)]

Hmm, I can't even reproduce the bug any more.  Can you?

Comment 10

[Loic]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> Hmm, I can't even reproduce the bug any more.  Can you?

Check my comment #6.

Comment 11

[(no longer active)]

Ah, clicking between the dots is the key!  Thanks.

Aryeh, in order to reproduce this bug, I suggest that you break on nsFrame::HandlePress and see what element we end up with in the hit testing and what we try to do with it.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

On the click that doesn't do anything, nsFrame::HandlePress isn't called.  Only the second click (which works) calls it.  Any other ideas for things to watch?  It looks like the event isn't fired to start with.

Comment 13

[(no longer active)]

(In reply to :Aryeh Gregor from comment #12)
> On the click that doesn't do anything, nsFrame::HandlePress isn't called. 
> Only the second click (which works) calls it.  Any other ideas for things to
> watch?  It looks like the event isn't fired to start with.

Hmm, I would expect HandlePress to be called for *all* clicks, isn't that the case roc?

(BTW, I seem to be unable to reproduce the bug again...)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can reproduce the bug and I'm debugging it now (with rr!).

The problem is that the EventStateManager::PostHandleEvent processing for the first click doesn't focus the text input, because the nsTextFrame that is was target of the mouse event has been destroyed as follows:

#0  PresShell::NotifyDestroyingFrame (this=0x77e9a4c0, aFrame=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/base/nsPresShell.cpp:2097
#1  0x5d5b01bd in nsFrame::DestroyFrom (this=0x6cfd7378, 
    aDestructRoot=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/generic/nsFrame.cpp:653
#2  0x5d6fcdd8 in nsTextFrame::DestroyFrom (this=0x6cfd7378, 
    aDestructRoot=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/generic/nsTextFrame.cpp:3910
#3  0x5d4a75c1 in nsIFrame::Destroy (this=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/base/../generic/nsIFrame.h:444
#4  0x5d541f18 in nsBlockFrame::DoRemoveFrame (this=0x6cfd71f8, 
    aDeletedFrame=0x6cfd7378, aFlags=2)
    at /home/roc/mozilla-inbound/layout/generic/nsBlockFrame.cpp:5492
#5  0x5d53e5b9 in nsBlockFrame::RemoveFrame (this=0x6cfd71f8, 
    aListID=mozilla::layout::kPrincipalList, aOldFrame=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/generic/nsBlockFrame.cpp:5051
#6  0x5d454939 in nsFrameManager::RemoveFrame (this=0x81f37350, 
    aListID=mozilla::layout::kPrincipalList, aOldFrame=0x6cfd7378)
    at /home/roc/mozilla-inbound/layout/base/nsFrameManager.cpp:413
#7  0x5d3d076d in nsCSSFrameConstructor::ContentRemoved (this=0x81f37350, 
    aContainer=0x6d23add0, aChild=0x6d23ae70, aOldNextSibling=0x0, 
    aFlags=nsCSSFrameConstructor::REMOVE_CONTENT, aDidReconstruct=0xffff9f77)
    at /home/roc/mozilla-inbound/layout/base/nsCSSFrameConstructor.cpp:7674
#8  0x5d2cf08d in PresShell::ContentRemoved (this=0x77e9a4c0, aDocument=
    0x86b49000, aContainer=0x6d23add0, aChild=0x6d23ae70, aIndexInContainer=0, 
    aPreviousSibling=0x0)
    at /home/roc/mozilla-inbound/layout/base/nsPresShell.cpp:4362
#9  0x5c245186 in nsNodeUtils::ContentRemoved (aContainer=0x6d23add0, 
    aChild=0x6d23ae70, aIndexInContainer=0, aPreviousSibling=0x0)
    at /home/roc/mozilla-inbound/content/base/src/nsNodeUtils.cpp:184
#10 0x5c1ed7c1 in nsINode::doRemoveChildAt (this=0x6d23add0, aIndex=0, 
    aNotify=true, aKid=0x6d23ae70, aChildArray=...)
    at /home/roc/mozilla-inbound/content/base/src/nsINode.cpp:1590
#11 0x5c06317d in mozilla::dom::FragmentOrElement::RemoveChildAt (
    this=0x6d23add0, aIndex=0, aNotify=true)
    at /home/roc/mozilla-inbound/content/base/src/FragmentOrElement.cpp:1056
#12 0x5c1e3d5e in nsINode::RemoveChild (this=0x6d23add0, aOldChild=..., 
    aError=...) at /home/roc/mozilla-inbound/content/base/src/nsINode.cpp:486
#13 0x5cd14429 in DeleteNodeTxn::DoTransaction (this=0x747812e0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/DeleteNodeTxn.cpp:70
#14 0x5cd198ec in EditAggregateTxn::DoTransaction (this=0x747812b0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/EditAggregateTxn.cpp:34
#15 0x5cd15e63 in DeleteRangeTxn::DoTransaction (this=0x747812b0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/DeleteRangeTxn.cpp:93
#16 0x5cd198ec in EditAggregateTxn::DoTransaction (this=0x6ea2f1a0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/EditAggregateTxn.cpp:34
#17 0x5afee773 in nsTransactionItem::DoTransaction (this=0x6ea2f1e0)
    at /home/roc/mozilla-inbound/editor/txmgr/src/nsTransactionItem.cpp:173
#18 0x5aff9252 in nsTransactionManager::BeginTransaction (this=0x81f31ca0, 
---Type <return> to continue, or q <return> to quit---
    aTransaction=0x6ea2f1a0, aData=0x0)
    at /home/roc/mozilla-inbound/editor/txmgr/src/nsTransactionManager.cpp:784
#19 0x5aff443e in nsTransactionManager::DoTransaction (this=0x81f31ca0, 
    aTransaction=0x6ea2f1a0)
    at /home/roc/mozilla-inbound/editor/txmgr/src/nsTransactionManager.cpp:79
#20 0x5cd32096 in nsEditor::DoTransaction (this=0x7dbe4580, aTxn=0x6ea2f1a0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/nsEditor.cpp:695
#21 0x5cd55748 in nsEditor::DeleteSelectionImpl (this=0x7dbe4580, aAction=0, 
    aStripWrappers=0)
    at /home/roc/mozilla-inbound/editor/libeditor/base/nsEditor.cpp:4125
#22 0x5cdc1432 in nsTextEditRules::WillDeleteSelection (this=0x7db1bf40, 
    aSelection=0x6d23af10, aCollapsedAction=0, aCancel=0xffffa4c7, 
    aHandled=0xffffa4c6)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsTextEditRules.cpp:861
#23 0x5cdba8fa in nsTextEditRules::WillDoAction (this=0x7db1bf40, 
    aSelection=0x6d23af10, aInfo=0xffffa494, aCancel=0xffffa4c7, 
    aHandled=0xffffa4c6)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsTextEditRules.cpp:243
#24 0x5cdad5a9 in nsPlaintextEditor::DeleteSelection (this=0x7dbe4580, 
    aAction=0, aStripWrappers=0)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsPlaintextEditor.cpp:674
#25 0x5cdbe515 in nsTextEditRules::WillInsertText (this=0x7db1bf40, 
    aAction=insertText, aSelection=0x6d23af10, aCancel=0xffffa7ab, 
    aHandled=0xffffa7aa, inString=0xffffa938, outString=0xffffa714, 
    aMaxLength=-1)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsTextEditRules.cpp:612
#26 0x5cdba88d in nsTextEditRules::WillDoAction (this=0x7db1bf40, 
    aSelection=0x6d23af10, aInfo=0xffffa6e4, aCancel=0xffffa7ab, 
    aHandled=0xffffa7aa)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsTextEditRules.cpp:240
#27 0x5cdadcdf in nsPlaintextEditor::InsertText (this=0x7dbe4580, 
    aStringToInsert=...)
    at /home/roc/mozilla-inbound/editor/libeditor/text/nsPlaintextEditor.cpp:716
#28 0x5c663a61 in nsTextEditorState::SetValue (this=0x6d054790, aValue=..., 
    aUserInput=false, aSetValueChanged=false)
    at /home/roc/mozilla-inbound/content/html/content/src/nsTextEditorState.cpp:1879
#29 0x5c65de71 in nsTextEditorState::PrepareEditor (this=0x6d054790, 
    aValue=0x0)
    at /home/roc/mozilla-inbound/content/html/content/src/nsTextEditorState.cpp:1370
#30 0x5c51459d in mozilla::dom::HTMLInputElement::CreateEditor (
    this=0x559548c0)
    at /home/roc/mozilla-inbound/content/html/content/src/HTMLInputElement.cpp:2440
#31 0x5d79af6b in nsTextControlFrame::EnsureEditorInitialized (this=0x6cfd29d8)
    at /home/roc/mozilla-inbound/layout/forms/nsTextControlFrame.cpp:293
---Type <return> to continue, or q <return> to quit---
#32 0x5c51a4fa in mozilla::dom::HTMLInputElement::PreHandleEvent (
    this=0x559548c0, aVisitor=...)
    at /home/roc/mozilla-inbound/content/html/content/src/HTMLInputElement.cpp:3270
#33 0x5b72fe7b in mozilla::EventTargetChainItem::PreHandleEvent (
    this=0x7471901c, aVisitor=...)
    at /home/roc/mozilla-inbound/dom/events/EventDispatcher.cpp:230
#34 0x5b732f58 in mozilla::EventDispatcher::Dispatch (aTarget=0x6d23add0, 
    aPresContext=0x8639b400, aEvent=0xffffb2a8, aDOMEvent=0x0, 
    aEventStatus=0xffffb1d4, aCallback=0xffffaeb8, aTargets=0x0)
    at /home/roc/mozilla-inbound/dom/events/EventDispatcher.cpp:560
#35 0x5d2e8756 in PresShell::HandleEventInternal (this=0x77e9a4c0, 
    aEvent=0xffffb2a8, aStatus=0xffffb1d4)
    at /home/roc/mozilla-inbound/layout/base/nsPresShell.cpp:7389
#36 0x5d2e6054 in PresShell::HandlePositionedEvent (this=0x77e9a4c0, 
    aTargetFrame=0x6cfd7378, aEvent=0xffffb2a8, aEventStatus=0xffffb1d4)
    at /home/roc/mozilla-inbound/layout/base/nsPresShell.cpp:7118
#37 0x5d2e4460 in PresShell::HandleEvent (this=0x6c076800, aFrame=0x781b02b8, 
    aEvent=0xffffb2a8, aDontRetargetEvents=false, aEventStatus=0xffffb1d4)
    at /home/roc/mozilla-inbound/layout/base/nsPresShell.cpp:6918

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That means mCurrentEventTargetFrame is null. Also, instantiating the editor removes the anonymous text node from the document (replacing it with a new node), so PresShell::GetCurrentEventTargetFrame() can't find a new frame to use instead.

I'm not sure how to fix this yet.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One issue is that SetValue really shouldn't be doing anything at all, because we're not changing the value here. nsTextEditorState::SetValue checks if (!currentValue.Equals(aValue)) but that returns true since currentValue is a string of bullets and aValue is the actual password.

Comment 17

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> One issue is that SetValue really shouldn't be doing anything at all,
> because we're not changing the value here. nsTextEditorState::SetValue
> checks if (!currentValue.Equals(aValue)) but that returns true since
> currentValue is a string of bullets and aValue is the actual password.

Ah I bet that is the bug, isn't it?  We should really compare the argument to the real value of the form control, not those bullet characters.

Comment 18

[(no longer active)]

Also, how do you guys reproduce this?  I'm not sure why I can't reproduce any more, that's a bit concerning.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

On the page attached to this bug, I click on the dots.  If you click at the edge of the input, the bug doesn't occur.  The cursor should be vertically aligned near the middle of the input, and horizontally somewhere within the dotted region (and not close to its left or right edge).  Reproduces consistently on self-compiled Linux and on downloaded Windows 7 nightly.

Comment 20

[(no longer active)]

(In reply to comment #19)
> On the page attached to this bug, I click on the dots.  If you click at the
> edge of the input, the bug doesn't occur.  The cursor should be vertically
> aligned near the middle of the input, and horizontally somewhere within the
> dotted region (and not close to its left or right edge).  Reproduces
> consistently on self-compiled Linux and on downloaded Windows 7 nightly.

I think I got the hang of it again, finally!  Thanks.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW the reason the overflowing text control doesn't show the problem is because it receives an "overflow" event and HTMLInputElement::NeedToInitializeEditorForEvent returns true for that event. So all overflow text inputs instantiate their editors. We should fix that.

Comment 22

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> BTW the reason the overflowing text control doesn't show the problem is
> because it receives an "overflow" event and
> HTMLInputElement::NeedToInitializeEditorForEvent returns true for that
> event. So all overflow text inputs instantiate their editors. We should fix
> that.

Agreed, filed bug 994940.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Green on Linux. https://tbpl.mozilla.org/?tree=Try&rev=f21b1ba54854

Comment 24

[(no longer active)]

Comment on attachment 8406772 [details] [diff] [review]
fix

Review of attachment 8406772 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/test_bug821307.html
@@ +27,5 @@
> +  dummy.focus();
> +  is(document.activeElement, dummy, "Check dummy element is now focused");
> +
> +  var input = document.getElementById('input');
> +  synthesizeMouse(input, 30, 30, {});

This really depends on the placement of the asterisk chars for it to do the right thing, which is fragile  and won't test what we want across platform.  Please brute-force this by for example going from 1 to 30 across the X axis and running the test once for each value.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +230,3 @@
>  {
>    NS_PRECONDITION(aDoc && !aSelCon, "bad arg");
>    NS_ENSURE_TRUE(aDoc, NS_ERROR_NULL_POINTER);

Can you MOZ_ASSERT() that aInitialValue is an empty string here please?  This will alleviate part of my "ewww" comment. :-)

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ -125,5 @@
>      mRules->DetachEditor();
>      mRules = nullptr;
>    }
>    
> -  if (1)

!!!

@@ +141,5 @@
>  
>    NS_ENSURE_SUCCESS(rulesRes, rulesRes);
> +
> +  // mRules may not have been initialized yet, when this is called via
> +  // nsHTMLEditor::Init.

Ewww...  I wanted to r- because of this, but the problem is only there in theory (since we won't do anything with this for nsHTMLEditor) and I can't think of a better way to do this short of storing the initial value in a member variable which is not nice...

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #24)
> ::: content/html/content/test/test_bug821307.html
> @@ +27,5 @@
> > +  dummy.focus();
> > +  is(document.activeElement, dummy, "Check dummy element is now focused");
> > +
> > +  var input = document.getElementById('input');
> > +  synthesizeMouse(input, 30, 30, {});
> 
> This really depends on the placement of the asterisk chars for it to do the
> right thing, which is fragile  and won't test what we want across platform. 
> Please brute-force this by for example going from 1 to 30 across the X axis
> and running the test once for each value.

I don't really know how to make that work.

I changed the code to
  var rect = input.getBoundingClientRect();
  synthesizeMouse(input, 100, rect.height/2, {});
The font size is 40px so this should be reasonably robust. It's only necessary to hit the text node, we don't have to hit any specific character.

Comment 26

[(no longer active)]

(In reply to comment #25)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #24)
> > ::: content/html/content/test/test_bug821307.html
> > @@ +27,5 @@
> > > +  dummy.focus();
> > > +  is(document.activeElement, dummy, "Check dummy element is now focused");
> > > +
> > > +  var input = document.getElementById('input');
> > > +  synthesizeMouse(input, 30, 30, {});
> > 
> > This really depends on the placement of the asterisk chars for it to do the
> > right thing, which is fragile  and won't test what we want across platform. 
> > Please brute-force this by for example going from 1 to 30 across the X axis
> > and running the test once for each value.
> 
> I don't really know how to make that work.
> 
> I changed the code to
>   var rect = input.getBoundingClientRect();
>   synthesizeMouse(input, 100, rect.height/2, {});
> The font size is 40px so this should be reasonably robust. It's only necessary
> to hit the text node, we don't have to hit any specific character.

OK, that's good enough.  :-)  Thanks, let's not block on this further.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b54b4287fbf

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b54b4287fbf