Changeset 121128 in webkit

Timestamp:

Jun 24, 2012, 6:47:35 PM (14 years ago)

Author:

tkent@chromium.org

Message:

Selected option is not restored correctly
​https://bugs.webkit.org/show_bug.cgi?id=89623

Reviewed by Hajime Morita.

Source/WebCore:

Details of the bug:
We saved a state of a <select> element as a string of which length was
the size of <select>'s children. e.g. If a <select> had five children
and the second and the fifth items were selected, the state string was:

".X..X"

This didn't work well if the structure of the children was updated after
parsing. For example,

1. A page has the following <select> initially: <select multiple>

<option>Banana
<option>Lemon
<option>Orange
<option>Strawberry

<select>

2. For some reasons, <option>Apple</option> is prepended to the children.
3. Some items are selected.
4. The page is unloaded. Selection state is saved.
5. A user go back to the page again. A browser parses the page again.
6. Try to restore the <select> state with the saved data at 4. But "Apple" is missing. The <select> has wrong selections.

Solution:
We save the state as a set of selected values. If "Banana" and
"Strawberry" are selected in the above <select>, we save two strings;
"Banana" and "Strawberry", not ".X..X".

Test: fast/forms/select/select-state-restore.html

• html/HTMLSelectElement.cpp:

(WebCore::HTMLSelectElement::saveFormControlState):
Store selected value strings to a FormControlState object.
(WebCore::HTMLSelectElement::searchOptionsForValue):
A helper function to find an <option> with the specified value.
(WebCore::HTMLSelectElement::restoreFormControlState):
Clear all of selections, then select options with saved values.
In order to avoid O(M x N) loop, we start searching at position we found
the previous value.

• html/HTMLSelectElement.h: Declare searchOptionsForValue.
• html/FormController.cpp:

(formStateSignature): Bump up the version because this is a incompatible
change.

LayoutTests:

• fast/forms/select/select-state-restore-expected.txt: Added.
• fast/forms/select/select-state-restore.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/select/select-state-restore-expected.txt (added)
• LayoutTests/fast/forms/select/select-state-restore.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/FormController.cpp (modified) (1 diff)
• Source/WebCore/html/HTMLSelectElement.cpp (modified) (2 diffs)
• Source/WebCore/html/HTMLSelectElement.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-06-21  Kent Tamura  <tkent@chromium.org>
+
+        Selected option is not restored correctly
+        https://bugs.webkit.org/show_bug.cgi?id=89623
+
+        Reviewed by Hajime Morita.
+
+        * fast/forms/select/select-state-restore-expected.txt: Added.
+        * fast/forms/select/select-state-restore.html: Added.
+
 2012-06-24  David Barr  <davidbarr@chromium.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-06-22  Kent Tamura  <tkent@chromium.org>
+
+        Selected option is not restored correctly
+        https://bugs.webkit.org/show_bug.cgi?id=89623
+
+        Reviewed by Hajime Morita.
+
+        Details of the bug:
+        We saved a state of a <select> element as a string of which length was
+        the size of <select>'s children. e.g. If a <select> had five children
+        and the second and the fifth items were selected, the state string was:
+                ".X..X"
+
+        This didn't work well if the structure of the children was updated after
+        parsing. For example,
+        1. A page has the following <select> initially:
+          <select multiple>
+            <option>Banana
+            <option>Lemon
+            <option>Orange
+            <option>Strawberry
+          <select>
+
+        2. For some reasons, <option>Apple</option> is prepended to the children.
+        3. Some items are selected.
+        4. The page is unloaded. Selection state is saved.
+        5. A user go back to the page again. A browser parses the page again.
+        6. Try to restore the <select> state with the saved data at 4.
+          But "Apple" is missing. The <select> has wrong selections.
+
+        Solution:
+        We save the state as a set of selected values. If "Banana" and
+        "Strawberry" are selected in the above <select>, we save two strings;
+        "Banana" and "Strawberry", not ".X..X".
+
+        Test: fast/forms/select/select-state-restore.html
+
+        * html/HTMLSelectElement.cpp:
+        (WebCore::HTMLSelectElement::saveFormControlState):
+        Store selected value strings to a FormControlState object.
+        (WebCore::HTMLSelectElement::searchOptionsForValue):
+        A helper function to find an <option> with the specified value.
+        (WebCore::HTMLSelectElement::restoreFormControlState):
+        Clear all of selections, then select options with saved values.
+        In order to avoid O(M x N) loop, we start searching at position we found
+        the previous value.
+        * html/HTMLSelectElement.h: Declare searchOptionsForValue.
+        * html/FormController.cpp:
+        (formStateSignature): Bump up the version because this is a incompatible
+        change.
+
 2012-06-24  David Barr  <davidbarr@chromium.org>
 

