Changeset 63338 in webkit


Ignore:
Timestamp:
Jul 14, 2010 11:49:42 AM (14 years ago)
Author:
eric@webkit.org
Message:

2010-07-13 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

reconstructActiveFormElements should reconstruct attributes as well
​https://bugs.webkit.org/show_bug.cgi?id=42222

  • html5lib/resources/adoption01.dat:
  • html5lib/runner-expected-html5.txt:
  • html5lib/runner-expected.txt:

2010-07-13 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

reconstructActiveFormElements should reconstruct attributes as well
​https://bugs.webkit.org/show_bug.cgi?id=42222

The case in question is "<p><b foo='bar'></p>text</b>".
When the "b" is re-opened to wrap the text it should include
any attributes from the original (now closed) tag name.

There are also similar cases for the Adoption Agency algorithm, but since
the html5lib test suite did not cover those (and it wasn't immediately
obvious to me how to test those) I've saved fixing that bug for a
later patch. For now I've just made the adoption agency use
HTMLConstructionSite::createHTMLElementFromElementRecord so the
FIXME can be in one place instead of two.

In order to cleanly support createHTMLElementFromSavedElement
I re-factored "attachToCurrent" out from createHTMLElementAndAttachToCurrent
and changed all callers to use attachToCurrent(createHTMLElement(token)).

