Changeset 288174 in webkit


Ignore:
Timestamp:
Jan 18, 2022 6:31:32 PM (6 months ago)
Author:
Chris Dumez
Message:

When inserting a selected <option> in a <select> element, its selected state should remain
​https://bugs.webkit.org/show_bug.cgi?id=235237

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline WPT tests that are now passing.

  • web-platform-tests/html/semantics/forms/the-select-element/inserted-or-removed-expected.txt:
  • web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt:

Source/WebCore:

When inserting a selected <option> in a <select> element, its selected state should remain and other selected
options should be de-selected.

This is as per the specification [1] that says:
"""
If the multiple attribute is absent, whenever an option element in the select element's list of options has
its selectedness set to true, and whenever an option element with its selectedness set to true is added to
the select element's list of options, the user agent must set the selectedness of all the other option
elements in its list of options to false.
"""

Firefox and Chrome correctly implement this.

WebKit was trying to implement this logic from inside HTMLOptionElement::insertedIntoAncestor(). However,
there were several issues with that:

  1. It was checking m_isSelected after calling updateValidity(). updateValidity() would call recalcListItems(), which could update m_isSelected. This part could have been addressed by saving m_isSelected before calling updateValidity() but would not have addressed the following issues.
  2. In the case where an <optgroup> containing several <option> elements is inserted into a <select>, insertedIntoAncestor() gets called from each options being inserted. When calling insertedIntoAncestor() from the first <option> and if this <option> is selected, it would deselect all following <option> elements even though insertedIntoAncestor() has not yet been called for them. As a result, we would end up selecting the first inserted <option> that had the selected state, instead of the last one.
  3. When more than one <option> is inserted at once, the current implementation would be really inefficient as every <option> would dirty and recalc the item list from insertedIntoAncestor().

