Bug 121297

Summary: Bugs in textarea.selectionStart/selectionEnd (as seen on wikipedia)
Product: [Applications] konqueror Reporter: Marijn Schouten <hkBst>
Component: khtml ecmaAssignee: Konqueror Developers <konq-bugs>
Status: RESOLVED WORKSFORME    
Severity: normal CC: maksim
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Gentoo Packages   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description Marijn Schouten 2006-02-03 17:04:58 UTC
Version:           3.5.1 (using KDE KDE 3.5.1)
Installed from:    Gentoo Packages
Compiler:          gcc-3.4.5-vanilla 
OS:                Linux

When surfing to http://en.wikipedia.org/w/index.php?title=Fundamental_vector_field&action=edit I get no insertTag buttons, which I do get in firefox. Javascript is enabled.
Comment 1 Thiago Macieira 2006-02-04 16:19:38 UTC
Konqueror doesn't show them because Wikipedia disables them for Konqueror:

	// Don't generate buttons for browsers which don't fully
	// support it.
	if (!document.selection && !is_gecko) {
		return false;
	}

If you spoof and force them to show, they don't work.
Comment 2 Marijn Schouten 2006-02-06 11:55:23 UTC
>If you spoof and force them to show, they don't work. 

Fact remains that it doesn't work. So I'd hardly call this bug resolved.
Comment 3 Marijn Schouten 2006-02-06 11:57:07 UTC
see previous comment
Comment 4 Maksim Orlovich 2006-02-06 15:41:59 UTC
That's because you're using the obsolete ;-) 3.5.1, and not the development version, which supports the Mozilla-proprietary feature the site uses (though I still have to fix a few bugs in it)
Comment 5 Thiago Macieira 2006-02-07 19:18:36 UTC
Retitling.

If it's committed to trunk, close it.
Comment 6 Maksim Orlovich 2006-02-07 19:32:10 UTC
Wrong title. document.selection is an IEism here, and not relevant.

Comment 7 Maksim Orlovich 2006-02-08 23:31:32 UTC
SVN commit 507283 by orlovich:

Make selectionStart/selectionEnd more reliable -- in particular 
WikiPedia insertion things seem to work very well right now. The bulk of the 
fix is in moving CR LF expansion to form submission, so the renderer and the 
DOM don't fight about whether to use CR or CR LF. There is still one case that's
broken: when physical wrap text areas are first created, they wrap the text. but the DOM 
doesn't know that. Not sure how to fix that.

Also, remove the onChange call in ::value -- too dangerous -- and re-fix onChange differently,
by keeping track of need to emit it separately from the m_value dirtiness.

CCBUG:121297


 M  +41 -17    html/html_formimpl.cpp  
 M  +2 -1      html/html_formimpl.h  
 M  +19 -52    rendering/render_form.cpp  


--- branches/KDE/3.5/kdelibs/khtml/html/html_formimpl.cpp #507282:507283
@@ -2550,7 +2550,8 @@
     m_rows = 2;
     m_cols = 20;
     m_wrap = ta_Virtual;
-    m_dirtyvalue = true;
+    m_changed     = false;
+    m_dirtyvalue  = true;
     m_initialized = false;
     m_unsubmittedFormChange = false;
 }
@@ -2649,12 +2650,47 @@
     _style->deref();
 }
 
+
+static QString expandLF(const QString& s)
+{
+    // LF -> CRLF
+    unsigned crs = s.contains( '\n' );
+    if (crs == 0)
+	return s;
+    unsigned len = s.length();
+
+    QString r;
+    r.reserve(len + crs + 1);
+    unsigned pos2 = 0;
+    for(unsigned pos = 0; pos < len; pos++)
+    {
+       QChar c = s.at(pos);
+       switch(c.unicode())
+       {
+         case '\n':
+           r[pos2++] = '\r';
+           r[pos2++] = '\n';
+           break;
+
+         case '\r':
+           break;
+
+         default:
+           r[pos2++]= c;
+           break;
+       }
+    }
+    r.squeeze();
+    return r;
+}
+
+
 bool HTMLTextAreaElementImpl::encoding(const QTextCodec* codec, encodingList& encoding, bool)
 {
     if (name().isEmpty()) return false;
 
     encoding += fixUpfromUnicode(codec, name().string());
-    encoding += fixUpfromUnicode(codec, value().string());
+    encoding += fixUpfromUnicode(codec, expandLF(value().string()));
 
     return true;
 }
@@ -2664,14 +2700,14 @@
     setValue(defaultValue());
 }
 
+
 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; // before onChange (#100963)
-            onChange();
+            m_dirtyvalue = false;
         } else {
             m_value = defaultValue().string();
             m_initialized = true;
@@ -2787,19 +2823,7 @@
 
 long HTMLTextAreaElementImpl::textLength()
 {
-    //First, get the value. This is like ::value, only pure.
-    DOMString val = m_value;
-    if (m_dirtyvalue) {
-        if ( m_render && m_initialized ) {
-            RenderTextArea* renderArea = static_cast<RenderTextArea*>( m_render );
-            val = renderArea->text();
-        } else {
-            val = defaultValue();
-        }
-    }
-
-    //now we can get the length.
-    return val.length();
+    return value().length();
 }
 
 // -------------------------------------------------------------------------
--- branches/KDE/3.5/kdelibs/khtml/html/html_formimpl.h #507282:507283
@@ -566,7 +566,8 @@
     int m_cols;
     WrapMethod m_wrap;
     QString m_value;
-    bool m_dirtyvalue: 1;
+    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;
 };
