[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    [PATCH] Please review --- multiple textarea fixes (#141457,
From:       Maksim Orlovich <mo85 () cornell ! edu>
Date:       2007-06-09 19:02:25
Message-ID: 200706091502.26013.mo85 () cornell ! edu
[Download RAW message or body]

Hi.. Please take a look at the attached patch, which fixes a number of 
problems with syncing between the renderer and the DOM (including one bug 
that made deleting selection in wordpress impossible, and the losing-value on 
re-display bug), and along the way adds the innerText support Stefan Eilers 
asked for. 

The main idea of the changes is a follows:
1. Simplify the syncing logic by making the renderer always be definitive when 
it exists.

2. Change how we  initialize from the defaultValue --- instead of doing it in 
the renderer's close (which is what causes the disappearing text bug), we 
update value to defaultValue when the children change. That also makes 
innerText work sensibly on textareas.

Now, a good question is, do other browsers behave like that, e.g. 
change .value when children change? Well, I tested Mozilla, Opera, IE6, and 
looked at the code of Safari. All of them do something totally different. IE6 
and Mozilla's behavior at all pretty weird. Opera's makes sense, but basic IE 
compatibility is a weird hack in innerText. I actually adopted what Safari 
does, because it's simple and as far as I can see should be compatible with 
IE in the less obscure of the cases...

Comments?
-Maks

["textarea-sync.diff" (text/x-diff)]

Index: rendering/render_form.cpp
===================================================================
--- rendering/render_form.cpp	(revision 671589)
+++ rendering/render_form.cpp	(working copy)
@@ -1603,23 +1603,18 @@
     edit->setTabChangesFocus( ! settings->allowTabulation() );
 
     connect(edit,SIGNAL(textChanged()),this,SLOT(slotTextChanged()));
+
+    setText(element->value().string());
 }
 
 RenderTextArea::~RenderTextArea()
 {
-    if ( element()->m_dirtyvalue ) {
-        element()->m_value = text();
-        element()->m_dirtyvalue = false;
-    }
+    element()->m_value = text();
 }
 
 void RenderTextArea::handleFocusOut()
 {
     TextAreaWidget* w = static_cast<TextAreaWidget*>(m_widget);
-    if ( w && element()->m_dirtyvalue ) {
-        element()->m_value = text();
-        element()->m_dirtyvalue = false;
-    }
 
     if ( w && element()->m_changed ) {
         element()->m_changed = false;
@@ -1677,38 +1672,35 @@
     }
 }
 
-void RenderTextArea::updateFromElement()
+void RenderTextArea::setText(const QString& newText)
 {
     TextAreaWidget* w = static_cast<TextAreaWidget*>(m_widget);
-    w->setReadOnly(element()->readOnly());
-    QString elementText = element()->value().string();
-    if ( elementText != text() )
-    {
+    // When this is called, m_value in the element must have just 
+    // been set to new value --- see if we have any work to do
+    if ( newText != text() ) {
         w->blockSignals(true);
         int line, col;
         w->getCursorPosition( &line, &col );
         int cx = w->contentsX();
         int cy = w->contentsY();
-        w->setText( elementText );
+        w->setText( newText );
         w->setCursorPosition( line, col );
         w->scrollBy( cx, cy );
         w->blockSignals(false);
     }
-    element()->m_dirtyvalue = false;
-
-    RenderFormElement::updateFromElement();
 }
 
-void RenderTextArea::close( )
+void RenderTextArea::updateFromElement()
 {
-    element()->setValue( element()->defaultValue() );
-
-    RenderFormElement::close();
+    TextAreaWidget* w = static_cast<TextAreaWidget*>(m_widget);
+    w->setReadOnly(element()->readOnly());
+    RenderFormElement::updateFromElement();
 }
 
-
 QString RenderTextArea::text()
 {
+    // ### We may want to cache this when physical, since value() no longer caches, 
+    // but seeing how text() has always been called on textChanged(), it's probably not needed
     QString txt;
     TextAreaWidget* w = static_cast<TextAreaWidget*>(m_widget);
 
@@ -1833,7 +1825,6 @@
 
 void RenderTextArea::slotTextChanged()
 {
-    element()->m_dirtyvalue = true;
     element()->m_changed    = true;
     if (element()->m_value != text())
         element()->m_unsubmittedFormChange = true;
Index: rendering/render_form.h
===================================================================
--- rendering/render_form.h	(revision 671589)
+++ rendering/render_form.h	(working copy)
@@ -460,7 +460,6 @@
     virtual void layout();
     virtual void setStyle(RenderStyle *style);
 
-    virtual void close ( );
     virtual void updateFromElement();
 
     // don't even think about making this method virtual!
@@ -469,6 +468,8 @@
     { return static_cast<DOM::HTMLTextAreaElementImpl*>(RenderObject::element()); }
 
     QString text();
+    void    setText(const QString& text);
+
     void highLightWord( unsigned int length, unsigned int pos );
 
     void select();
Index: html/html_formimpl.h
===================================================================
--- html/html_formimpl.h	(revision 671589)
+++ html/html_formimpl.h	(working copy)
@@ -536,6 +536,7 @@
     ~HTMLTextAreaElementImpl();
 
     virtual Id id() const;
+    virtual void childrenChanged();
 
     long cols() const { return m_cols; }
 
@@ -580,7 +581,6 @@
     WrapMethod m_wrap;
     QString m_value;
     bool m_changed: 1;    //States whether the contents has been editted
-    bool m_dirtyvalue: 1; //States whether m_value is out-of-date compared to the renderer or default
     bool m_unsubmittedFormChange: 1;
     bool m_initialized: 1;
 };
Index: html/html_formimpl.cpp
===================================================================
--- html/html_formimpl.cpp	(revision 671589)
+++ html/html_formimpl.cpp	(working copy)
@@ -2638,6 +2638,17 @@
 
 // -------------------------------------------------------------------------
 
+/*
+ The rules for storing the value are simple:
+
+ If there is no renderer, either m_value or defaultValue() is definitive, 
+    depending on whether m_initialized is true or not.
+ If there is a renderer, m_render->text() is definitive. During its construction,
+    m_value is initialized if needed,  so there is no longer any need to worry 
+    about default values or not.
+*/
+
+
 HTMLTextAreaElementImpl::HTMLTextAreaElementImpl(DocumentImpl *doc, HTMLFormElementImpl *f)
     : HTMLGenericFormElementImpl(doc, f)
 {
@@ -2646,7 +2657,6 @@
     m_cols = 20;
     m_wrap = ta_Virtual;
     m_changed     = false;
-    m_dirtyvalue  = true;
     m_initialized = false;
     m_unsubmittedFormChange = false;
 }
@@ -2675,7 +2685,6 @@
 {
     setDefaultValue(state.left(state.length()-1));
     m_unsubmittedFormChange = state.endsWith("M");
-    // the close() in the rendertree will take care of transferring defaultvalue to 'value'
 }
 
 void HTMLTextAreaElementImpl::select(  )
@@ -2685,6 +2694,11 @@
     onSelect();
 }
 
+void HTMLTextAreaElementImpl::childrenChanged()
+{
+    setValue(defaultValue());
+}
+
 void HTMLTextAreaElementImpl::parseAttribute(AttributeImpl *attr)
 {
     switch(attr->id())
@@ -2804,15 +2818,13 @@
 
 DOMString HTMLTextAreaElementImpl::value()
 {
-    if ( m_dirtyvalue) {
-        if ( m_render && m_initialized ) {
-            RenderTextArea* renderArea = static_cast<RenderTextArea*>( m_render );
-            m_value = renderArea->text();
-            m_dirtyvalue = false;
-        } else {
+    if (m_render) {
+        RenderTextArea* renderArea = static_cast<RenderTextArea*>( m_render );
+        m_value = renderArea->text();
+    } else {
+        if (!m_initialized) {
             m_value = defaultValue().string();
             m_initialized = true;
-            m_dirtyvalue = false;
         }
     }
 
@@ -2826,8 +2838,13 @@
     // \r\n -> \n, \r -> \n
     QString str = _value.string().replace( "\r\n", "\n" );
     m_value = str.replace( '\r', '\n' );
-    m_dirtyvalue = false;
     m_initialized = true;
+
+    if (m_render) {
+        RenderTextArea* renderArea = static_cast<RenderTextArea*>( m_render );
+        renderArea->setText( m_value );
+    }
+
     setChanged(true);
 }
 


[prev in list] [next in list] [prev in thread] [next in thread]