To address these issues, I got rid of HTMLOptionElement::insertedIntoAncestor(). Instead we now deal with
<option> insertion from inside HTMLSelectElement::childrenChanged() and HTMLOptGroupElement::childrenChanged().
Using the parent element's childrenChanged() is useful because it only gets called once when several <option>
elements are inserted at once. I added logic to those childrenChanged() functions to keep track of the last
selected <option> element being inserted. Then, after we recalc the item list (which may change <option>s'
selected state, I make sure to this <option> is selected. This is similar to the logic that was previously
in HTMLOptionElement::insertedIntoAncestor().

[1] ​https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element

No new tests, rebaselined existing tests.

  • dom/CharacterData.cpp:

(WebCore::makeChildChange):

  • dom/ContainerNode.cpp:

(WebCore::ContainerNode::removeAllChildrenWithScriptAssertion):
(WebCore::makeChildChangeForRemoval):
(WebCore::makeChildChangeForInsertion):
(WebCore::ContainerNode::childrenChanged):
(WebCore::affectsElements): Deleted.

  • dom/ContainerNode.h:

(WebCore::ContainerNode::ChildChange::isInsertion const):
(WebCore::ContainerNode::ChildChange::affectsElements const):

  • Add 'siblingChanged' member to ChildChange in addition to the previous / next siblings. This is useful for HTMLOptGroupElement and HTMLSelectElement where we want to know if a newly inserted child is an HTMLOptionElement (and if it is selected).
  • Move affectsElements() to the header so that it can be reused by HTMLOptGroupElement and HTMLOptionElement.
  • html/HTMLOptGroupElement.cpp:

(WebCore::HTMLOptGroupElement::childrenChanged):

  • html/HTMLOptGroupElement.h:
  • html/HTMLOptionElement.cpp:

(WebCore::HTMLOptionElement::insertedIntoAncestor): Deleted.

  • html/HTMLOptionElement.h:
  • html/HTMLSelectElement.cpp:

(WebCore::HTMLSelectElement::optionToSelectFromChildChangeScope): Added.
(WebCore::HTMLSelectElement::childrenChanged):

LayoutTests:

Rebaseline WPT test that is now passing.

  • platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt:
Location:
trunk
Files:
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r288159 r288174  
     12022-01-18  Chris Dumez  <cdumez@apple.com>
     2
     3        When inserting a selected <option> in a <select> element, its selected state should remain
     4        https://bugs.webkit.org/show_bug.cgi?id=235237
     5
     6        Reviewed by Darin Adler.
     7
     8        Rebaseline WPT test that is now passing.
     9
     10        * platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt:
     11
    1122022-01-18  Alan Bujtas  <zalan@apple.com>
    213

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r288162 r288174  
     12022-01-18  Chris Dumez  <cdumez@apple.com>
     2
     3        When inserting a selected <option> in a <select> element, its selected state should remain
     4        https://bugs.webkit.org/show_bug.cgi?id=235237
     5
     6        Reviewed by Darin Adler.
     7
     8        Rebaseline WPT tests that are now passing.
     9
     10        * web-platform-tests/html/semantics/forms/the-select-element/inserted-or-removed-expected.txt:
     11        * web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt:
     12
    1132022-01-18  Chris Dumez  <cdumez@apple.com>
    214

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/inserted-or-removed-expected.txt

    r269598 r288174  
    22
    33PASS The last selected OPTION should win; Inserted by parser
    4 FAIL The last selected OPTION should win; Inserted by DOM API assert_equals: expected "Second" but got "First"
     4PASS The last selected OPTION should win; Inserted by DOM API
    55PASS The last selected OPTION should win; Inserted by jQuery append()
    66PASS The last selected OPTION should win; Inserted by innerHTML

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt

    r283525 r288174  
    55PASS Validation on selects with multiple set
    66PASS Validation on selects with non-empty disabled option
    7 FAIL Remove and add back the placeholder label option assert_false: If the placeholder label option is selected, required select element shouldn't be valid. expected false got true
     7PASS Remove and add back the placeholder label option
    88

  • trunk/LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-validity-expected.txt

    r283538 r288174  
    55PASS Validation on selects with multiple set
    66PASS Validation on selects with non-empty disabled option
    7 FAIL Remove and add back the placeholder label option assert_false: If the placeholder label option is selected, required select element shouldn't be valid. expected false got true
     7PASS Remove and add back the placeholder label option
    88

  • trunk/Source/WebCore/ChangeLog

    r288167 r288174  
     12022-01-18  Chris Dumez  <cdumez@apple.com>
     2
     3        When inserting a selected <option> in a <select> element, its selected state should remain
     4        https://bugs.webkit.org/show_bug.cgi?id=235237
     5
     6        Reviewed by Darin Adler.
     7
     8        When inserting a selected <option> in a <select> element, its selected state should remain and other selected
     9        options should be de-selected.
     10
     11        This is as per the specification [1] that says:
     12        """
     13        If the multiple attribute is absent, whenever an option element in the select element's list of options has
     14        its selectedness set to true, and whenever an option element with its selectedness set to true is added to
     15        the select element's list of options, the user agent must set the selectedness of all the other option
     16        elements in its list of options to false.
     17        """
     18
     19        Firefox and Chrome correctly implement this.
     20
     21        WebKit was trying to implement this logic from inside HTMLOptionElement::insertedIntoAncestor(). However,
     22        there were several issues with that:
     23        1. It was checking m_isSelected after calling updateValidity(). updateValidity() would call recalcListItems(),
     24           which could update m_isSelected. This part could have been addressed by saving m_isSelected before
     25           calling updateValidity() but would not have addressed the following issues.
     26        2. In the case where an <optgroup> containing several <option> elements is inserted into a <select>,
     27           insertedIntoAncestor() gets called from each options being inserted. When calling insertedIntoAncestor()
     28           from the first <option> and if this <option> is selected, it would deselect all following <option> elements
     29           even though insertedIntoAncestor() has not yet been called for them. As a result, we would end up selecting
     30           the first inserted <option> that had the selected state, instead of the last one.
     31        3. When more than one <option> is inserted at once, the current implementation would be really inefficient as
     32           every <option> would dirty and recalc the item list from insertedIntoAncestor().
     33
     34        To address these issues, I got rid of HTMLOptionElement::insertedIntoAncestor(). Instead we now deal with
     35        <option> insertion from inside HTMLSelectElement::childrenChanged() and HTMLOptGroupElement::childrenChanged().
     36        Using the parent element's childrenChanged() is useful because it only gets called once when several <option>
     37        elements are inserted at once. I added logic to those childrenChanged() functions to keep track of the last
     38        selected <option> element being inserted. Then, after we recalc the item list (which may change <option>s'
     39        selected state, I make sure to this <option> is selected. This is similar to the logic that was previously
     40        in HTMLOptionElement::insertedIntoAncestor().
     41
     42
     43        [1] https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element
     44
     45        No new tests, rebaselined existing tests.
     46
     47        * dom/CharacterData.cpp:
     48        (WebCore::makeChildChange):
     49        * dom/ContainerNode.cpp:
     50        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion):
     51        (WebCore::makeChildChangeForRemoval):
     52        (WebCore::makeChildChangeForInsertion):
     53        (WebCore::ContainerNode::childrenChanged):
     54        (WebCore::affectsElements): Deleted.
     55        * dom/ContainerNode.h:
     56        (WebCore::ContainerNode::ChildChange::isInsertion const):
     57        (WebCore::ContainerNode::ChildChange::affectsElements const):
     58        - Add 'siblingChanged' member to ChildChange in addition to the previous / next siblings.
     59          This is useful for HTMLOptGroupElement and HTMLSelectElement where we want to know
     60          if a newly inserted child is an HTMLOptionElement (and if it is selected).
     61        - Move affectsElements() to the header so that it can be reused by HTMLOptGroupElement
     62          and HTMLOptionElement.
     63
     64        * html/HTMLOptGroupElement.cpp:
     65        (WebCore::HTMLOptGroupElement::childrenChanged):
     66        * html/HTMLOptGroupElement.h:
     67        * html/HTMLOptionElement.cpp:
     68        (WebCore::HTMLOptionElement::insertedIntoAncestor): Deleted.
     69        * html/HTMLOptionElement.h:
     70        * html/HTMLSelectElement.cpp:
     71        (WebCore::HTMLSelectElement::optionToSelectFromChildChangeScope): Added.
     72        (WebCore::HTMLSelectElement::childrenChanged):
     73
    1742022-01-18  Michael Catanzaro  <mcatanzaro@gnome.org>
    275

  • trunk/Source/WebCore/dom/CharacterData.cpp

    r288069 r288174  
    7979    return {
    8080        ContainerNode::ChildChange::Type::TextChanged,
     81        nullptr,
    8182        ElementTraversal::previousSibling(characterData),
    8283        ElementTraversal::nextSibling(characterData),

  • trunk/Source/WebCore/dom/ContainerNode.cpp

    r288069 r288174  
    113113    disconnectSubframesIfNeeded(*this, DescendantsOnly);
    114114
    115     ContainerNode::ChildChange childChange { ChildChange::Type::AllChildrenRemoved, nullptr, nullptr, source };
     115    ContainerNode::ChildChange childChange { ChildChange::Type::AllChildrenRemoved, nullptr, nullptr, nullptr, source };
    116116
    117117    WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
    … …  
    153153    return {
    154154        changeType,
     155        dynamicDowncast<Element>(childToRemove),
    155156        ElementTraversal::previousSibling(childToRemove),
    156157        ElementTraversal::nextSibling(childToRemove),
    … …  
    226227{
    227228    if (replacedAllChildren == ReplacedAllChildren::Yes)
    228         return { ContainerNode::ChildChange::Type::AllChildrenReplaced, nullptr, nullptr, source };
     229        return { ContainerNode::ChildChange::Type::AllChildrenReplaced, nullptr, nullptr, nullptr, source };
    229230
    230231    auto changeType = [&] {
    … …  
    238239    return {
    239240        changeType,
     241        dynamicDowncast<Element>(child),
    240242        beforeChild ? ElementTraversal::previousSibling(*beforeChild) : ElementTraversal::lastChild(containerNode),
    241243        !beforeChild || is<Element>(*beforeChild) ? downcast<Element>(beforeChild) : ElementTraversal::nextSibling(*beforeChild),
    … …  
    827829}
    828830
    829 static bool affectsElements(const ContainerNode::ChildChange& change)
    830 {
    831     switch (change.type) {
    832     case ContainerNode::ChildChange::Type::ElementInserted:
    833     case ContainerNode::ChildChange::Type::ElementRemoved:
    834     case ContainerNode::ChildChange::Type::AllChildrenRemoved:
    835     case ContainerNode::ChildChange::Type::AllChildrenReplaced:
    836         return true;
    837     case ContainerNode::ChildChange::Type::TextInserted:
    838     case ContainerNode::ChildChange::Type::TextRemoved:
    839     case ContainerNode::ChildChange::Type::TextChanged:
    840     case ContainerNode::ChildChange::Type::NonContentsChildInserted:
    841     case ContainerNode::ChildChange::Type::NonContentsChildRemoved:
    842         return false;
    843     }
    844     ASSERT_NOT_REACHED();
    845     return false;
    846 }
    847 
    848831void ContainerNode::childrenChanged(const ChildChange& change)
    849832{
    850833    document().incDOMTreeVersion();
    851834
    852     if (affectsElements(change))
     835    if (change.affectsElements())
    853836        document().invalidateAccessKeyCache();
    854837

  • trunk/Source/WebCore/dom/ContainerNode.h

    r288069 r288174  
    8181
    8282        ChildChange::Type type;
     83        Element* siblingChanged;
    8384        Element* previousSiblingElement;
    8485        Element* nextSiblingElement;
    … …  
    103104            return false;
    104105        }
     106
     107        bool affectsElements() const
     108        {
     109            switch (type) {
     110            case ChildChange::Type::ElementInserted:
     111            case ChildChange::Type::ElementRemoved:
     112            case ChildChange::Type::AllChildrenRemoved:
     113            case ChildChange::Type::AllChildrenReplaced:
     114                return true;
     115            case ChildChange::Type::TextInserted:
     116            case ChildChange::Type::TextRemoved:
     117            case ChildChange::Type::TextChanged:
     118            case ChildChange::Type::NonContentsChildInserted:
     119            case ChildChange::Type::NonContentsChildRemoved:
     120                return false;
     121            }
     122            ASSERT_NOT_REACHED();
     123            return false;
     124        }
    105125    };
    106126    virtual void childrenChanged(const ChildChange&);

  • trunk/Source/WebCore/html/HTMLOptGroupElement.cpp

    r288069 r288174  
    7878void HTMLOptGroupElement::childrenChanged(const ChildChange& change)
    7979{
     80    bool isRelevant = change.affectsElements();
     81    RefPtr select = isRelevant ? ownerSelectElement() : nullptr;
     82    if (!isRelevant || !select) {
     83        HTMLElement::childrenChanged(change);
     84        return;
     85    }
     86
     87    auto selectOptionIfNecessaryScope = select->optionToSelectFromChildChangeScope(change, this);
     88
    8089    recalcSelectOptions();
    8190    HTMLElement::childrenChanged(change);

  • trunk/Source/WebCore/html/HTMLOptionElement.cpp

    r288033 r288174  
    316316}
    317317
    318 Node::InsertedIntoAncestorResult HTMLOptionElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
    319 {
    320     if (RefPtr select = ownerSelectElement()) {
    321         select->setRecalcListItems();
    322         select->updateValidity();
    323         // Do not call selected() since calling updateListItemSelectedStates()
    324         // at this time won't do the right thing. (Why, exactly?)
    325         // FIXME: Might be better to call this unconditionally, always passing m_isSelected,
    326         // rather than only calling it if we are selected.
    327         if (m_isSelected)
    328             select->optionSelectionStateChanged(*this, true);
    329         select->scrollToSelection();
    330     }
    331 
    332     return HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
    333 }
    334 
    335318String HTMLOptionElement::collectOptionInnerText() const
    336319{

  • trunk/Source/WebCore/html/HTMLOptionElement.h

    r287363 r288174  
    6262
    6363    void setSelectedState(bool);
     64    bool selectedWithoutUpdate() const { return m_isSelected; }
    6465
    6566private:
    … …  
    7273    void parseAttribute(const QualifiedName&, const AtomString&) final;
    7374
    74     InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;
    7575    bool accessKeyAction(bool) final;
    7676

  • trunk/Source/WebCore/html/HTMLSelectElement.cpp

    r288033 r288174  
    372372}
    373373
     374CompletionHandlerCallingScope HTMLSelectElement::optionToSelectFromChildChangeScope(const ContainerNode::ChildChange& change, HTMLOptGroupElement* parentOptGroup)
     375{
     376    if (multiple())
     377        return { };
     378
     379    auto getLastSelectedOption = [](HTMLOptGroupElement& optGroup) -> HTMLOptionElement* {
     380        for (auto* option = Traversal<HTMLOptionElement>::lastChild(optGroup); option; option = Traversal<HTMLOptionElement>::previousSibling(*option)) {
     381            if (option->selectedWithoutUpdate())
     382                return option;
     383        }
     384        return nullptr;
     385    };
     386
     387    RefPtr<HTMLOptionElement> optionToSelect;
     388    if (change.type == ChildChange::Type::ElementInserted) {
     389        if (is<HTMLOptionElement>(change.siblingChanged)) {
     390            auto& option = downcast<HTMLOptionElement>(*change.siblingChanged);
     391            if (option.selectedWithoutUpdate())
     392                optionToSelect = &option;
     393        } else if (!parentOptGroup && is<HTMLOptGroupElement>(change.siblingChanged))
     394            optionToSelect = getLastSelectedOption(*downcast<HTMLOptGroupElement>(change.siblingChanged));
     395    } else if (parentOptGroup && change.type == ContainerNode::ChildChange::Type::AllChildrenReplaced)
     396        optionToSelect = getLastSelectedOption(*parentOptGroup);
     397
     398    return CompletionHandlerCallingScope { [optionToSelect = WTFMove(optionToSelect), isInsertion = change.isInsertion(), select = Ref { *this }] {
     399        if (optionToSelect)
     400            select->optionSelectionStateChanged(*optionToSelect, true);
     401        else if (isInsertion)
     402            select->scrollToSelection();
     403    } };
     404}
     405
    374406void HTMLSelectElement::childrenChanged(const ChildChange& change)
    375407{
     408    if (!change.affectsElements()) {
     409        HTMLFormControlElementWithState::childrenChanged(change);
     410        return;
     411    }
     412
     413    auto selectOptionIfNecessaryScope = optionToSelectFromChildChangeScope(change);
     414
    376415    setRecalcListItems();
    377416    updateValidity();

  • trunk/Source/WebCore/html/HTMLSelectElement.h

    r287529 r288174  
    2828#include "HTMLFormControlElementWithState.h"
    2929#include "TypeAhead.h"
     30#include <wtf/CompletionHandler.h>
    3031
    3132namespace WebCore {
    … …  
    105106    void optionSelectionStateChanged(HTMLOptionElement&, bool optionIsSelected);
    106107    bool allowsNonContiguousSelection() const { return m_allowsNonContiguousSelection; };
     108
     109    CompletionHandlerCallingScope optionToSelectFromChildChangeScope(const ChildChange&, HTMLOptGroupElement* parentOptGroup = nullptr);
    107110
    108111protected:
Note: See TracChangeset for help on using the changeset viewer.