trunk/Source/WebCore/html/FormController.cpp

@@ -82 +82 @@
     // attribute value of a form control. The following string literal should
     // contain some characters which are rarely used for name attribute values.
-    DEFINE_STATIC_LOCAL(String, signature, ("\n\r?% WebKit serialized form state version 2 \n\r=&"));
+    DEFINE_STATIC_LOCAL(String, signature, ("\n\r?% WebKit serialized form state version 3 \n\r=&"));
     return signature;
 }

trunk/Source/WebCore/html/HTMLSelectElement.cpp

@@ -920 +920 @@
     const Vector<HTMLElement*>& items = listItems();
     size_t length = items.size();
-    StringBuilder builder;
-    builder.reserveCapacity(length);
+    FormControlState state;
     for (unsigned i = 0; i < length; ++i) {
-        HTMLElement* element = items[i];
-        bool selected = element->hasTagName(optionTag) && toHTMLOptionElement(element)->selected();
-        builder.append(selected ? 'X' : '.');
-    }
-    return FormControlState(builder.toString());
+        if (!items[i]->hasTagName(optionTag))
+            continue;
+        HTMLOptionElement* option = toHTMLOptionElement(items[i]);
+        if (!option->selected())
+            continue;
+        state.append(option->value());
+        if (!multiple())
+            break;
+    }
+    return state;
+}
+
+size_t HTMLSelectElement::searchOptionsForValue(const String& value, size_t listIndexStart, size_t listIndexEnd) const
+{
+    const Vector<HTMLElement*>& items = listItems();
+    size_t loopEndIndex = std::min(items.size(), listIndexEnd);
+    for (size_t i = listIndexStart; i < loopEndIndex; ++i) {
+        if (!items[i]->hasLocalName(optionTag))
+            continue;
+        if (static_cast<HTMLOptionElement*>(items[i])->value() == value)
+            return i;
+    }
+    return notFound;
 }
 
@@ -935 +952 @@
 
     const Vector<HTMLElement*>& items = listItems();
-    size_t length = items.size();
-
-    String mask = state[0];
-    for (size_t i = 0; i < length; ++i) {
-        HTMLElement* element = items[i];
-        if (element->hasTagName(optionTag))
-            toHTMLOptionElement(element)->setSelectedState(mask[i] == 'X');
+    size_t itemsSize = items.size();
+    if (!itemsSize)
+        return;
+
+    for (size_t i = 0; i < itemsSize; ++i) {
+        if (!items[i]->hasLocalName(optionTag))
+            continue;
+        static_cast<HTMLOptionElement*>(items[i])->setSelectedState(false);
+    }
+
+    if (!multiple()) {
+        size_t foundIndex = searchOptionsForValue(state[0], 0, itemsSize);
+        if (foundIndex != notFound)
+            toHTMLOptionElement(items[foundIndex])->setSelectedState(true);
+    } else {
+        size_t startIndex = 0;
+        for (size_t i = 0; i < state.valueSize(); ++i) {
+            const String& value = state[i];
+            size_t foundIndex = searchOptionsForValue(value, startIndex, itemsSize);
+            if (foundIndex == notFound)
+                foundIndex = searchOptionsForValue(value, 0, startIndex);
+            if (foundIndex == notFound)
+                continue;
+            toHTMLOptionElement(items[foundIndex])->setSelectedState(true);
+            startIndex = foundIndex + 1;
+        }
     }
 

trunk/Source/WebCore/html/HTMLSelectElement.h

@@ -165 +165 @@
     void listBoxDefaultEventHandler(Event*);
     void setOptionsChangedOnRenderer();
+    size_t searchOptionsForValue(const String&, size_t listIndexStart, size_t listIndexEnd) const;
 
     enum SkipDirection {