This is covered by two existing tests in html5lib/runner.html
and I wrote two more. One to cover the basic case that we now pass
and a second to cover an evil edge case which we do not.

  • html/HTMLConstructionSite.cpp: (WebCore::HTMLConstructionSite::attachToCurrent): (WebCore::HTMLConstructionSite::insertHTMLHtmlElement): (WebCore::HTMLConstructionSite::insertHTMLHeadElement): (WebCore::HTMLConstructionSite::insertHTMLBodyElement): (WebCore::HTMLConstructionSite::insertHTMLElement): (WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement): (WebCore::HTMLConstructionSite::insertScriptElement): (WebCore::HTMLConstructionSite::insertForeignElement): (WebCore::HTMLConstructionSite::createHTMLElementFromElementRecord): (WebCore::HTMLConstructionSite::createHTMLElementFromSavedElement): (WebCore::HTMLConstructionSite::reconstructTheActiveFormattingElements):
  • html/HTMLConstructionSite.h:
  • html/HTMLTreeBuilder.cpp: (WebCore::HTMLTreeBuilder::callTheAdoptionAgency):
Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r63337 r63338  
     12010-07-13  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        reconstructActiveFormElements should reconstruct attributes as well
     6        https://bugs.webkit.org/show_bug.cgi?id=42222
     7
     8        * html5lib/resources/adoption01.dat:
     9        * html5lib/runner-expected-html5.txt:
     10        * html5lib/runner-expected.txt:
     11
    1122010-07-14  Kenneth Russell  <kbr@google.com>
    213

  • trunk/LayoutTests/html5lib/resources/adoption01.dat

    r62537 r63338  
    125125|         <p>
    126126|           <a>
     127
     128#data
     129<p>1<s id="A">2<b id="B">3</p>4</s>5</b>
     130#errors
     131#document
     132| <html>
     133|   <head>
     134|   <body>
     135|     <p>
     136|       "1"
     137|       <s>
     138|         id="A"
     139|         "2"
     140|         <b>
     141|           id="B"
     142|           "3"
     143|     <s>
     144|       id="A"
     145|       <b>
     146|         id="B"
     147|         "4"
     148|     <b>
     149|       id="B"
     150|       "5"
     151
     152#data
     153<p><b id="A"><script>document.getElementById("A").id = "B"</script></p>TEXT</b>
     154#errors
     155#document
     156| <html>
     157|   <head>
     158|   <body>
     159|     <p>
     160|       <b>
     161|         id="B"
     162|         <script>
     163|           "document.getElementById("A").id = "B""
     164|     <b>
     165|       id="A"
     166|       "TEXT"

  • trunk/LayoutTests/html5lib/runner-expected-html5.txt

    r63274 r63338  
    2424|         "br"
    2525|       <a>
     26|         href="foo"
    2627|       "x"
    2728|       <table>
    … …  
    3031|             <td>
    3132|     <a>
     33|       href="foo"
    3234|       "aoe"
    3335Expected:
    … …  
    6264|       "aba"
    6365|     <a>
     66|       href="blah"
    6467|     "x"
    6568|     <table>
    … …  
    7174|               "br"
    7275|     <a>
     76|       href="blah"
    7377|       "aoe"
    7478Expected:
    … …  
    323327resources/adoption01.dat:
    3243283
    325 
    326 Test 3 of 9 in resources/adoption01.dat failed. Input:
     32911
     330
     331Test 3 of 11 in resources/adoption01.dat failed. Input:
    327332<a>1<button>2</a>3</button>
    328333Got:
    … …  
    343348|         "2"
    344349|     "3"
     350
     351Test 11 of 11 in resources/adoption01.dat failed. Input:
     352<p><b id="A"><script>document.getElementById("A").id = "B"</script></p>TEXT</b>
     353Got:
     354| <html>
     355|   <head>
     356|   <body>
     357|     <p>
     358|       <b>
     359|         id="B"
     360|         <script>
     361|           "document.getElementById("A").id = "B""
     362|     <b>
     363|       id="B"
     364|       "TEXT"
     365Expected:
     366| <html>
     367|   <head>
     368|   <body>
     369|     <p>
     370|       <b>
     371|         id="B"
     372|         <script>
     373|           "document.getElementById("A").id = "B""
     374|     <b>
     375|       id="A"
     376|       "TEXT"
    345377resources/inbody01.dat: PASS
    346378

  • trunk/LayoutTests/html5lib/runner-expected.txt

    r63185 r63338  
    500450048
    500550059
    5006 
    5007 Test 1 of 9 in resources/adoption01.dat failed. Input:
     500611
     5007
     5008Test 1 of 11 in resources/adoption01.dat failed. Input:
    50085009<a><p></a></p>
    50095010Got:
    … …  
    50215022|       <a>
    50225023
    5023 Test 6 of 9 in resources/adoption01.dat failed. Input:
     5024Test 6 of 11 in resources/adoption01.dat failed. Input:
    50245025<table><a>1<p>2</a>3</p>
    50255026Got:
    … …  
    50465047|     <table>
    50475048
    5048 Test 7 of 9 in resources/adoption01.dat failed. Input:
     5049Test 7 of 11 in resources/adoption01.dat failed. Input:
    50495050<b><b><a><p></a>
    50505051Got:
    … …  
    50665067|           <a>
    50675068
    5068 Test 8 of 9 in resources/adoption01.dat failed. Input:
     5069Test 8 of 11 in resources/adoption01.dat failed. Input:
    50695070<b><a><b><p></a>
    50705071Got:
    … …  
    50885089|           <a>
    50895090
    5090 Test 9 of 9 in resources/adoption01.dat failed. Input:
     5091Test 9 of 11 in resources/adoption01.dat failed. Input:
    50915092<a><b><b><p></a>
    50925093Got:
    … …  
    51115112|         <p>
    51125113|           <a>
     5114
     5115Test 11 of 11 in resources/adoption01.dat failed. Input:
     5116<p><b id="A"><script>document.getElementById("A").id = "B"</script></p>TEXT</b>
     5117Got:
     5118| <html>
     5119|   <head>
     5120|   <body>
     5121|     <p>
     5122|       <b>
     5123|         id="B"
     5124|         <script>
     5125|           "document.getElementById("A").id = "B""
     5126|     <b>
     5127|       id="B"
     5128|       "TEXT"
     5129Expected:
     5130| <html>
     5131|   <head>
     5132|   <body>
     5133|     <p>
     5134|       <b>
     5135|         id="B"
     5136|         <script>
     5137|           "document.getElementById("A").id = "B""
     5138|     <b>
     5139|       id="A"
     5140|       "TEXT"
    51135141resources/inbody01.dat: PASS
    51145142

  • trunk/WebCore/ChangeLog

    r63335 r63338  
     12010-07-13  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        reconstructActiveFormElements should reconstruct attributes as well
     6        https://bugs.webkit.org/show_bug.cgi?id=42222
     7
     8        The case in question is "<p><b foo='bar'></p>text</b>".
     9        When the "b" is re-opened to wrap the text it should include
     10        any attributes from the original (now closed) tag name.
     11
     12        There are also similar cases for the Adoption Agency algorithm, but since
     13        the html5lib test suite did not cover those (and it wasn't immediately
     14        obvious to me how to test those) I've saved fixing that bug for a
     15        later patch.  For now I've just made the adoption agency use
     16        HTMLConstructionSite::createHTMLElementFromElementRecord so the
     17        FIXME can be in one place instead of two.
     18
     19        In order to cleanly support createHTMLElementFromSavedElement
     20        I re-factored "attachToCurrent" out from createHTMLElementAndAttachToCurrent
     21        and changed all callers to use attachToCurrent(createHTMLElement(token)).
     22
     23        This is covered by two existing tests in html5lib/runner.html
     24        and I wrote two more.  One to cover the basic case that we now pass
     25        and a second to cover an evil edge case which we do not.
     26
     27        * html/HTMLConstructionSite.cpp:
     28        (WebCore::HTMLConstructionSite::attachToCurrent):
     29        (WebCore::HTMLConstructionSite::insertHTMLHtmlElement):
     30        (WebCore::HTMLConstructionSite::insertHTMLHeadElement):
     31        (WebCore::HTMLConstructionSite::insertHTMLBodyElement):
     32        (WebCore::HTMLConstructionSite::insertHTMLElement):
     33        (WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement):
     34        (WebCore::HTMLConstructionSite::insertScriptElement):
     35        (WebCore::HTMLConstructionSite::insertForeignElement):
     36        (WebCore::HTMLConstructionSite::createHTMLElementFromElementRecord):
     37        (WebCore::HTMLConstructionSite::createHTMLElementFromSavedElement):
     38        (WebCore::HTMLConstructionSite::reconstructTheActiveFormattingElements):
     39        * html/HTMLConstructionSite.h:
     40        * html/HTMLTreeBuilder.cpp:
     41        (WebCore::HTMLTreeBuilder::callTheAdoptionAgency):
     42
    1432010-07-13  Alexey Proskuryakov  <ap@apple.com>
    244

  • trunk/WebCore/html/HTMLConstructionSite.cpp

    r63037 r63338  
    194194}
    195195
    196 PassRefPtr<Element> HTMLConstructionSite::createHTMLElementAndAttachToCurrent(AtomicHTMLToken& token)
     196PassRefPtr<Element> HTMLConstructionSite::attachToCurrent(PassRefPtr<Element> child)
     197{
     198    return attach(currentElement(), child);
     199}
     200
     201void HTMLConstructionSite::insertHTMLHtmlElement(AtomicHTMLToken& token)
     202{
     203    ASSERT(!m_redirectAttachToFosterParent);
     204    m_openElements.pushHTMLHtmlElement(attachToCurrent(createHTMLElement(token)));
     205}
     206
     207void HTMLConstructionSite::insertHTMLHeadElement(AtomicHTMLToken& token)
     208{
     209    ASSERT(!m_redirectAttachToFosterParent);
     210    m_head = attachToCurrent(createHTMLElement(token));
     211    m_openElements.pushHTMLHeadElement(m_head);
     212}
     213
     214void HTMLConstructionSite::insertHTMLBodyElement(AtomicHTMLToken& token)
     215{
     216    ASSERT(!m_redirectAttachToFosterParent);
     217    m_openElements.pushHTMLBodyElement(attachToCurrent(createHTMLElement(token)));
     218}
     219
     220void HTMLConstructionSite::insertHTMLElement(AtomicHTMLToken& token)
     221{
     222    m_openElements.push(attachToCurrent(createHTMLElement(token)));
     223}
     224
     225void HTMLConstructionSite::insertSelfClosingHTMLElement(AtomicHTMLToken& token)
    197226{
    198227    ASSERT(token.type() == HTMLToken::StartTag);
    199     return attach(currentElement(), createHTMLElement(token));
    200 }
    201 
    202 void HTMLConstructionSite::insertHTMLHtmlElement(AtomicHTMLToken& token)
    203 {
    204     ASSERT(!m_redirectAttachToFosterParent);
    205     m_openElements.pushHTMLHtmlElement(createHTMLElementAndAttachToCurrent(token));
    206 }
    207 
    208 void HTMLConstructionSite::insertHTMLHeadElement(AtomicHTMLToken& token)
    209 {
    210     ASSERT(!m_redirectAttachToFosterParent);
    211     m_head = createHTMLElementAndAttachToCurrent(token);
    212     m_openElements.pushHTMLHeadElement(m_head);
    213 }
    214 
    215 void HTMLConstructionSite::insertHTMLBodyElement(AtomicHTMLToken& token)
    216 {
    217     ASSERT(!m_redirectAttachToFosterParent);
    218     m_openElements.pushHTMLBodyElement(createHTMLElementAndAttachToCurrent(token));
    219 }
    220 
    221 void HTMLConstructionSite::insertHTMLElement(AtomicHTMLToken& token)
    222 {
    223     m_openElements.push(createHTMLElementAndAttachToCurrent(token));
    224 }
    225 
    226 void HTMLConstructionSite::insertSelfClosingHTMLElement(AtomicHTMLToken& token)
    227 {
    228     ASSERT(token.type() == HTMLToken::StartTag);
    229     createHTMLElementAndAttachToCurrent(token);
     228    attachToCurrent(createHTMLElement(token));
    230229    // FIXME: Do we want to acknowledge the token's self-closing flag?
    231230    // http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#acknowledge-self-closing-flag
    … …  
    245244    RefPtr<HTMLScriptElement> element = HTMLScriptElement::create(scriptTag, m_document, true);
    246245    element->setAttributeMap(token.takeAtributes(), m_fragmentScriptingPermission);
    247     m_openElements.push(attach(currentElement(), element.release()));
     246    m_openElements.push(attachToCurrent(element.release()));
    248247}
    249248
    … …  
    253252    notImplemented(); // parseError when xmlns or xmlns:xlink are wrong.
    254253
    255     RefPtr<Element> element = attach(currentElement(), createElement(token, namespaceURI));
     254    RefPtr<Element> element = attachToCurrent(createElement(token, namespaceURI));
    256255    if (!token.selfClosing())
    257256        m_openElements.push(element);
    … …  
    291290    ASSERT(element->isHTMLElement());
    292291    return element.release();
     292}
     293
     294PassRefPtr<Element> HTMLConstructionSite::createHTMLElementFromElementRecord(HTMLElementStack::ElementRecord* record)
     295{
     296    // FIXME: This will change to use
     297    // return createHTMLElementFromSavedElement(record->element());
     298    // in a later patch once tested.
     299    AtomicHTMLToken fakeToken(HTMLToken::StartTag, record->element()->localName());
     300    return createHTMLElement(fakeToken);
     301}
     302
     303namespace {
     304
     305PassRefPtr<NamedNodeMap> cloneAttributes(Element* element)
     306{
     307    NamedNodeMap* attributes = element->attributes(true);
     308    if (!attributes)
     309        return 0;
     310
     311    RefPtr<NamedNodeMap> newAttributes = NamedNodeMap::create();
     312    for (size_t i = 0; i < attributes->length(); ++i) {
     313        Attribute* attribute = attributes->attributeItem(i);
     314        RefPtr<Attribute> clone = Attribute::createMapped(attribute->name(), attribute->value());
     315        newAttributes->addAttribute(clone);
     316    }
     317    return newAttributes.release();
     318}
     319
     320}
     321
     322PassRefPtr<Element> HTMLConstructionSite::createHTMLElementFromSavedElement(Element* element)
     323{
     324    // FIXME: This method is wrong.  We should be using the original token.
     325    // Using an Element* causes us to fail examples like this:
     326    // <b id="1"><p><script>document.getElementById("1").id = "2"</script></p>TEXT</b>
     327    // When reconstructActiveFormattingElements calls this method to open
     328    // a second <b> tag to wrap TEXT, it will have id "2", even though the HTML5
     329    // spec implies it should be "1".  Minefield matches the HTML5 spec here.
     330
     331    ASSERT(element->isHTMLElement()); // otherwise localName() might be wrong.
     332    AtomicHTMLToken fakeToken(HTMLToken::StartTag, element->localName(), cloneAttributes(element));
     333    return createHTMLElement(fakeToken);
    293334}
    294335
    … …  
    320361    for (; unopenEntryIndex < m_activeFormattingElements.size(); ++unopenEntryIndex) {
    321362        HTMLFormattingElementList::Entry& unopenedEntry = m_activeFormattingElements.at(unopenEntryIndex);
    322         // FIXME: We're supposed to save the original token in the entry.
    323         AtomicHTMLToken fakeToken(HTMLToken::StartTag, unopenedEntry.element()->localName());
    324         insertHTMLElement(fakeToken);
     363        RefPtr<Element> reconstructed = createHTMLElementFromSavedElement(unopenedEntry.element());
     364        m_openElements.push(attachToCurrent(reconstructed.release()));
    325365        unopenedEntry.replaceElement(currentElement());
    326366    }

  • trunk/WebCore/html/HTMLConstructionSite.h

    r62926 r63338  
    6565
    6666    PassRefPtr<Element> createHTMLElement(AtomicHTMLToken&);
     67    PassRefPtr<Element> createHTMLElementFromElementRecord(HTMLElementStack::ElementRecord*);
    6768
    6869    void fosterParent(Node*);
    … …  
    114115    template<typename ChildType>
    115116    PassRefPtr<ChildType> attach(Node* parent, PassRefPtr<ChildType> child);
     117    PassRefPtr<Element> attachToCurrent(PassRefPtr<Element>);
    116118
    117119    void attachAtSite(const AttachmentSite&, PassRefPtr<Node> child);
    118120    void findFosterSite(AttachmentSite&);
    119121
     122    PassRefPtr<Element> createHTMLElementFromSavedElement(Element*);
    120123    PassRefPtr<Element> createElement(AtomicHTMLToken&, const AtomicString& namespaceURI);
    121124
    122     PassRefPtr<Element> createHTMLElementAndAttachToCurrent(AtomicHTMLToken&);
    123125    void mergeAttributesFromTokenIntoElement(AtomicHTMLToken&, Element*);
    124126

  • trunk/WebCore/html/HTMLTreeBuilder.cpp

    r63274 r63338  
    16761676                break;
    16771677            // 6.5
    1678             // FIXME: We're supposed to save the original token in the entry.
    1679             AtomicHTMLToken fakeToken(HTMLToken::StartTag, node->element()->localName());
    1680             // Is createHTMLElement correct? (instead of insertHTMLElement)
    1681             // Does this code ever leave newElement unattached?
    1682             RefPtr<Element> newElement = m_tree.createHTMLElement(fakeToken);
     1678            RefPtr<Element> newElement = m_tree.createHTMLElementFromElementRecord(node);
    16831679            HTMLFormattingElementList::Entry* nodeEntry = m_tree.activeFormattingElements()->find(node->element());
    16841680            nodeEntry->replaceElement(newElement.get());
    … …  
    17091705        }
    17101706        // 8
    1711         // FIXME: We're supposed to save the original token in the entry.
    1712         AtomicHTMLToken fakeToken(HTMLToken::StartTag, formattingElement->localName());
    1713         RefPtr<Element> newElement = m_tree.createHTMLElement(fakeToken);
     1707        RefPtr<Element> newElement = m_tree.createHTMLElementFromElementRecord(formattingElementRecord);
    17141708        // 9
    17151709        reparentChildren(furthestBlock->element(), newElement.get());
Note: See TracChangeset for help on using the changeset viewer.