Placeholder code creates unnecessary frames and DOM nodes

RESOLVED FIXED in mozilla2.0b7

Status

()

P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: roc, Assigned: mounir)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 8 obsolete attachments)

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?

Updated

9 years ago
Blocks: 457800
No longer depends on: 457800

Updated

9 years ago
Blocks: 554763
(Assignee)

Comment 1

9 years ago
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.
No. It should be created in CreateAnonymousCOntent if the 'placeholder' attribute is present.

Updated

9 years ago
Blocks: 473178
(Assignee)

Comment 3

9 years ago
(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.
I think what is being suggested is that addition/removal of the placeholder attribute should cause the frame for the input to be reconstructed.
Right. You could do this by calling nsCSSFrameConstructor::PostRestyleEvent in an AttributeChanged callback.
(Assignee)

Updated

9 years ago
Assignee: nobody → mounir.lamouri
Or better yet, by adding the placeholder attribute to nsHTMLInputElement::GetAttributeChangeHint (and similar for textarea), returning NS_STYLE_HINT_FRAMECHANGE.
(Assignee)

Comment 7

9 years ago
Created attachment 439522 [details] [diff] [review]
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.
Attachment #439522 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 years ago
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

9 years ago
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.
Attachment #439522 - Flags: review?(roc)

Comment 10

9 years ago
(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.
Depends on: 557620

Updated

9 years ago
Keywords: perf
> I used GetAttributeChangedHint 

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

Comment 12

9 years ago
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 ?
> 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.  ;)
(Assignee)

Comment 14

9 years ago
Created attachment 439689 [details] [diff] [review]
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.
Attachment #439522 - Attachment is obsolete: true
Attachment #439689 - Flags: review?(bzbarsky)

Comment 15

9 years ago
(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.
(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

9 years ago
(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?
(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);
(Assignee)

Comment 19

9 years ago
(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.
(Assignee)

Comment 20

9 years ago
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

9 years ago
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).
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

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

Comment 24

9 years ago
Also, Mounir, this probably breaks some of the accessibility tests, have you tested this on the try server?
(Assignee)

Comment 25

9 years ago
(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.
(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

9 years ago
(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.
Whiteboard: [needs new patch]
(Assignee)

Comment 28

9 years ago
Created attachment 440217 [details] [diff] [review]
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.
Attachment #439689 - Attachment is obsolete: true
Attachment #440217 - Flags: review?(bzbarsky)
Attachment #439689 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch]
(Assignee)

Updated

9 years ago
Attachment #440217 - Flags: review?(ehsan)
(Assignee)

Comment 29

9 years ago
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 :)
(In reply to comment #28)

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

Sorry, what's the question?
(Assignee)

Comment 31

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

extras directory sounds good if you ask about this

Updated

9 years ago
Attachment #440217 - Flags: review?(ehsan) → review+

Comment 33

9 years ago
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.
blocking2.0: ? → final+
Priority: -- → P2
(Assignee)

Comment 34

9 years ago
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.
(Assignee)

Comment 35

9 years ago
Created attachment 443102 [details] [diff] [review]
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.
Attachment #440217 - Attachment is obsolete: true
Attachment #440217 - Flags: review?(bzbarsky)

Comment 36

9 years ago
(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?
Depends on: 534785
(Assignee)

Comment 37

9 years ago
(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

9 years ago
(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
(Assignee)

Updated

8 years ago
No longer blocks: 554763
(Assignee)

Comment 39

8 years ago
Created attachment 463537 [details] [diff] [review]
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).
Attachment #443102 - Attachment is obsolete: true
(Assignee)

Comment 40

8 years ago
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?
> changing @placeholder recreates the frame

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

Comment 42

8 years ago
(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 ;)
Er... why do you need setTimeout?
(Assignee)

Comment 44

8 years ago
(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?
Ah, I bet the accessibility code should be flushing and isn't....
You could work around by flushing yourself, btw.
(Assignee)

Comment 47

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

Which component can I use to flush?
  document.documentElement.getBoundingClientRect();

should do it.

Comment 49

8 years ago
(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.
(Assignee)

Updated

8 years ago
Depends on: 585463
(Assignee)

Comment 50

8 years ago
Created attachment 463956 [details] [diff] [review]
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.
Attachment #463537 - Attachment is obsolete: true
Attachment #463956 - Flags: review?(ehsan)
(Assignee)

Updated

8 years ago
Attachment #463956 - Flags: review?(roc)
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?
(Assignee)

Comment 52

8 years ago
(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

8 years ago
(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!

Updated

8 years ago
No longer depends on: 585463

Updated

8 years ago
Depends on: 536421
(Assignee)

Comment 54

8 years ago
Created attachment 464049 [details] [diff] [review]
Patch v5.1
Attachment #463956 - Attachment is obsolete: true
Attachment #464049 - Flags: review?(roc)
Attachment #463956 - Flags: review?(roc)
Attachment #463956 - Flags: review?(ehsan)
(Assignee)

Updated

8 years ago
Attachment #464049 - Flags: review?(ehsan)

Comment 55

8 years ago
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 on attachment 464049 [details] [diff] [review]
Patch v5.1

Thanks!
Attachment #464049 - Flags: review?(roc) → review+
(Assignee)

Comment 57

8 years ago
(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 ;)
(Assignee)

Comment 58

8 years ago
Created attachment 464214 [details] [diff] [review]
Patch v5.2
Attachment #464049 - Attachment is obsolete: true
Attachment #464214 - Flags: review?(ehsan)
Attachment #464049 - Flags: review?(ehsan)

Comment 59

8 years ago
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!  :-)
Attachment #464214 - Flags: review?(ehsan) → review+
(Assignee)

Comment 60

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

Updated

8 years ago
No longer blocks: 457801
(Assignee)

Comment 61

8 years ago
Created attachment 482418 [details] [diff] [review]
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?
Attachment #464214 - Attachment is obsolete: true

Comment 62

8 years ago
Looks to me like an instance of bug 602130...
(Assignee)

Comment 63

8 years ago
Indeed, that sounds to be linked to bug 602130.
Depends on: 602130

Updated

8 years ago
No longer depends on: 602130

Updated

8 years ago
Depends on: 602130
(Assignee)

Comment 64

8 years ago
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

8 years ago
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.
(Assignee)

Comment 66

8 years ago
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>.
(Assignee)

Comment 67

8 years ago
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 :-/
If we can't test it, we can't. But we should still land it!

Comment 69

8 years ago
(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?
(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.
(Assignee)

Updated

8 years ago
Depends on: 605465
(Assignee)

Comment 71

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(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.
(Assignee)

Comment 73

8 years ago
(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.
(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.
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.