221150 - text edit fields should default to white-space: nowrap

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Ivan Eisen]

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

Comment 1

[Ivan Eisen]

Attachment: 3292 input fields

I would recommend saving this file to local disk when testing to avoid network 

fluctuations..

Comment 2

[Ivan Eisen]

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.  
   

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that discussion in bug 148636 led to the filing of this bug.

Comment 4

[Ivan Eisen]

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     } 

Comment 5

[Alec Flett]

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...

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ivan?  ping?

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1

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?

Comment 9

[Boris Zbarsky [: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 10

[Peter Van der Beken [:peterv]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm ok with either one, preferring the former (because it's faster).

brade, Daniel, would you be ok with that API change?

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 14

[Thomas Bertels]

What about asking for a review ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to my comments, merged to tip

Comment 17

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

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).

Comment 18

[Boris Zbarsky [:bzbarsky]]

If setWrapWidth were named something else, yes.  Want me to do that?

Comment 19

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

I think it's worth doing, yeah.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With the different function

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in.