text edit fields should default to white-space: nowrap

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Ivan Eisen, Assigned: peterv)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

14 years ago
Hardware: PC → All
(Reporter)

Comment 1

14 years ago
Created attachment 132599 [details]
3292 input fields

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

fluctuations..
(Reporter)

Comment 2

14 years ago
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.
(Reporter)

Comment 4

14 years ago
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     } 

Updated

14 years ago
Blocks: 148636

Comment 5

14 years ago
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...
(Reporter)

Updated

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

Comment 8

12 years ago
Created attachment 220322 [details] [diff] [review]
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?
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
(Assignee)

Comment 10

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

Comment 12

12 years ago
Created attachment 221821 [details] [diff] [review]
v2
Attachment #220322 - Attachment is obsolete: true
(Assignee)

Comment 13

11 years ago
Created attachment 241885 [details] [diff] [review]
v2.1
Attachment #221821 - Attachment is obsolete: true

Comment 14

11 years ago
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+
Created attachment 273534 [details] [diff] [review]
Updated to my comments, merged to tip
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.
Created attachment 273604 [details] [diff] [review]
With the different function
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
Last Resolved: 11 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.