- Revision
- 121128
- Author
- [email protected]
- Date
- 2012-06-24 18:47:35 -0700 (Sun, 24 Jun 2012)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (121127 => 121128)
--- trunk/LayoutTests/ChangeLog 2012-06-25 01:46:14 UTC (rev 121127)
+++ trunk/LayoutTests/ChangeLog 2012-06-25 01:47:35 UTC (rev 121128)
@@ -1,3 +1,13 @@
+2012-06-21 Kent Tamura <[email protected]>
+
+ 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 <[email protected]>
Add snap to css3-images image-resolution
Added: trunk/LayoutTests/fast/forms/select/select-state-restore-expected.txt (0 => 121128)
--- trunk/LayoutTests/fast/forms/select/select-state-restore-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/select/select-state-restore-expected.txt 2012-06-25 01:47:35 UTC (rev 121128)
@@ -0,0 +1,14 @@
+Test if selected options are correctly restored even if their positions were changed.
+
+PASS $("opt-la").selected is false
+PASS $("opt-tk").selected is false
+PASS $("opt-os").selected is true
+PASS $("opt-ak").selected is false
+PASS $("opt-wy").selected is false
+We don't care which one is selected because their values are identical:
+PASS !!($("opt-ca-1").selected ^ $("opt-ca-2").selected) is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
Property changes on: trunk/LayoutTests/fast/forms/select/select-state-restore-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/forms/select/select-state-restore.html (0 => 121128)
--- trunk/LayoutTests/fast/forms/select/select-state-restore.html (rev 0)
+++ trunk/LayoutTests/fast/forms/select/select-state-restore.html 2012-06-25 01:47:35 UTC (rev 121128)
@@ -0,0 +1,69 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<script src=""
+</head>
+<body>
+<p>Test if selected options are correctly restored even if their positions were changed.</p>
+<div id="console"></div>
+
+<input id="emptyOnFirstVisit">
+<div id="parent">
+<form action="" id=form1>
+<select id="select1" multiple>
+ <option id="opt-ca-1" value="CA">California 1</option>
+ <option id="opt-ca-2" value="CA" selected>California 2</option>
+ <option id="opt-la">LA</option>
+ <optgroup>
+ <option id="opt-tk" selected>Tokyo</option>
+ <option id="opt-os">Osaka</option>
+ </optgroup>
+ <option id="opt-ak" value="AK">Alaska</option>
+ <option id="opt-wy" selected>WY</option>
+</select>
+</form>
+</div>
+
+<script>
+
+jsTestIsAsync = true;
+
+function runTest()
+{
+ var state = document.getElementById('emptyOnFirstVisit');
+ if (!state.value) {
+ // First visit.
+ setTimeout(function() {
+ state.value = 'visited';
+ $('opt-tk').selected = false;
+ $('opt-os').selected = true;
+ $('select1').insertBefore($('opt-ak'), $('opt-ca-1'));
+ $('select1').removeChild($('opt-wy'));
+ var newOption = document.createElement('option');
+ newOption.textContent = 'NY';
+ $('select1').appendChild(newOption);
+ newOption.selected = true;
+ $('form1').submit();
+ }, 0);
+ } else {
+ // Went back to this page again, and form state should be restored.
+ shouldBeFalse('$("opt-la").selected');
+ shouldBeFalse('$("opt-tk").selected');
+ shouldBeTrue('$("opt-os").selected');
+ shouldBeFalse('$("opt-ak").selected');
+ shouldBeFalse('$("opt-wy").selected');
+ debug('We don\'t care which one is selected because their values are identical:');
+ shouldBeTrue('!!($("opt-ca-1").selected ^ $("opt-ca-2").selected)');
+
+ $('parent').innerHTML = '';
+ setTimeout(function() {
+ finishJSTest();
+ }, 0);
+ }
+}
+
+runTest();
+</script>
+<script src=""
+</body>
Property changes on: trunk/LayoutTests/fast/forms/select/select-state-restore.html
___________________________________________________________________
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (121127 => 121128)
--- trunk/Source/WebCore/ChangeLog 2012-06-25 01:46:14 UTC (rev 121127)
+++ trunk/Source/WebCore/ChangeLog 2012-06-25 01:47:35 UTC (rev 121128)
@@ -1,3 +1,54 @@
+2012-06-22 Kent Tamura <[email protected]>
+
+ 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 <[email protected]>
Add snap to css3-images image-resolution
Modified: trunk/Source/WebCore/html/FormController.cpp (121127 => 121128)
--- trunk/Source/WebCore/html/FormController.cpp 2012-06-25 01:46:14 UTC (rev 121127)
+++ trunk/Source/WebCore/html/FormController.cpp 2012-06-25 01:47:35 UTC (rev 121128)
@@ -81,7 +81,7 @@
// In the legacy version of serialized state, the first item was a name
// 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;
}
Modified: trunk/Source/WebCore/html/HTMLSelectElement.cpp (121127 => 121128)
--- trunk/Source/WebCore/html/HTMLSelectElement.cpp 2012-06-25 01:46:14 UTC (rev 121127)
+++ trunk/Source/WebCore/html/HTMLSelectElement.cpp 2012-06-25 01:47:35 UTC (rev 121128)
@@ -919,30 +919,66 @@
{
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' : '.');
+ if (!items[i]->hasTagName(optionTag))
+ continue;
+ HTMLOptionElement* option = toHTMLOptionElement(items[i]);
+ if (!option->selected())
+ continue;
+ state.append(option->value());
+ if (!multiple())
+ break;
}
- return FormControlState(builder.toString());
+ 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;
+}
+
void HTMLSelectElement::restoreFormControlState(const FormControlState& state)
{
recalcListItems();
const Vector<HTMLElement*>& items = listItems();
- size_t length = items.size();
+ size_t itemsSize = items.size();
+ if (!itemsSize)
+ return;
- 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');
+ 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;
+ }
+ }
+
setOptionsChangedOnRenderer();
setNeedsValidityCheck();
}
Modified: trunk/Source/WebCore/html/HTMLSelectElement.h (121127 => 121128)
--- trunk/Source/WebCore/html/HTMLSelectElement.h 2012-06-25 01:46:14 UTC (rev 121127)
+++ trunk/Source/WebCore/html/HTMLSelectElement.h 2012-06-25 01:47:35 UTC (rev 121128)
@@ -164,6 +164,7 @@
bool platformHandleKeydownEvent(KeyboardEvent*);
void listBoxDefaultEventHandler(Event*);
void setOptionsChangedOnRenderer();
+ size_t searchOptionsForValue(const String&, size_t listIndexStart, size_t listIndexEnd) const;
enum SkipDirection {
SkipBackwards = -1,
