553097 - Placeholder code creates unnecessary frames and DOM nodes

Status: RESOLVED FIXED

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

Description

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

Bug 457800 added 'placeholder' support. It makes us create an extra nsHTMLScrollFrame, nsBlockFrame, nsTextFrame, nsHTMLDivElement and nsTextNode for every single <input type="text">, even those which don't use 'placeholder' (i.e., almost all of them). This is surely a memory and page-load-time regression on input-heavy pages.

We need to only create these objects when 'placeholder' is actually being used. Couldn't we simply modify CreateAnonymousContent to only call CreatePlaceholderDiv when 'placeholder' is set, and make adding or removing 'placeholder' reconstruct the nsTextControlFrame?

Comment 1

[Mounir Lamouri (:mounir)]

That means the placeholder div shouldn't be created in CreateAnonymousContent but should be added to the nsStackFrome when needed ?
Just to be sure I'm not missing something before writing some code.

Comment 2

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

No. It should be created in CreateAnonymousCOntent if the 'placeholder' attribute is present.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> No. It should be created in CreateAnonymousCOntent if the 'placeholder'
> attribute is present.

I don't know how I could add something to the AnonymousContent after the frame initialization. It would be needed if the placeholder attribute is added after the input creation.

Comment 4

[Timothy Nikkel (:tnikkel)]

I think what is being suggested is that addition/removal of the placeholder attribute should cause the frame for the input to be reconstructed.

Comment 5

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

Right. You could do this by calling nsCSSFrameConstructor::PostRestyleEvent in an AttributeChanged callback.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Or better yet, by adding the placeholder attribute to nsHTMLInputElement::GetAttributeChangeHint (and similar for textarea), returning NS_STYLE_HINT_FRAMECHANGE.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I used GetAttributeChangedHint as suggested by Boris to re-create the frame when the placeholder attribute is added or removed.
I also used this patch to add a missing weakFrame check.

Comment 8

[Mounir Lamouri (:mounir)]

Btw, I'm using |IsSingleLineTextControl| method introduced in bug 557620 so we will have to wait for it to be fixed (should happen soon) before landing this patch.

Comment 9

[(no longer active)]

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

Some drive-by review comments follow.

>diff -r bef4ee63e9bb layout/forms/nsTextControlFrame.cpp
>@@ -1529,19 +1530,21 @@ nsTextControlFrame::InitEditor()

You seem to be using an old tree.  nsTextControlFrame::InitEditor no longer exists!  :-)  See bug 221820.  Specifically, I changed a few of the places were placeholders were being modified, so I bet this patch requires updating.  So, I'm clearing the review request for now.

