Projects/Acid3: Difference between revisions

    From KDE TechBase
    No edit summary
    m (Text replace - "<code diff>" to "<syntaxhighlight lang="diff">")
     
    (20 intermediate revisions by 4 users not shown)
    Line 1: Line 1:
    == Acid3 Test Failures of KHTML ==
    == Acid3 Test Failures of KHTML ==


    This is an overview of the remaing [https://bugs.kde.org/show_bug.cgi?id=156947 failures] of KHTML on the [http://acid3.acidtests.org/ Acid3] test. FF3.1 beta2pre scores 91/100, with 5 of the colored boxes filled in correctly.
    This is an overview of the remaing [https://bugs.kde.org/show_bug.cgi?id=156947 failures] of KHTML on the [http://acid3.acidtests.org/ Acid3] test. FF3.1 beta2pre scores 92/100, with 4 of the 6 colored boxes filled in correctly.


    {| border="1"
    {| border="1"
    Line 12: Line 12:
    ! passes in FF3.1
    ! passes in FF3.1
    |-
    |-
    | 04 || HTML Parser || Parser bug: iframe missing text kid || discard_until = ID_IFRAME+ID_CLOSE_TAG --- added in http://lists.kde.org/?l=kde-commits&m=99906936412933&w=2. || No || Yes
    | 13 || DOM Range || Unhandled mutation (will port wc patch. Sp.)|| || Yes || Yes
    |-
    | 13 || DOM Range || Unhandled mutation || || Yes || Yes
    |-
    |-
    | 26/27 || JS + DOM Memory management || Cycle breaking cleaning up too much || || Yes, but slow || Yes, but slow
    | 26/27 || JS + DOM Memory management || Cycle breaking cleaning up too much || || Yes, but slow || Yes, but slow
    Line 20: Line 18:
    | 29 || HTML Parser || Parser bug: table missing whitespace kid || || Yes || Yes
    | 29 || HTML Parser || Parser bug: table missing whitespace kid || || Yes || Yes
    |-
    |-
    | 40 || CSS Parser || Misparse of html*.test selector (see below) || || Yes
    | 48 (red linktest failed) || CSS || :visited doesn't match URL of part || Bug in konqueror's history manager. Works in testkhtml || Yes || Yes
    |-
    |-
    | 43 || || || Checkboxes, value, attributes || Yes || Yes
    | 51 || DOM2 Table || Stray row || Buggy test. Raised with Ian Hickson. || Yes || Yes
    |-
    |-
    | 44 || ????|| ???? || || Yes || Yes
    | 53 || DOM2 Forms || Not managing form's element collection when not in document. || || Yes || Yes
    |-
    |-
    | 48 (red linktest failed) || CSS || :visited doesn't match relative URL right || Performance critical, hot on things like Qt docs, and already slow || Yes || Yes
    | 70 || XML Parser || " UTF-8 encoded XML document with invalid character did not have a well-formedness error" || || No || Yes
    |-
    |-
    | 51 || DOM2 Table || Stray row || Buggy test. Raised with Ian Hickson. || Yes || Yes
    | 74 || DOM || "getSVGDocument missing on <iframe> element." || || Yes || Yes
    |-
    |-
    | 53 || DOM2 Forms || Not managing form's element collection when not in document. || || Yes || Yes
    | 75 || SVG+SMIL DOM || "SVG DOM interface SVGRectElement not supported." || || No || No
    |-
    |-
    | 65/69 || Part loading || onload events not emitted for many objects || || Yes, after several attempts || 69 fails due to network timeout
    | 76 || SVG+SMIL DOM || ?? || || No || No
    |-
    |-
    | 70-80 || || Can't run due to 65/69 || SVG, SMIL + XHTML || No || 72, 73, and 80 pass; rest fail
    | 77,78,79 || SVG Fonts || ?? || || No || No
    |}
    |}


    Note: An upgrade to PCRE 7.7 is required to make tests 89 and 90 pass.
    Note: An upgrade to PCRE 7.7 is required to make tests 89 and 90 pass.
    == Acid3 Error report ==
    Error obtained with Konqueror 4.2.1, using Qt4.5RC. 20-march-2009.
    <pre>
    Failed 15 tests.
    Test 04 failed: expected 'null' but got '[object HTMLIFrameElement]' - expectation 21 failed
    Test 08 passed, but took 86ms (less than 30fps)
    Test 09 passed, but took 59ms (less than 30fps)
    Test 13 failed: collapsed is wrong after deletion
    Test 26 failed: e1 - parent element doesn't exist after looping
    Test 27 failed: e1 - parent element doesn't exist after waiting
    Test 29 failed: expected '2' but got '1' - cloned table had wrong number of children
    Test 33 passed, but took 61ms (less than 30fps)
    Test 40 passed, but took 46ms (less than 30fps)
    Test 44 failed: expected '0' but got '1' - misparsed selectors
    Test 46 passed, but took 57ms (less than 30fps)
    Test 47 passed, but took 52ms (less than 30fps)
    Test 51 failed: expected '6' but got '5' - wrong number of rows
    Test 53 failed: expected '1' but got '0' - form's elements array has wrong size
    Test 69 passed, but took 213 attempts (less than perfect).
    Test 70 failed: UTF-8 encoded XML document with invalid character did not have a well-formedness error
    Test 74 failed: getSVGDocument missing on <iframe> element.
    Test 75 failed: SVG DOM interface SVGRectElement not supported.
    Test 76 failed: Undefined value
    Test 77 failed: SVGTextContentElement.getNumberOfChars() not supported.
    Test 78 failed: Attempt to use a non-function object or a value as a function.
    Test 79 failed: Attempt to use a non-function object or a value as a function.
    Test 98 passed, but took 63ms (less than 30fps)
    Total elapsed time: 17.30s
    </pre>


    == Random patch storage ==  
    == Random patch storage ==  
    Line 45: Line 72:
    === #16, red cat. ===
    === #16, red cat. ===
    Ugly, but roughly correct.
    Ugly, but roughly correct.
    <pre>
    <syntaxhighlight lang="diff">
    Index: kio/slavebase.cpp
    Index: kio/slavebase.cpp
    ===================================================================
    ===================================================================
    Line 70: Line 97:
         send( INF_MIME_TYPE, data );
         send( INF_MIME_TYPE, data );
         while(true)
         while(true)
    </pre>
    </syntaxhighlight>
     
    === #4, iframe kids. ===
    This one may be committable, actually
    <pre>
    --- a/html/htmlparser.cpp
    +++ b/html/htmlparser.cpp
    @@ -874,7 +874,7 @@ NodeImpl *KHTMLParser::getElement(Token* t)
            // a bit a special case, since the frame is inlined...
        case ID_IFRAME:
            n = new HTMLIFrameElementImpl(document);
    -        if (!t->flat) discard_until = ID_IFRAME+ID_CLOSE_TAG;
    +        //if (!t->flat) discard_until = ID_IFRAME+ID_CLOSE_TAG;
            break;
     
    // form elements
    </pre>
     
    === #40 testcase ===
    <pre>
    <style>
    html*.test { border: 2px solid red; margin: 5px; }
    </style>
    <body class="test">
      <p class="test">There should be no red borders here.
    </body>
    </pre>
     


    === #48, red linktest failed ===
    === #48, red linktest failed ===
    It's a bug inside KonqHistoryManager --- it ignores some inserts, which makes the link not :visited. We may still want to consider the diff below, but it'll likely slow things down too much.
    It's a bug inside KonqHistoryManager --- it ignores some inserts, which makes the link not :visited.
    <pre>
    Index: css/cssstyleselector.h
    ===================================================================
    --- css/cssstyleselector.h (revision 873693)
    +++ css/cssstyleselector.h (working copy)
    @@ -29,6 +29,7 @@
    #include "css/css_mediaquery.h"
    #include <QtCore/QVarLengthArray>
    #include <QtCore/QList>
    +#include <kurl.h>
    class KHTMLSettings;
    class KHTMLView;
    @@ -187,11 +188,7 @@
    QVector<int> fixedFontSizes() const { return m_fixedFontSizes; }
    bool strictParsing;
    - struct Encodedurl {
    -     QString host; //also contains protocol
    -     QString path;
    -     QString file;
    - } encodedurl;
    + KUrl encodedurl;
            // called from KHTMLView::print()
    void computeFontSizes(int logicalDpiY, int zoomFactor);
    Index: css/cssstyleselector.cpp
    ===================================================================
    --- css/cssstyleselector.cpp (revision 873693)
    +++ css/cssstyleselector.cpp (working copy)
    @@ -287,15 +287,7 @@
        u.setQuery( QString() );
        u.setRef( QString() );
    -    encodedurl.file = u.url();
    -    int pos = encodedurl.file.lastIndexOf('/');
    -    encodedurl.path = encodedurl.file;
    -    if ( pos > 0 ) {
    - encodedurl.path.truncate( pos );
    - encodedurl.path += '/';
    -    }
    -    u.setPath( QString() );
    -    encodedurl.host = u.url();
    +    encodedurl = u;
        //kDebug() << "CSSStyleSelector::CSSStyleSelector encoded url " << encodedurl.path;
    }
    @@ -919,46 +911,8 @@
        return numProps;
    }
    -// modified version of the one in kurl.cpp
    -static void cleanpath(QString &path)
    +static PseudoState checkPseudoState( const KUrl& encodedurl, DOM::ElementImpl *e )
    {
    -    int pos;
    -    while ( (pos = path.indexOf( "/../" )) != -1 ) {
    -        int prev = 0;
    -        if ( pos > 0 )
    -            prev = path.lastIndexOf( "/", pos -1 );
    -        // don't remove the host, i.e. http://foo.org/../foo.html
    -        if (prev < 0 || (prev > 3 && path.lastIndexOf("://", prev-1) == prev-2))
    -            path.remove( pos, 3);
    -        else
    -            // matching directory found ?
    -            path.remove( prev, pos- prev + 3 );
    -    }
    -    pos = 0;
    -
    -    // Don't remove "//" from an anchor identifier. -rjw
    -    // Set refPos to -2 to mean "I haven't looked for the anchor yet".
    -    // We don't want to waste a function call on the search for the anchor
    -    // in the vast majority of cases where there is no "//" in the path.
    -    int refPos = -2;
    -    while ( (pos = path.indexOf( "//", pos )) != -1) {
    -        if (refPos == -2)
    -            refPos = path.indexOf("#", 0);
    -        if (refPos > 0 && pos >= refPos)
    -            break;
    -
    -        if ( pos == 0 || path[pos-1] != ':' )
    -            path.remove( pos, 1 );
    -        else
    -            pos += 2;
    -    }
    -    while ( (pos = path.indexOf( "/./" )) != -1)
    -        path.remove( pos, 2 );
    -    //kDebug() << "checkPseudoState " << path;
    -}
    -
    -static PseudoState checkPseudoState( const CSSStyleSelector::Encodedurl& encodedurl, DOM::ElementImpl *e )
    -{
        if( e->id() != ID_A ) {
            return PseudoNone;
        }
    @@ -966,20 +920,20 @@
        if( attr.isNull() ) {
            return PseudoNone;
        }
    -    QString u = QString::fromRawData(attr.unicode(), attr.length());
    -    if ( !u.contains("://") ) {
    -        if ( u[0] == '/' )
    -            u = encodedurl.host + u;
    -        else if ( u[0] == '#' )
    -            u = encodedurl.file + u;
    -        else
    -            u = encodedurl.path + u;
    -        cleanpath( u );
    -    }
    -    //completeURL( attr.string() );
    +    QString rel = QString::fromRawData(attr.unicode(), attr.length());
    +
    +    // Note: we do want to check both fragment and query here.
    +    KUrl absolute(encodedurl, rel);
    +    absolute.cleanPath();
    +
    +    QString u = absolute.url();
    +   
    +    kDebug() << u;
    +
        bool contains = KHTMLGlobal::vLinks()->contains( u );
        if ( !contains && u.count('/')==2 )
          contains = KHTMLGlobal::vLinks()->contains( u+'/' );
    +    kDebug() << u << "was found?" << contains;
        return contains ? PseudoVisited : PseudoLink;
    }
    </pre>
     
    === 7x tests ===
    A lot of the fail due to XSS checks disabling all access to XML. Below fixes it:
    <pre>
    Index: khtml_part.cpp
    ===================================================================
    --- khtml_part.cpp (revision 873693)
    +++ khtml_part.cpp (working copy)
    @@ -5314,7 +5321,7 @@
      if (callingHtmlPart == this)
        return true; // trivial
    -  if (htmlDocument().isNull()) {
    +  if (!xmlDocImpl()) {
    #ifdef DEBUG_FINDFRAME
        kDebug(6050) << "Empty part" << this << "URL = " << url();
    #endif
    @@ -5322,9 +5329,9 @@
      }
      // now compare the domains
    -  if (callingHtmlPart && callingHtmlPart->docImpl() && docImpl())  {
    -    DOM::DOMString actDomain = callingHtmlPart->docImpl()->domain();
    -    DOM::DOMString destDomain = docImpl()->domain();
    +  if (callingHtmlPart && callingHtmlPart->xmlDocImpl() && xmlDocImpl())  {
    +    DOM::DOMString actDomain = callingHtmlPart->xmlDocImpl()->domain();
    +    DOM::DOMString destDomain = xmlDocImpl()->domain();
    #ifdef DEBUG_FINDFRAME
        kDebug(6050) << "actDomain =" << actDomain.string() << "destDomain =" << destDomain.string();
    Index: ecma/kjs_window.cpp
    ===================================================================
    --- ecma/kjs_window.cpp (revision 873693)
    +++ ecma/kjs_window.cpp (working copy)
    @@ -1312,17 +1313,13 @@
      if ( !part->xmlDocImpl() )
        return true; // allow to access a window that was just created (e.g. with window.open("about:blank"))
    -  DOM::HTMLDocumentImpl* thisDocument = part->docImpl();
    -  if ( !thisDocument ) {
    -    kDebug(6070) << "Window::isSafeScript: trying to access an XML document !?";
    -    return false;
    -  }
    +  DOM::DocumentImpl* thisDocument = part->xmlDocImpl();
      KHTMLPart *activeKHTMLPart = qobject_cast<KHTMLPart*>(activePart);
      if (!activeKHTMLPart)
        return true; // not a KHTMLPart
    -  DOM::HTMLDocumentImpl* actDocument = activeKHTMLPart->docImpl();
    +  DOM::DocumentImpl* actDocument = activeKHTMLPart->xmlDocImpl();
      if ( !actDocument ) {
        kDebug(6070) << "Window::isSafeScript: active part has no document!";
        return false;
    </pre>
     
    === #71 testcase ===
    With above, we fail at least the following in #71:
    <pre>
    <script>
    var doc = document;
    doc.open();
    doc.write("<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0 Transitional//EN\"><title><\/title><span><\/span><script type=\"text/javascript\"><\/script>");
    doc.close();
    alert(document.childNodes.length);
    alert(document.childNodes[0]);
    alert(document.childNodes[1]);
    </script>
    </pre>
    This is fixed by below, but integrated test still fails:
    <pre>
    --- html/htmlparser.cpp (revision 873693)
    +++ html/htmlparser.cpp (working copy)
    @@ -316,7 +316,8 @@
    void KHTMLParser::parseDoctypeToken(DoctypeToken* t)
    {
        // Ignore any doctype after the first. TODO It should be also ignored when processing DocumentFragment
    -    if (current != document || document->doctype())
    +    // We do want to accept it even after we have a document, however.
    +    if (document->doctype())
            return;
     
        DocumentTypeImpl* doctype = new DocumentTypeImpl(document->implementation(), document, t->name, t->publicID, t->systemID);
    </pre>

    Latest revision as of 21:14, 29 June 2011

    Acid3 Test Failures of KHTML

    This is an overview of the remaing failures of KHTML on the Acid3 test. FF3.1 beta2pre scores 92/100, with 4 of the 6 colored boxes filled in correctly.

    Test No Area Diagnosis Comment passes in FF3 passes in FF3.1
    13 DOM Range Unhandled mutation (will port wc patch. Sp.) Yes Yes
    26/27 JS + DOM Memory management Cycle breaking cleaning up too much Yes, but slow Yes, but slow
    29 HTML Parser Parser bug: table missing whitespace kid Yes Yes
    48 (red linktest failed) CSS :visited doesn't match URL of part Bug in konqueror's history manager. Works in testkhtml Yes Yes
    51 DOM2 Table Stray row Buggy test. Raised with Ian Hickson. Yes Yes
    53 DOM2 Forms Not managing form's element collection when not in document. Yes Yes
    70 XML Parser " UTF-8 encoded XML document with invalid character did not have a well-formedness error" No Yes
    74 DOM "getSVGDocument missing on <iframe> element." Yes Yes
    75 SVG+SMIL DOM "SVG DOM interface SVGRectElement not supported." No No
    76 SVG+SMIL DOM ?? No No
    77,78,79 SVG Fonts ?? No No

    Note: An upgrade to PCRE 7.7 is required to make tests 89 and 90 pass.

    Acid3 Error report

    Error obtained with Konqueror 4.2.1, using Qt4.5RC. 20-march-2009.

    Failed 15 tests.
    Test 04 failed: expected 'null' but got '[object HTMLIFrameElement]' - expectation 21 failed
    Test 08 passed, but took 86ms (less than 30fps)
    Test 09 passed, but took 59ms (less than 30fps)
    Test 13 failed: collapsed is wrong after deletion
    Test 26 failed: e1 - parent element doesn't exist after looping
    Test 27 failed: e1 - parent element doesn't exist after waiting
    Test 29 failed: expected '2' but got '1' - cloned table had wrong number of children
    Test 33 passed, but took 61ms (less than 30fps)
    Test 40 passed, but took 46ms (less than 30fps)
    Test 44 failed: expected '0' but got '1' - misparsed selectors
    Test 46 passed, but took 57ms (less than 30fps)
    Test 47 passed, but took 52ms (less than 30fps)
    Test 51 failed: expected '6' but got '5' - wrong number of rows
    Test 53 failed: expected '1' but got '0' - form's elements array has wrong size
    Test 69 passed, but took 213 attempts (less than perfect).
    Test 70 failed: UTF-8 encoded XML document with invalid character did not have a well-formedness error
    Test 74 failed: getSVGDocument missing on <iframe> element.
    Test 75 failed: SVG DOM interface SVGRectElement not supported.
    Test 76 failed: Undefined value
    Test 77 failed: SVGTextContentElement.getNumberOfChars() not supported.
    Test 78 failed: Attempt to use a non-function object or a value as a function.
    Test 79 failed: Attempt to use a non-function object or a value as a function.
    Test 98 passed, but took 63ms (less than 30fps)
    Total elapsed time: 17.30s
    

    Random patch storage

    These are not meant for commit, but more as a proof of analysis, and starting point for proper fix:

    #16, red cat.

    Ugly, but roughly correct.

    Index: kio/slavebase.cpp
    ===================================================================
    --- kio/slavebase.cpp	(revision 793314)
    +++ kio/slavebase.cpp	(working copy)
    @@ -527,6 +527,7 @@
     void SlaveBase::errorPage()
     {
         send( INF_ERROR_PAGE );
    +    mOutgoingMetaData["__kio_error_page"] = "1";
     }
     
     static bool isSubCommand(int cmd)
    @@ -554,6 +555,11 @@
           KIO_DATA << mOutgoingMetaData;
           send( INF_META_DATA, data );
         }
    +
    +    // re-send the error-page flag as well.
    +    if (mOutgoingMetaData.contains("__kio_error_page"))
    +	send( INF_ERROR_PAGE );
    +    
         KIO_DATA << _type;
         send( INF_MIME_TYPE, data );
         while(true)
    

    #48, red linktest failed

    It's a bug inside KonqHistoryManager --- it ignores some inserts, which makes the link not :visited.