--- branches/KDE/3.5/kdelibs/khtml/rendering/render_form.cpp #507282:507283
@@ -1566,6 +1566,10 @@
     if ( w && element()->m_dirtyvalue ) {
         element()->m_value = text();
         element()->m_dirtyvalue = false;
+    }
+
+    if ( w && element()->m_changed ) {
+        element()->m_changed = false;
         element()->onChange();
     }
 }
@@ -1649,39 +1653,7 @@
     RenderFormElement::close();
 }
 
-static QString expandLF(const QString& s)
-{
-    // LF -> CRLF
-    unsigned crs = s.contains( '\n' );
-    if (crs == 0)
-	return s;
-    unsigned len = s.length();
 
-    QString r;
-    r.reserve(len + crs + 1);
-    unsigned pos2 = 0;
-    for(unsigned pos = 0; pos < len; pos++)
-    {
-       QChar c = s.at(pos);
-       switch(c.unicode())
-       {
-         case '\n':
-           r[pos2++] = '\r';
-           r[pos2++] = '\n';
-           break;
-
-         case '\r':
-           break;
-
-         default:
-           r[pos2++]= c;
-           break;
-       }
-    }
-    r.squeeze();
-    return r;
-}
-
 QString RenderTextArea::text()
 {
     QString txt;
@@ -1709,19 +1681,9 @@
     else
         txt = w->text();
 
-    return expandLF(txt);
+    return txt;
 }
 
-static int expandedCnt(unsigned short code)
-{
-    if (code == '\n')
-        return 2;
-    else if (code == '\r')
-        return 0;
-    else
-        return 1;
-}
-
 int RenderTextArea::queryParagraphInfo(int para, Mode m, int param) {
     /* We have to be a bit careful here, as we need to match up the positions
     to what our value returns here*/
@@ -1732,6 +1694,7 @@
 
     QString paragraphText = w->text(para);
     int pl                = w->paragraphLength(para);
+    int physicalPL        = pl;
     if (m == ParaPortionLength)
         pl = param;
 
@@ -1739,25 +1702,28 @@
         //Go through all the chars of paragraph, and count line changes, chars, etc.
         int lindex = w->lineOfChar(para, 0);
         for (int c = 0; c < pl; ++c) {
-            if (lindex != w->lineOfChar(para, c)) {
-                length += 2;
-                lindex =  w->lineOfChar(para, c);
+            ++length;
+            // Is there a change after this char?
+            if (c+1 < physicalPL && lindex != w->lineOfChar(para, c+1)) {
+                lindex =  w->lineOfChar(para, c+1);
+                ++length;
             }
-            length += expandedCnt(paragraphText.at(c).unicode());
             if (m == ParaPortionOffset && length > param)
                 return c;
         }
     } else {
-        //Make sure to count the LF, CR as appropriate..
+        //Make sure to count the LF, CR as appropriate. ### this is stupid now, simplify
         for (int c = 0; c < pl; ++c) {
-            length += expandedCnt(paragraphText.at(c).unicode());
+            ++length;
             if (m == ParaPortionOffset && length > param)
                 return c;
         }
     }
     if (m == ParaPortionOffset)
         return pl;
-    return length;
+    if (m == ParaPortionLength)
+        return length;
+    return length + 1;
 }
 
 long RenderTextArea::computeCharOffset(int para, int index) {
@@ -1766,7 +1732,7 @@
 
     long pos = 0;
     for (int cp = 0; cp < para; ++cp)
-        pos += queryParagraphInfo(cp, ParaLength) + 2;
+        pos += queryParagraphInfo(cp, ParaLength);
 
     if (index >= 0)
         pos += queryParagraphInfo(para, ParaPortionLength, index);
@@ -1787,7 +1753,7 @@
     long endPos       = 0;
     long startPos     = 0;
     for (int p = 0; p < w->paragraphs(); ++p) {
-        int len = queryParagraphInfo(p, ParaLength) + 2;
+        int len = queryParagraphInfo(p, ParaLength);
         endPos += len;
         if (endPos > offset) {
             containingPar = p;
@@ -1815,6 +1781,7 @@
 void RenderTextArea::slotTextChanged()
 {
     element()->m_dirtyvalue = true;
+    element()->m_changed    = true;
     if (element()->m_value != text())
         element()->m_unsubmittedFormChange = true;
 }
Comment 8 Marijn Schouten 2006-04-17 11:24:34 UTC
If I spoof as firefox 1.0 with konqueror 3.5.2 all insertion buttons work. Great job Maksim! 
I'm now trying to get wikipedia to recognize this at http://bugzilla.wikimedia.org/show_bug.cgi?id=5572
they seem to want a way to check for the required features.
Comment 9 Maksim Orlovich 2006-04-17 15:45:37 UTC
Just checking for selectionStart should work --- and I -think- that's what they did.
Comment 10 Marijn Schouten 2006-04-18 14:39:41 UTC
Ok, this issue has been completely resolved.