>@@ -1830,42 +1835,45 @@ nsTextControlFrame::IsLeaf() const
>+    if (IsPlaceholderUsed()) {
>+      nsWeakFrame weakFrame(this);
>+
>+      nsAutoString valueString;
>+      GetValue(valueString, PR_TRUE);
>+      if (valueString.IsEmpty()) {
>+        ShowPlaceholder();
>+      }
>+
>+      if (!weakFrame.IsAlive()) {
>+        return;
>+      }

Here, can the weak frame check be moved into the |if (valueString.IsEmpty())| block?

>-  classValue.Assign(NS_LITERAL_STRING("anonymous-div placeholder"));
>+  // We don't use {Get,Set}ClassList because it would be worst for performance.

Nit: s/worst/worse/

>diff -r bef4ee63e9bb layout/forms/nsTextControlFrame.h
>--- a/layout/forms/nsTextControlFrame.h	Fri Apr 16 14:46:14 2010 +0200
>+++ b/layout/forms/nsTextControlFrame.h	Fri Apr 16 15:45:11 2010 +0200
>@@ -332,20 +332,28 @@ private:
>+
>+  // Helper methodse returning if this text control frame uses a placeholder.

Nit: s/methodse/methods/.

>+  PRBool   IsPlaceholderUsed() const { return mPlaceholderDiv != nsnull; }

Nit: according to the coding styles, you should use |return !!mPlaceholderDiv;|.

Also, it would be a very good idea to submit this patch to the try server to make sure that we're not breaking anything before asking for review here.

Comment 10

[(no longer active)]

(In reply to comment #8)
> Btw, I'm using |IsSingleLineTextControl| method introduced in bug 557620 so we
> will have to wait for it to be fixed (should happen soon) before landing this
> patch.

Specifying this by setting a dependency on bug 557620.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I used GetAttributeChangedHint 

That needs to happen for nsHTMLTextareaElement too, right?  Plus tests?

Comment 12

[Mounir Lamouri (:mounir)]

Indeed, I forgot nsHTMLTextareaElement.
I'm wondering how I can test that. With -moz-placeholder, I could force the placeholder to be visible and set the background color to something non-white. There is a way I don't know to get the anonymous contents of an element ?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm wondering how I can test that.

A reftest where the reference is <textarea placeholder="foo"></textarea> and the test is <textarea></textarea> and dynamically adds the placeholder attribute should do the trick, right?

> There is a way I don't know to get the anonymous contents of an element ?

Probably not.  ;)

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Some tests were more or less testing what you recommended, Boris. I just had to remove |placeholder=""|. However, this is not checking the main change of this bug: it's not checking if the placeholder div is present when @placeholder is not specified.

Comment 15

[(no longer active)]

(In reply to comment #14)
> Some tests were more or less testing what you recommended, Boris. I just had to
> remove |placeholder=""|.

Why do you create a placeholder div if an empty placeholder attribute is specified?

> However, this is not checking the main change of this
> bug: it's not checking if the placeholder div is present when @placeholder is
> not specified.

CCing David to see if it's possible to use the accessibility API to get access to the native anonymous content tree from a mochitest.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> CCing David to see if it's possible to use the accessibility API to get access
> to the native anonymous content tree from a mochitest.

It's possible by Gecko accessibility API if native anonymous content accessible. So you try to get accessible object for explicit content by nsIAccessibleRetrival::getAccessibleFor(DOMNode) and get its children (nsIAccessible::GetChildAt() for example), then you can query nsIAccessNode to get DOMNode (nsIAccessNode::DOMNode). But I'm not sure it's good idea to run accessibility machine for non-a11y stuffs.

Comment 17

[(no longer active)]

(In reply to comment #16)
> (In reply to comment #15)
> 
> > CCing David to see if it's possible to use the accessibility API to get access
> > to the native anonymous content tree from a mochitest.
> 
> It's possible by Gecko accessibility API if native anonymous content
> accessible. So you try to get accessible object for explicit content by
> nsIAccessibleRetrival::getAccessibleFor(DOMNode) and get its children
> (nsIAccessible::GetChildAt() for example), then you can query nsIAccessNode to
> get DOMNode (nsIAccessNode::DOMNode).

Good.  I assume that all of those APIs are behind XPConnect, right? (i.e., js accessible).

> But I'm not sure it's good idea to run
> accessibility machine for non-a11y stuffs.

Yes, but we could always add it as an a11y test, right?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (In reply to comment #16)
> Good.  I assume that all of those APIs are behind XPConnect, right? (i.e., js
> accessible).

it's scriptable XPCOM interfaces and accessible retrieval is scriptable XPCOM service.

> > But I'm not sure it's good idea to run
> > accessibility machine for non-a11y stuffs.
> 
> Yes, but we could always add it as an a11y test, right?

Right. Mid-air collisions prevented me to suggest the same :). Just use accessible/tests/mochitests/common.js. There is getAccessible() function there. It could look like

var acc = getAccessible(input);
var anonAcc = acc.getChildAt(indexOfNativeAnonContent);
var anonNode = getNode(anonAcc);

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> (In reply to comment #14)
> > Some tests were more or less testing what you recommended, Boris. I just had to
> > remove |placeholder=""|.
> 
> Why do you create a placeholder div if an empty placeholder attribute is
> specified?

If we want to prevent creation of a placeholder div when @placeholder is set but empty, we need to know the previous value of @placeholder in |GetAttributeChangeHint| and we can't. Except if there is an elegant solution I don't know, this optimization will force us to keep the previous @placeholder value. I'm wondering if it worth it.

Comment 20

[Mounir Lamouri (:mounir)]

By "I'm wondering if it worth it." I mean this will make the code less straightforward. When @placeholder will be modified, deleted or added, we will have check the previous and new values to know if we have to reconstruct the div.

Comment 21

[(no longer active)]

Why do we need to have the previous value around?  We could just check the new value, and if it's empty, reconstruct the frame, otherwise leave the frame be (if IsPlaceholderUsed() == PR_TRUE, that is).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Why not just reconstruct the frame on any change to the placeholder attribute value and be done with it?  Do we expect these to change much?

Comment 23

[(no longer active)]

I don't think that for many common usecases, the attribute would change much, if at all.

Comment 24

[(no longer active)]

Also, Mounir, this probably breaks some of the accessibility tests, have you tested this on the try server?

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> Why not just reconstruct the frame on any change to the placeholder attribute
> value and be done with it?  Do we expect these to change much?

For code readability I prefer that and as we may, indeed, expect @placeholder to be set once, it should be ok regarding performance.

(In reply to comment #24)
> Also, Mounir, this probably breaks some of the accessibility tests, have you
> tested this on the try server?

My patch is using another patch so I need to merge them to send that to the try server but I have a lot of patches applied locally so... I need to found time for that.

Comment 26

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

(In reply to comment #15)
> (In reply to comment #14)
> > Some tests were more or less testing what you recommended, Boris. I just had to
> > remove |placeholder=""|.
> 
> Why do you create a placeholder div if an empty placeholder attribute is
> specified?
> 
> > However, this is not checking the main change of this
> > bug: it's not checking if the placeholder div is present when @placeholder is
> > not specified.
> 
> CCing David to see if it's possible to use the accessibility API to get access
> to the native anonymous content tree from a mochitest.

I think it might be simpler to add tests which want to use a11y API in the a11y test suite; as long as Alexander and Marco agree. Would probably create a folder called 'extras', or 'layout' or something.

I'm not sure though, since it doesn't test a11y exactly.

Comment 27

[(no longer active)]

(In reply to comment #25)
> (In reply to comment #22)
> > Why not just reconstruct the frame on any change to the placeholder attribute
> > value and be done with it?  Do we expect these to change much?
> 
> For code readability I prefer that and as we may, indeed, expect @placeholder
> to be set once, it should be ok regarding performance.

So, I guess we need a new patch here.

> (In reply to comment #24)
> > Also, Mounir, this probably breaks some of the accessibility tests, have you
> > tested this on the try server?
> 
> My patch is using another patch so I need to merge them to send that to the try
> server but I have a lot of patches applied locally so... I need to found time
> for that.

You can just apply all the patches that you have in your tree locally, and say:

hg push -f ssh://hg.mozilla.org/try/

It will push all of the applied patched to the try server, and starts a try run based on all of those patches.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

I've sent a patch to the try server, I will paste the results here.

For the a11y test, I will wait for Alexander and Marco's comments.

Comment 29

[Mounir Lamouri (:mounir)]

Maybe you could review the changes to the layout Ehsan ? So Boris will only have to review the very few lines changes in content. Given his review request list, it would be better ;)
I'm assuming you have the right to review layout patches. If I'm wrong just cancel the review and a feedback would be appreciated :)

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> For the a11y test, I will wait for Alexander and Marco's comments.

Sorry, what's the question?

Comment 31

[Mounir Lamouri (:mounir)]

See David's comment (comment 26).

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> See David's comment (comment 26).

extras directory sounds good if you ask about this

Comment 33

[(no longer active)]

Comment on attachment 440217 [details] [diff] [review]
Patch v3

(I'm not officially a layout peer; I'm just reviewing this because I'm familiar with this code.  I'll let Boris overrule my review if necessary.)

>diff -r 4d60fe5e25f3 content/html/content/src/nsHTMLInputElement.cpp
>+PRBool
>+nsHTMLInputElement::DoesPlaceholderApply() const
>+{
>+  switch (mType)
>+  {
>+    case NS_FORM_INPUT_TEXT:
>+    case NS_FORM_INPUT_SEARCH:
>+    case NS_FORM_INPUT_EMAIL:
>+    case NS_FORM_INPUT_URL:
>+    case NS_FORM_INPUT_TEL:
>+    case NS_FORM_INPUT_PASSWORD:
>+      return PR_TRUE;
>+#ifdef DEBUG
>+    case NS_FORM_INPUT_HIDDEN:
>+    case NS_FORM_INPUT_CHECKBOX:
>+    case NS_FORM_INPUT_RADIO:
>+    case NS_FORM_INPUT_FILE:
>+    case NS_FORM_INPUT_SUBMIT:
>+    case NS_FORM_INPUT_IMAGE:
>+    case NS_FORM_INPUT_RESET:
>+    case NS_FORM_INPUT_BUTTON:
>+      return PR_FALSE;
>+    default:
>+      NS_NOTYETIMPLEMENTED("Unexpected input type in DoesPlaceholderApply()");
>+      return PR_FALSE;
>+#else // DEBUG
>+    default:
>+      return PR_FALSE;
>+#endif // DEBUG

To make the code a bit more readable, you could do something like this:

    case NS_FORM_INPUT_HIDDEN:
    case NS_FORM_INPUT_CHECKBOX:
    case NS_FORM_INPUT_RADIO:
    case NS_FORM_INPUT_FILE:
    case NS_FORM_INPUT_SUBMIT:
    case NS_FORM_INPUT_IMAGE:
    case NS_FORM_INPUT_RESET:
    case NS_FORM_INPUT_BUTTON:
      return PR_FALSE;
    default:
      NS_NOTYETIMPLEMENTED("Unexpected input type in DoesPlaceholderApply()");
      return PR_FALSE;

You don't need to guard the code using #ifdef DEBUG, because NS_NOTYETIMPLEMENTED is preprocessed out in non-debug builds.

>@@ -1884,42 +1894,46 @@ nsTextControlFrame::IsLeaf() const
> {
>   return PR_TRUE;
> }
> 
> //IMPLEMENTING NS_IFORMCONTROLFRAME
> void nsTextControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
> {
>   if (!aOn) {
>-    nsWeakFrame weakFrame(this);
>-
>-    nsAutoString valueString;
>-    GetValue(valueString, PR_TRUE);
>-    if (valueString.IsEmpty())
>-      ShowPlaceholder();
>-
>-    if (!weakFrame.IsAlive())
>-    {
>-      return;
>+    if (IsPlaceholderUsed()) {
>+      nsAutoString valueString;
>+
>+      GetValue(valueString, PR_TRUE);

You could use GetTextLength here instead of getting the value.  Currently they both end up doing the same thing, but some day someone might make GetTextLength more efficient, and then this code would also benefit from it.

>@@ -2908,25 +2913,32 @@ nsTextControlFrame::IsScrollable() const
> {
>   return !IsSingleLineTextControl();
> }
> 
> void
> nsTextControlFrame::SetValueChanged(PRBool aValueChanged)
> {
>   // placeholder management
>-  if (!IsFocusedContent(mContent)) {
>+  if (IsPlaceholderUsed() && !IsFocusedContent(mContent)) {
>+    nsWeakFrame weakFrame(this);
>+
>     // If the content is focused, we don't care about the changes because
>     // the placeholder is going to be hide/show on blur.
>     nsAutoString valueString;
>     GetValue(valueString, PR_TRUE);

Ditto here.

>diff -r 4d60fe5e25f3 layout/reftests/forms/placeholder/placeholder-14.html
>--- a/layout/reftests/forms/placeholder/placeholder-14.html	Tue Apr 20 13:57:36 2010 +0200
>+++ b/layout/reftests/forms/placeholder/placeholder-14.html	Tue Apr 20 16:06:55 2010 +0200
>@@ -7,11 +7,11 @@
>       document.getElementById('p1').placeholder = 'my\n placeholder';
>     }
>     function disableReftestWait()
>     {
>       document.documentElement.className = '';
>     }
>   </script>
>   <body onload="setPlaceholder(); disableReftestWait();">
>-    <input type="text" id="p1" value="" placeholder="">
>+    <input type="text" id="p1" value="">

Why is this change necessary now?  Actually I think we need tests for both cases (where there's no placeholder attribute, and where it's empty.

>diff -r 4d60fe5e25f3 layout/reftests/forms/placeholder/placeholder-2.html
>--- a/layout/reftests/forms/placeholder/placeholder-2.html	Tue Apr 20 13:57:36 2010 +0200
>+++ b/layout/reftests/forms/placeholder/placeholder-2.html	Tue Apr 20 16:06:55 2010 +0200
>@@ -8,11 +8,11 @@
>     }
>     function disableReftestWait()
>     {
>       document.documentElement.className = '';
>     }
>   </script>
> 
>   <body onload="setPlaceholder(); disableReftestWait();">
>-    <input type="text" id="p1" value="" placeholder="">
>+    <input type="text" id="p1" value="">

Ditto here.

r=me pending the tests inside accessibility, and with the above comments addressed.

Comment 34

[Mounir Lamouri (:mounir)]

Results from the tinderbox:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271773586.1271783456.22060.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271773586.1271782791.18927.gz

I forgot to save the MacOS result but there wasn't more errors than Windows and GNU/Linux ones.

I will try to see how to make these tests.

Comment 35

[Mounir Lamouri (:mounir)]

Attachment: Patch v4

I do not understand why chrome://mochikit/content/browser/browser/base/content/test/browser_bug321000.js is failing. I mean, there is no direct link between this patch and line break stripping. I suppose there is a side effect I do not see.

In addition, placeholder-8.html is always failing and placeholder-7.html is randomly failing. Both because of caret. placeholder-7.html is randomly failing because the capture may be done before the caret is shown (and the reference has it shown) and placeholder-8.html is always failing because of the caret too. Actually, I'm not sure about this one. I've seen when reloading it, the caret appears only when I release CRTL+SHIFT+R. So maybe the reftests has the same issue for another reason. It should be interesting to remove carets for these tests.

Any help would be appreciated.

Comment 36

[(no longer active)]

(In reply to comment #35)
> Created an attachment (id=443102) [details]
> Patch v4
> 
> I do not understand why
> chrome://mochikit/content/browser/browser/base/content/test/browser_bug321000.js
> is failing. I mean, there is no direct link between this patch and line break
> stripping. I suppose there is a side effect I do not see.

I have an idea on what's happening here.

Your patch is breaking this code: <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#132>.  Note that when that code retrieves the editor property, the editor will be initialized if it's not around.  Because currently the editor object is attached to the frame, when the frame is reconstructed, the editor will be destroyed as well, and will be initialized as needed.  What I think is happening here is that the XBL constructor sets the line handling pref of the editor, then setting the emptytext property on the XUL textbox causes a reframe of the underlying input control, which recreates the editor, so now the old line handling value is lost.

I think the correct thing to do here is to fix this on top of bug 534785.  The editor will be attached to the content node in that bug, so reframing won't affect it.

> In addition, placeholder-8.html is always failing and placeholder-7.html is
> randomly failing. Both because of caret. placeholder-7.html is randomly failing
> because the capture may be done before the caret is shown (and the reference
> has it shown) and placeholder-8.html is always failing because of the caret
> too. Actually, I'm not sure about this one. I've seen when reloading it, the
> caret appears only when I release CRTL+SHIFT+R. So maybe the reftests has the
> same issue for another reason. It should be interesting to remove carets for
> these tests.

What I think is happening in placeholder-8.html is that the focus call initializes the editor, and setting the placeholder will cause a reframe which recreates the frame without the editor being initialized.  Actually, this will break more than just the caret - the editor won't be usable (i.e., users can't type into this box, etc.)  I think a similar thing is happening in placeholder-7.html as well.

Ultimately, I think taking this patch without bug 534785 will break text controls in more than one way.  We should really make sure that the editor survives a reframe.

On an unrelated note, why is this depending on bug 557620?

Comment 37

[Mounir Lamouri (:mounir)]

(In reply to comment #36)
> Ultimately, I think taking this patch without bug 534785 will break text
> controls in more than one way.  We should really make sure that the editor
> survives a reframe.

Looks like we seriously need bug 534785 :)

> On an unrelated note, why is this depending on bug 557620?

bug 557620 introduces |IsSingleLineTextControl| method which I'm using in this patch. That's not vital but I don't think this bug is going to be fixed before bug 557620. Actually, you've added this dependency, see comment 10 ;)

Comment 38

[(no longer active)]

(In reply to comment #37)
> > On an unrelated note, why is this depending on bug 557620?
> 
> bug 557620 introduces |IsSingleLineTextControl| method which I'm using in this
> patch. That's not vital but I don't think this bug is going to be fixed before
> bug 557620. Actually, you've added this dependency, see comment 10 ;)

Heh, that's embarrassing!

/me whistles and pretends that this didn't happen

Comment 39

[Mounir Lamouri (:mounir)]

Attachment: Patch - WIP

So, I've updated the patch and it looks like there is an interesting issue when we change @placeholder in an onfocus event handler (changing @placeholder recreates the frame so I assume it's the cause of the problem).

Comment 40

[Mounir Lamouri (:mounir)]

By the way, now, the frame doesn't have |mPlaceholderDiv| anymore (it's in nsTextEditorState) so the frame guess if it needs to manage placeholder by checking the placeholder content attribute. So, an empty placeholder will create an anonymous div. It was requested to not do that in comment 15 but it sounded to be the more elegant solution.
It would be possible to check if the placeholder is different from the empty string but it would force us to do the check in nsTextControlFrame (to not request the placeholder frame creation in that case) and to assume calls to placeholder related methods in nsTextEditorState with mPlaceholderDiv=nsnull are expected (I've added asserts in this patch which is more robust I think). Actually, the worst part is how GetPlaceholder() works. It is creating a placeholder div if |mPlaceholderDiv| is null. So, it will force us to check for the placeholder string at every GetPlaceholder call which wouldn't be very good for performance I think.

Ehsan, would you have any feedback?

Comment 41

[Boris Zbarsky [:bzbarsky]]

> changing @placeholder recreates the frame

But asynchronously, right?  I suppose that might still make us lose focus...

Comment 42

[Mounir Lamouri (:mounir)]

(In reply to comment #41)
> > changing @placeholder recreates the frame
> 
> But asynchronously, right?  I suppose that might still make us lose focus...

Yes, I guess that's asynchronous. Actually, for a11y tests checking how many anonymous children we have, it forces me to use dirty setTimeout that may make Ehsan angry ;)

Comment 43

[Boris Zbarsky [:bzbarsky]]

Er... why do you need setTimeout?

Comment 44

[Mounir Lamouri (:mounir)]

(In reply to comment #43)
> Er... why do you need setTimeout?

In accessible/tests/mochitest/layout/test_bug553097.html
+  // Check dynamic change.
+  // TODO: avoid setTimeout ?
+  // TODO: do the same for <textarea>
+  var input = document.getElementById('i');
+  input.placeholder = "foo";
+  setTimeout(function() {
+    checkPlaceholderDiv(getAccessible(input), true);
+
+    input.removeAttribute('placeholder');
+    setTimeout(function() {
+      checkPlaceholderDiv(getAccessible(input), false);
+      SimpleTest.finish();
+    }, 500);
+
+  }, 500);

If I do the check just after the addition/removal of the placeholder attribute, the number of children isn't correct. I guess it is because I have to wait for the frame re-construction, isn't it?

Comment 45

[Boris Zbarsky [:bzbarsky]]

Ah, I bet the accessibility code should be flushing and isn't....

Comment 46

[Boris Zbarsky [:bzbarsky]]

You could work around by flushing yourself, btw.

Comment 47

[Mounir Lamouri (:mounir)]

(In reply to comment #46)
> You could work around by flushing yourself, btw.

Which component can I use to flush?

Comment 48

[Boris Zbarsky [:bzbarsky]]

  document.documentElement.getBoundingClientRect();

should do it.

Comment 49

[(no longer active)]

(In reply to comment #40)
> By the way, now, the frame doesn't have |mPlaceholderDiv| anymore (it's in
> nsTextEditorState) so the frame guess if it needs to manage placeholder by
> checking the placeholder content attribute. So, an empty placeholder will
> create an anonymous div. It was requested to not do that in comment 15 but it
> sounded to be the more elegant solution.
> It would be possible to check if the placeholder is different from the empty
> string but it would force us to do the check in nsTextControlFrame (to not
> request the placeholder frame creation in that case) and to assume calls to
> placeholder related methods in nsTextEditorState with mPlaceholderDiv=nsnull
> are expected (I've added asserts in this patch which is more robust I think).
> Actually, the worst part is how GetPlaceholder() works. It is creating a
> placeholder div if |mPlaceholderDiv| is null. So, it will force us to check for
> the placeholder string at every GetPlaceholder call which wouldn't be very good
> for performance I think.
> 
> Ehsan, would you have any feedback?

You can maintain a boolean flag indicating whether the placeholder attribute exists and is non-empty, right?  That boolean flag only needs to get updated when the placeholder attribute changes.

Comment 50

[Mounir Lamouri (:mounir)]

Attachment: Patch v5

This patch should work correctly except for the issue mentioned in bug 585463. @placeholder changes re-creating the frame, the focus doesn't behave correctly and some reftests do not pass because of that.

I guess we can have this patch reviewed and wait for bug 585463 to land it.

Comment 51

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

Having nsITextControlElement::GetPlaceholderNode create the node seems a bit fragile. Wouldn't it be better to have a separate Create method, call that during CreateAnonyousContent, and have GetPlaceholderNode just return null if there's no node?

+  if (mUsePlaceholder && !aBeforeEditorInit &&
+      !nsContentUtils::IsFocusedContent(mContent))

Why is this call to IsFocusedContent needed?

Comment 52

[Mounir Lamouri (:mounir)]

(In reply to comment #51)
> Having nsITextControlElement::GetPlaceholderNode create the node seems a bit
> fragile. Wouldn't it be better to have a separate Create method, call that
> during CreateAnonyousContent, and have GetPlaceholderNode just return null if
> there's no node?

I agree but I tried to keep Ehsan's design. Ehsan, do you agree with adding a CreatePlaceholderNode() in nsITextControlElement or maybe a GetPlaceholderNode(PRBool aCreateIfNeeded) method instead of the current one?

> +  if (mUsePlaceholder && !aBeforeEditorInit &&
> +      !nsContentUtils::IsFocusedContent(mContent))
> 
> Why is this call to IsFocusedContent needed?

I would suspect this come for my merge with the old patch and it doesn't seem needed.

Comment 53

[(no longer active)]

(In reply to comment #52)
> (In reply to comment #51)
> > Having nsITextControlElement::GetPlaceholderNode create the node seems a bit
> > fragile. Wouldn't it be better to have a separate Create method, call that
> > during CreateAnonyousContent, and have GetPlaceholderNode just return null if
> > there's no node?
> 
> I agree but I tried to keep Ehsan's design. Ehsan, do you agree with adding a
> CreatePlaceholderNode() in nsITextControlElement or maybe a
> GetPlaceholderNode(PRBool aCreateIfNeeded) method instead of the current one?

Yes, it should be possible to separate this into a CreatePlaceholderNode.  I didn't do that because I don't see the advantage of yet another API on nsITextControlElement, but if this can be useful, go for it!

Comment 54

[Mounir Lamouri (:mounir)]

Attachment: Patch v5.1

Comment 55

[(no longer active)]

Comment on attachment 464049 [details] [diff] [review]
Patch v5.1

>+#ifdef DEBUG
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
>+  if (content) {
>+    nsAutoString placeholderTxt;
>+    content->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder,
>+                     placeholderTxt);
>+    nsContentUtils::RemoveNewlines(placeholderTxt);
>+    NS_ASSERTION(!placeholderTxt.IsEmpty(), "CreatePlaceholderNode() shouldn't \
>+be called if @placeholder is the empty string when trimmed from line breaks");
>+  }
>+#endif // DEBUG

Please move this into its own block, so that if someone adds a |content| variable at the function scope later on, their build doesn't break weirdly in debug mode.

>-asserts(2) asserts-if(gtk2Widget,12) load 323386-1.html # Bug 575011
>+asserts(2) asserts-if(gtk2Widget,8) load 323386-1.html # Bug 575011

Which assertions are you fixing here?

Comment 56

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

Comment on attachment 464049 [details] [diff] [review]
Patch v5.1

Thanks!

Comment 57

[Mounir Lamouri (:mounir)]

(In reply to comment #55)
> Comment on attachment 464049 [details] [diff] [review]
> Patch v5.1
> 
> >+#ifdef DEBUG
> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
> >+  if (content) {
> >+    nsAutoString placeholderTxt;
> >+    content->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder,
> >+                     placeholderTxt);
> >+    nsContentUtils::RemoveNewlines(placeholderTxt);
> >+    NS_ASSERTION(!placeholderTxt.IsEmpty(), "CreatePlaceholderNode() shouldn't \
> >+be called if @placeholder is the empty string when trimmed from line breaks");
> >+  }
> >+#endif // DEBUG
> 
> Please move this into its own block, so that if someone adds a |content|
> variable at the function scope later on, their build doesn't break weirdly in
> debug mode.

No problem, I will update the patch.

> >-asserts(2) asserts-if(gtk2Widget,12) load 323386-1.html # Bug 575011
> >+asserts(2) asserts-if(gtk2Widget,8) load 323386-1.html # Bug 575011
> 
> Which assertions are you fixing here?

It's some layout-related assertions. With the placeholder node not present, some assertions do not appear anymore. I'm not really fixing anything, we just have less asserting stuff ;)

Comment 58

[Mounir Lamouri (:mounir)]

Attachment: Patch v5.2

Comment 59

[(no longer active)]

Comment on attachment 464214 [details] [diff] [review]
Patch v5.2

I still don't feel confident that the assertion count change is correct (it's hard to judge whether intermittent assertions count has really changed), but let's not stall this patch any more!  :-)

Comment 60

[Mounir Lamouri (:mounir)]

Thanks for the review :)
But unfortunately this patch can't be landed as long as bug 536421 isn't fixed.

Comment 61

[Mounir Lamouri (:mounir)]

Attachment: Patch v5.3

Updated patch.

Unfortunately, reftests are failing with this patch even with the blocking bug fixed, see for example:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286825508.1286826374.5251.gz#err0
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286816815.1286817425.1435.gz#err0

When I load the reftests locally, I do not see the entire caret but only part of it. Though, this doesn't explain why the placeholder text is shown (and shown like a regular text) in these results.

Any idea?

Comment 62

[(no longer active)]

Looks to me like an instance of bug 602130...

Comment 63

[Mounir Lamouri (:mounir)]

Indeed, that sounds to be linked to bug 602130.

Comment 64

[Mounir Lamouri (:mounir)]

With patch from bug 602130 applied, the caret is fixed but I still see the placeholder text without the placeholder style (that's because -moz-placeholder do not apply) while the input is focused in the reftests. However, when I launch the reftest in a page, the placeholder disappears so I guess the placeholder div isn't hidden immediatly. I will look at that later but if anyone has an idea on top of his head...

Comment 65

[(no longer active)]

I may be wrong, but I highly suspect that this is because we only hide/show the placeholder if aBeforeEditorInit is false <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#1437>.

This was done in <http://hg.mozilla.org/mozilla-central/rev/35aa2569f5b7>, and for good reason, because we used to take care of hiding the placeholder in EnsureEditorIsInitializedInternal <http://hg.mozilla.org/mozilla-central/file/35aa2569f5b7/layout/forms/nsTextControlFrame.cpp#l1587> after the editor was initialized.

But now that the editor is moved to the content node, I can't seem to find an equivalent code which does that.

Here's what I suggest you do in order to debug this.  First try to comment out this line and see if it fixes the placeholder display problem: <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#1434>.  If it does, set a conditional breakpoint on nsTextControlFrame::UpdateValueDisplay with the condition aBeforeEditorInit!=0, and when it gets hit, set another (non-conditional) breakpoint on SetPlaceholderClass to see if we ever call it after nsEditor::Init is called.  If we don't, then I think the correct fix is to remove the if condition, as the premise based on which I added it no longer holds true.

Comment 66

[Mounir Lamouri (:mounir)]

This looks to be working on tip now... At least, locally. I sent the patch on try to be sure. In addition, a had to fix a test which was failing: I don't understand how it was working before <textarea cols='5'>my placeholder</textarea> should have a scrollbar but not <textarea cols='5' placeholder='my placeholder'></textarea>.

Comment 67

[Mounir Lamouri (:mounir)]

Indeed, this is now fixed.

But since bug 604386 has landed, the tests I wrote in accessible/layout/ doesn't work anymore (placeholder div is not in .childCount). I don't see another way to test that :-/

Comment 68

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

If we can't test it, we can't. But we should still land it!

Comment 69

[(no longer active)]

(In reply to comment #67)
> But since bug 604386 has landed, the tests I wrote in accessible/layout/
> doesn't work anymore (placeholder div is not in .childCount). I don't see
> another way to test that :-/

Maybe David or Surkov know?

Comment 70

[alexander :surkov (:asurkov)]

(In reply to comment #69)
> (In reply to comment #67)
> > But since bug 604386 has landed, the tests I wrote in accessible/layout/
> > doesn't work anymore (placeholder div is not in .childCount). I don't see
> > another way to test that :-/
> 
> Maybe David or Surkov know?

Right, we do not expose accessibles for text nodes created for placeholder.

You could try a trick, get an accessible for text node created for textbox value, and try to get placeholder text nodes from DOM like

var valueNode = textboxAcc.firstChild.DOMNode;
var divRootNode = valueNode.parentNode;
var divPlaceholderNode = divRootNode.nextSibling;

if it works.

Comment 71

[Mounir Lamouri (:mounir)]

Your trick didn't work Alexander :(
I've open bug 605465 for this test.

This is now pushed:
http://hg.mozilla.org/mozilla-central/rev/ed1eff4526ce

Comment 72

[alexander :surkov (:asurkov)]

(In reply to comment #71)
> Your trick didn't work Alexander :(

It's not allowed to traverse native anonymous content? Just I might be wrong in native anonymous DOM tree structure I provided in example.

Comment 73

[Mounir Lamouri (:mounir)]

(In reply to comment #72)
> (In reply to comment #71)
> > Your trick didn't work Alexander :(
> 
> It's not allowed to traverse native anonymous content? Just I might be wrong in
> native anonymous DOM tree structure I provided in example.

Actually, textboxAcc.firstChild.DOMNode returns null.

Comment 74

[alexander :surkov (:asurkov)]

(In reply to comment #73)
> (In reply to comment #72)
> > (In reply to comment #71)
> > > Your trick didn't work Alexander :(
> > 
> > It's not allowed to traverse native anonymous content? Just I might be wrong in
> > native anonymous DOM tree structure I provided in example.
> 
> Actually, textboxAcc.firstChild.DOMNode returns null.

hm, if textboxAcc.firstChild is not null then textboxAcc.firstChild.DOMNode shouldn't be null. If it's undefined (not null) then it means you should query nsIAccessNode.