595310 - Drag and drop of text into an input field does not remove placeholder

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[bugzilla]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre
Build Identifier: 20100909031151

 

Reproducible: Always

Steps to Reproduce:
Drag text into an input field that has a placeholder attribute.
Actual Results:  
The dragged text is displayed on top of the placeholder text.

Expected Results:  
The placeholder text should disappear.

Works as expected in Google Chrome. (In Chrome, the input field additionally gets the focus - maybe it works just because of that.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Taking focus would definitely drop placeholder styling.

Mounir, I thought we'd tested this....

Comment 2

[Mounir Lamouri (:mounir)]

Eh, thanks for reporting. I don't think we have a test checking that. We had issues with drag and drop but it was because the placeholder div was taking the mouse events. So, there are some tests checking that they don't.

I've attached a data-url to test the issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b219912edfec&tochange=ac1ed3f6b2e7

So yeah, regression from bug 534785 (moving editor to content).

Comment 4

[Boris Zbarsky [:bzbarsky]]

nsTextControlFrame::SetValueChanged used to show/hide placeholders.  That code was removed, but I see no obvious replacement for it on the content side...

In particular, the codepath coming through nsTextInputListener::EditAction will still call SetValueChanged() on the frame, but this will now just call SetValueChanged() on the content which did not grow any handling of placeholders. Either the content's SetValueChanged needs to handle this, or EditAction does, I would think.

Comment 5

[Boris Zbarsky [:bzbarsky]]

May be possible to write a test for this by grabbing the editor off the node and issuing direct editor commands....

Comment 6

[Benjamin Smedberg]

This is ugly, but I don't think it blocks.

Comment 7

[(no longer active)]

I think the correct way to fix this bug would be to add this removed code <http://hg.mozilla.org/mozilla-central/rev/2437636244f3#l19.2138> to the content's SetValueChanged method.

Comment 8

[Mounir Lamouri (:mounir)]

Kyle, it would be great to have this fixed before we ship. Please let me know as soon as possible if you think you will not have time for it.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I could fix it this weekend, I've just been prioritizing things that have b+.

Comment 10

[Mounir Lamouri (:mounir)]

Kyle, any update?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This fixes the bug.

I modified nsTextInputListener::EditAction to handle the placeholder rather than mucking with the content objects themselves because it feels neater to me to have the multiple sites that need to deal with this contained in this file rather than spread out among all the content nodes.

The nsTextEditorState* is perhaps not ideal but based on the comments in the header there shouldn't be any lifetime issues with a stale * (nsTextEditorState is always supposed to be the last owner of the nsTextInputListener).  From my ten minutes in this file it's not clear why nsTextEditorState and nsTextInputListener aren't the same object.  I'd like to merge them if possible; that would eliminate the need for nsTextInputListener to maintain a pointer to its owner and be generally neater IMO.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This doesn't quite work (it crashes on a few mochitests) but this is more or less what I'd like to do.  It's probably too risky for 2.0 though.

Comment 13

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

I think it would be best to clear out the nsTextEditorState* from nsTextInputListener when the nsTextEditorState is destroyed.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Simpler patch

Why not a patch fixing this bug as described in comments 4 and 7?

We could open a follow-up for a more complex patch that can't go in Gecko 2.0.

This patch tries to test the bug but drag and drop testing is a real pain :/

Comment 15

[Mounir Lamouri (:mounir)]

Oups, I didn't see the first patch.
The first one is fixing the issue and the second one is just what you would like as a follow-up?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The second is what I would like to do, the first is what I think is safe enough for 2.0.  I agree that the second should ultimately end up in another bug.

I'll address roc's comment once I'm back at my dev machine.  I suppose I should be asking a DOM peer for review here too since that's where the code lives now.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 500363 [details] [diff] [review]
Simpler patch

I will try to understand why the test doesn't work.

I guess you can ask Boris to review this patch given that he is both DOM and layout peer.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Test

I was close to have a working-enough synthesizeDrop to really simulate the drag and drop but I already spent to much time on that. Let's use nsIPlaintextEditor.insertText and I will try to see later if I can simulate a drag and drop on a text field.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Test

Hmm, for some reasons the previous test was never failing.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 500513 [details] [diff] [review]
Test

r=me

Comment 21

[(no longer active)]

Comment on attachment 499760 [details] [diff] [review]
Patch

C++ mistake: constructors taking more than 1 parameters do not need to be explicit.

Have you considered just calling ValueWasChanged from OnValueChanged?

Comment 22

[(no longer active)]

Comment on attachment 500513 [details] [diff] [review]
Test

This test is insufficient.  We also need to test the behavior of textarea's here.

Comment 23

[(no longer active)]

(In reply to comment #18)
> Created attachment 500512 [details] [diff] [review]
> Test
> 
> I was close to have a working-enough synthesizeDrop to really simulate the drag
> and drop but I already spent to much time on that. Let's use
> nsIPlaintextEditor.insertText and I will try to see later if I can simulate a
> drag and drop on a text field.

The dragdrop support in our test suite doesn't work with the way that editor handles drag and drop...

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Mounir's patch is better.  He's going to finish this off.

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 26

[(no longer active)]

Comment on attachment 507158 [details] [diff] [review]
Patch v1

>+  var i = document.getElementById('i');
>+  var t = document.getElementById('t');
>+  content.removeChild(document.getElementById('input-witness'));
>+  content.removeChild(document.getElementById('textarea-witness'));
>+  content.appendChild(i);
>+  content.appendChild(document.createTextNode(' '));
>+  content.appendChild(t);

i and t are already in the DOM.  I'm not sure if injecting them again is valid.  Maybe you want cloneNode?

>+  SimpleTest.executeSoon(function() {

nsHTML{Input,TextArea}Element::GetEditor wil fail if the element doesn't have a frame.  You should explicitly flush using something like |document.body.clientWidth;| instead of relying on executeSoon to do that for you.

>+    function synthesizeDropText(aElement, aText)
>+    {
>+      var Ci = Components.interfaces;
>+      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Swap those two lines, please.

>+
>+  if (!nsContentUtils::IsFocusedContent(mContent)) {
>+    // If the content is focused, we don't care about the changes because
>+    // the placeholder is going to be hidden/shown on blur.
>+    PRInt32 textLength;
>+    GetTextLength(&textLength);
>+
>+    nsWeakFrame weakFrame(this);
>+    txtCtrl->SetPlaceholderClass(!textLength, PR_TRUE);
>+    if (!weakFrame.IsAlive()) {
>+      return;
>+    }
>+  }

It's probably a good idea to add a helper function for this, and call it from SetFocus too.

Comment 27

[(no longer active)]

(In reply to comment #26)
> Comment on attachment 507158 [details] [diff] [review]
> Patch v1
> 
> >+  var i = document.getElementById('i');
> >+  var t = document.getElementById('t');
> >+  content.removeChild(document.getElementById('input-witness'));
> >+  content.removeChild(document.getElementById('textarea-witness'));
> >+  content.appendChild(i);
> >+  content.appendChild(document.createTextNode(' '));
> >+  content.appendChild(t);
> 
> i and t are already in the DOM.  I'm not sure if injecting them again is valid.
>  Maybe you want cloneNode?

Actually, what is the purpose of all this?  It shouldn't be needed to test this bug at all.

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to comment #26)
> Comment on attachment 507158 [details] [diff] [review]
> Patch v1
> 
> >+  var i = document.getElementById('i');
> >+  var t = document.getElementById('t');
> >+  content.removeChild(document.getElementById('input-witness'));
> >+  content.removeChild(document.getElementById('textarea-witness'));
> >+  content.appendChild(i);
> >+  content.appendChild(document.createTextNode(' '));
> >+  content.appendChild(t);
> 
> i and t are already in the DOM.  I'm not sure if injecting them again is valid.
>  Maybe you want cloneNode?

It's perfectly fine to move nodes in the DOM like that.

> >+  SimpleTest.executeSoon(function() {
> 
> nsHTML{Input,TextArea}Element::GetEditor wil fail if the element doesn't have a
> frame.  You should explicitly flush using something like
> |document.body.clientWidth;| instead of relying on executeSoon to do that for
> you.

Already done locally with .getBoundingClientRect().

> >+    function synthesizeDropText(aElement, aText)
> >+    {
> >+      var Ci = Components.interfaces;
> >+      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> 
> Swap those two lines, please.

Ok.

> >+  if (!nsContentUtils::IsFocusedContent(mContent)) {
> >+    // If the content is focused, we don't care about the changes because
> >+    // the placeholder is going to be hidden/shown on blur.
> >+    PRInt32 textLength;
> >+    GetTextLength(&textLength);
> >+
> >+    nsWeakFrame weakFrame(this);
> >+    txtCtrl->SetPlaceholderClass(!textLength, PR_TRUE);
> >+    if (!weakFrame.IsAlive()) {
> >+      return;
> >+    }
> >+  }
> 
> It's probably a good idea to add a helper function for this, and call it from
> SetFocus too.

Hmmm, why not.

(In reply to comment #27)
> (In reply to comment #26)
> > Comment on attachment 507158 [details] [diff] [review]
> > Patch v1
> > 
> > >+  var i = document.getElementById('i');
> > >+  var t = document.getElementById('t');
> > >+  content.removeChild(document.getElementById('input-witness'));
> > >+  content.removeChild(document.getElementById('textarea-witness'));
> > >+  content.appendChild(i);
> > >+  content.appendChild(document.createTextNode(' '));
> > >+  content.appendChild(t);
> > 
> > i and t are already in the DOM.  I'm not sure if injecting them again is valid.
> >  Maybe you want cloneNode?
> 
> Actually, what is the purpose of all this?  It shouldn't be needed to test this
> bug at all.

There is no way to know if the placeholder div is shown or not. The only way to do that is to create an input and a textarea with "bar" inside, take a screenshot then do our test and check that we have the same rendering (a reftest isn't doable because we need privileges). When the test fails, both elements have foo and bar mixed up.

Comment 29

[(no longer active)]

(In reply to comment #28)
> There is no way to know if the placeholder div is shown or not. The only way to
> do that is to create an input and a textarea with "bar" inside, take a
> screenshot then do our test and check that we have the same rendering (a
> reftest isn't doable because we need privileges). When the test fails, both
> elements have foo and bar mixed up.

Still, you don't need to mess up with the DOM like that.  Just use the "witness" fields, take a screenshot (s1), then empty them and set the placeholder attribute, take another screenshot (s2), synthesize drop, and take a third screenshot (s3).  Now, you should verify that s1!=s2 (to make sure that the placeholder stuff shows up, and s1==s3 (like you do right now).

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to comment #29)
> (In reply to comment #28)
> > There is no way to know if the placeholder div is shown or not. The only way to
> > do that is to create an input and a textarea with "bar" inside, take a
> > screenshot then do our test and check that we have the same rendering (a
> > reftest isn't doable because we need privileges). When the test fails, both
> > elements have foo and bar mixed up.
> 
> Still, you don't need to mess up with the DOM like that.  Just use the
> "witness" fields, take a screenshot (s1), then empty them and set the
> placeholder attribute, take another screenshot (s2), synthesize drop, and take
> a third screenshot (s3).  Now, you should verify that s1!=s2 (to make sure that
> the placeholder stuff shows up, and s1==s3 (like you do right now).

You are right. Actually, it wasn't working when I tried that but I know understand why: I wasn't forcing a reflow so I guess the placeholder wasn't created when the drag and drop was simulated so the test was always positive. I've updated it and it makes thing simpler, indeed.

(In reply to comment #26)
> >+  if (!nsContentUtils::IsFocusedContent(mContent)) {
> >+    // If the content is focused, we don't care about the changes because
> >+    // the placeholder is going to be hidden/shown on blur.
> >+    PRInt32 textLength;
> >+    GetTextLength(&textLength);
> >+
> >+    nsWeakFrame weakFrame(this);
> >+    txtCtrl->SetPlaceholderClass(!textLength, PR_TRUE);
> >+    if (!weakFrame.IsAlive()) {
> >+      return;
> >+    }
> >+  }
> 
> It's probably a good idea to add a helper function for this, and call it from
> SetFocus too.

I thought it would have been possible easily but it's not exactly the same logic and I do not think that merging them would make things simpler/worths it.

Comment 31

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 32

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Hmmm, I'm sure I've selected the correct file. Be careful, file picker, don't play too much with that...

Comment 33

[(no longer active)]

Comment on attachment 507449 [details] [diff] [review]
Patch v1.1

Looks great, but please move the test to layout/forms/tests, as this is testing textframe code, and not content code.  :-)

Comment 34

[Mounir Lamouri (:mounir)]

Comment on attachment 507449 [details] [diff] [review]
Patch v1.1

This is 2.0-wanted so I guess it should be pushed ;)

Comment 35

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2 [ready to land]

Comment 36

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b005c115fea2

Comment 37

[Mounir Lamouri (:mounir)]

Thank you for pushing that Jonathan :)