Closed Bug 821307 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Editor, defect)

2.0 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: valebtin, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file 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.
Component: Untriaged → General
Attachment #691814 - Attachment mime type: text/plain → text/html
It works if the user clicks in the imput field just after the dots.
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.
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
Attached file 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.
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
Blocks: 221820
Status: UNCONFIRMED → NEW
Component: General → Editor
Ever confirmed: true
OS: Windows 7 → All
Product: Firefox → Core
Version: 17 Branch → 2.0 Branch
(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.
Aryeh, can you please take a look?  Thanks!
Assignee: nobody → ayg
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?
Flags: needinfo?(ehsan)
Hmm, I can't even reproduce the bug any more.  Can you?
Flags: needinfo?(ehsan)
(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.
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.
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.
Flags: needinfo?(ehsan)
(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...)
Flags: needinfo?(ehsan) → needinfo?(roc)
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
Flags: needinfo?(roc)
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.
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.
(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.
Also, how do you guys reproduce this?  I'm not sure why I can't reproduce any more, that's a bit concerning.
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.
(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.
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.
(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.
Assignee: ayg → roc
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...
Attachment #8406772 - Flags: review?(ehsan) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/5b54b4287fbf
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 997805
Depends on: 1001936
No longer depends on: 997805
You need to log in before you can comment on or make changes to this bug.