Closed Bug 221150 Opened 18 years ago Closed 14 years ago

text edit fields should default to white-space: nowrap

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ire0, Assigned: peterv)

References

Details

(Keywords: perf)

Attachments

(2 files, 4 obsolete files)

I found a line of code that is costing about 14% of the time in bringing up forms
with lots of input fields.  I commented out the call to set the text editor
created for each input field to nowrap ("white-space: pre").  The code I am
talking about  is in layout/html/forms/src/nsTextControlFrame.cpp.  I marked it
with //ivan

 // Set up wrapping
1844     if (IsTextArea()) {
1845       // wrap=off means -1 for wrap width no matter what cols is
1846       nsFormControlHelper::nsHTMLTextWrap wrapProp;
1847       nsFormControlHelper::GetWrapPropertyEnum(mContent, wrapProp);
1848       if (wrapProp == nsFormControlHelper::eHTMLTextWrap_Off) {
1849         // do not wrap when wrap=off
1850   //ivan      textEditor->SetWrapWidth(-1);
1851       } else {
1852         // Set wrapping normally otherwise
1853         textEditor->SetWrapWidth(GetCols());
1854       }
1855     } else {
1856       // Never wrap non-textareas
1857  //ivan    textEditor->SetWrapWidth(-1);
1858     }

The SetWrapWidth(-1) turns off wrapping ("white-space: pre").  I believe the
default for wrapping is inherit.  I assume that should be nowrap.  I tested
after commenting out the 2 lines above and my tests ran 20% faster.

Note,  I initially though this fix would save over 20%;  however,  I was fooled
because the time goes up dramatically the second time you run any test with lots
of input fields (this is something that may be worth exploring).  I compared
the original code the second time executed with the fixed code the first time,
thus  exaggerating the benefit.

Ivan
Hardware: PC → All
Attached file 3292 input fields
I would recommend saving this file to local disk when testing to avoid network 

fluctuations..
Results of my testing on a 660 Mhz 512mb Linux Redhat system running an
optimized moz 1.4.

  old code -> first time 17.73 secs  second time 19.81 secs.
  new code ->            15.43 secs              17.09 secs.  
   
Note that discussion in bug 148636 led to the filing of this bug.
I believe nowrap is both correct and faster for editing an input field.
I don't think leaving out the nowrap setting will hurt anything. If an
input field fails the check to text area than it can't wrap so the setting is
meaningless.  I believe it is a single line text control.
  I made a mistake in my first try for a fix.  Only the second
"textEditor->SetWrapWidth(-1);" should have been commented out (see below).

 // Set up wrapping
1844     if (IsTextArea()) {
1845       // wrap=off means -1 for wrap width no matter what cols is
1846       nsFormControlHelper::nsHTMLTextWrap wrapProp;
1847       nsFormControlHelper::GetWrapPropertyEnum(mContent, wrapProp);
1848       if (wrapProp == nsFormControlHelper::eHTMLTextWrap_Off) {
1849         // do not wrap when wrap=off
1850         textEditor->SetWrapWidth(-1);
1851       } else {
1852         // Set wrapping normally otherwise
1853         textEditor->SetWrapWidth(GetCols());
1854       }
1855     } else {
1856       // Never wrap non-textareas
1857  //ivan    textEditor->SetWrapWidth(-1);
1858     } 
Blocks: 148636
I don't know much about how the textEditor objects are created, but we should
look into either:
1) lazily deciding on a text-wrapping style, or coming up with some way to avoid
making the decision until its actually needed (because more than likely its not
needed BEFORE this function is called)
2) specifying the default text-wrapping on object creation, so that instead of
saying
  a) create text object
  b) set wrap mode
  we say
  a) create text object with the specified wrap mode

Anyway, like I said I don't know exactly how this works, or where the quoted
code is being exercised...
Keywords: perf
So... we already set "white-space: nowrap" on that div at content creation time
(see
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFrame.cpp#1626)

I suspect the "white-space: nowrap" can be replaced with "white-space: pre"
there and then the call to SetWrapWidth() in the "Never wrap non-textareas" case
can be skipped entirely (if I read SetWrapWidth correctly).

Ivan, could you try that and see what effect it has?
Ivan?  ping?
Attached patch v1 (obsolete) — Splinter Review
This does what Boris suggested in comment 6. My measurements of load times show this:

Without patch:
2445 2458 2445 2454 2477  Average: 2455.8

With patch
1999 2026 2074 2038 2038  Average: 2035

So about 15-20% improvement.

Boris: I tried some simple testcases with <textarea> and <input type=text> and saw nothing odd. Any suggestions for additional testcases?
Assignee: layout.form-controls → peterv
Status: NEW → ASSIGNED
Attachment #220322 - Flags: superreview?(bzbarsky)
Attachment #220322 - Flags: review?(bzbarsky)
Well.. It looks like this will leave the editor in an inconsistent state, no?  It'll have mWrapColumn at 0, but the style will not match.  If nothing else, that affects serialization and such.

Any caller of GetWrapWidth should be tested:

  http://lxr.mozilla.org/seamonkey/search?string=.wrapWidth

and

  http://lxr.mozilla.org/seamonkey/search?string=GetWrapWidth%28
Comment on attachment 220322 [details] [diff] [review]
v1

Yeah, mWrapColumn being out-of-sync is wrong. It seems we should either allow the caller to say whether it wants the style-munging to happen, or we should not do it if the computed styles are already ok.
Attachment #220322 - Flags: superreview?(bzbarsky)
Attachment #220322 - Flags: superreview-
Attachment #220322 - Flags: review?(bzbarsky)
Attachment #220322 - Flags: review-
I'm ok with either one, preferring the former (because it's faster).

brade, Daniel, would you be ok with that API change?
Attached patch v2 (obsolete) — Splinter Review
Attachment #220322 - Attachment is obsolete: true
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #221821 - Attachment is obsolete: true
What about asking for a review ?
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Comment on attachment 241885 [details] [diff] [review]
v2.1

>Index: editor/idl/nsIPlaintextEditor.idl
>Index: mail/components/compose/content/MsgComposeCommands.js
>+      gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail)
>+                        .wrapWidth(gMsgCompose.wrapLength, true);

That should be a setWrapWidth.

This is missing the forms.css change to use "white-space: pre".

With those changes, r=bzbarsky.
Attachment #241885 - Flags: review+
Attachment #241885 - Attachment is obsolete: true
Attachment #273534 - Flags: superreview?(roc)
This potentially breaks some XUL apps. Seems like it wouldn't be hard to preserve apps that set wrapWidth directly; keep wrapWidth writable, and make setting it call setWrapWidth(..., PR_TRUE).
If setWrapWidth were named something else, yes.  Want me to do that?
I think it's worth doing, yeah.
Attachment #273534 - Attachment is obsolete: true
Attachment #273604 - Flags: superreview?(roc)
Attachment #273534 - Flags: superreview?(roc)
Attachment #273604 - Flags: superreview?(roc)
Attachment #273604 - Flags: superreview+
Attachment #273604 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 398942
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
You need to log in before you can comment on or make changes to this bug.