The 'selected' property of an <option> inside of a <select> is updated by SelectElement::recalcListItems(). This is called on certain property accesses of the <select> (like selectedIndex) but when the form is created by normal document parsing then it is only called as a side effect of recalcStyle() - see https://bugs.webkit.org/show_bug.cgi?id=15088. The test that 'selected' is up to date (fast/forms/HTMLOptionElement_selected.html) passes currently because the test code runs in a 'load' event handler, which dispatches after the 'DOMContentLoaded' event. Node::dispatchGenericEvent() makes a call to Document::updateStyleForAllDocuments() so the 'selected' property of the <option> element is up to date by the time the test code runs. If the property is queried before or during the DOMContentLoaded event dispatch, or if the call to Document::updateStyleForAllDocuments() in Node::dispatchGenericEvent() is removed (see https://bugs.webkit.org/show_bug.cgi?id=32580) then this test fails.
Created attachment 45024 [details] Test, shows the incorrect behavior The 'selected' attribute should be 'true' on both queries. However it ToT currently it is 'false' on the first access since the <option>'s selected state is not updated until the query for selectedIndex
Created attachment 45839 [details] test case as HTML Same text as plain HTML for easier verification (also, tweaked a bit to make it work in Firefox).
Created attachment 45852 [details] Calls recalcListItems() when the "</select>" is parsed to match Firefox behavior This patch causes the list items and selected states to be updated when the "</select>" is parsed, which matches Firefox's behavior. To verify this run the test case in Firefox and move the <script> block to be immediately before the </select>. Doing this in the finishedParsingChildren() call is a bit weird, but I think the current behavior is even weirder and clearly wrong in some cases.
> To verify this run > the test case in Firefox and move the <script> block to be immediately before > the </select>. Ideally, we'd have that landed as a regression test, too. Even if we're not positive that we want to implement some quirks, it helps to know it when behavior changes unexpectedly (test text should clearly state when it's the case though).
Comment on attachment 45852 [details] Calls recalcListItems() when the "</select>" is parsed to match Firefox behavior > + if (m_data.shouldRecalcListItems()) > + SelectElement::recalcListItems(m_data, this); Indentation is wrong. > + // Recalculate list items and selected state when parsing the </select> to match Firefox. > + virtual void finishParsingChildren(); Can you test cases involving DOM manipulation to prove that this really needs to be triggered by parsing? It's OK to do this if we must, but not if it's the wrong rule. What about IE? Does our behavior match theirs? I suggest making the override of the virtual function private, since it probably would be a programming error to call this on a HTMLSelectElement*.
> Can you test cases involving DOM manipulation to prove that this really needs > to be triggered by parsing? It's OK to do this if we must, but not if it's the > wrong rule. The test that we made was: try accessing the value from a script that's either before or after "</select>". Firefox gave different results in these cases. This is why I asked to land both cases as regression tests.
I don't think Firefox's or WebKit's behavior are very good before the </select> is hit currently. In Firefox, queries for the first option's 'selected' are false for the first <option> and the select's selectedIndex is '-1' before the </select>. After the </select> the first option's 'selected' attribute becomes true and selectedIndex becomes '0'. WebKit behavior is a bit weirder - the 'selected' attribute is false until an event dispatches (any event), the 'selectedIndex' attribute is queried, layout is forced in any way (say by querying a derived attribute of any element from javascript, or parsing a <style>). This patch doesn't fix everything but it does ensure that after a <select> is fully parsed the 'selected' attribute is coherent. I do not think that testing before the </select> is very useful since the current behavior in Firefox for this case is just plain wrong.
It's fine to test for behavior that's not necessarily perfect - at least, we'll make sure that this code path doesn't crash. Also, as I already mentioned, knowing about unexpected changes is good. I agree with Darin that knowing more about IE behavior would be good.
(In reply to comment #8) > It's fine to test for behavior that's not necessarily perfect - at least, we'll > make sure that this code path doesn't crash. Also, as I already mentioned, > knowing about unexpected changes is good. I strongly agree with these remarks. It's good to have test cases even if we may want to change behavior later. The creators of the test cases should just do their best to make clear they are not "defining correct behavior".
OK, I will do the following: 1.) Investigate what IE does. 2.) Create a test case for what we do prior to the </select> and document what both Firefox and IE do on this case. I don't think we necessarily want to match either of them but I see that a test case will be helpful regardless. I'll update the patch once I have done that. Thanks for the feedback.
Created attachment 46080 [details] Much better patch This patch adds a check in the OptionElement::selected() getter and recalculates everything if items are out of date. It also adds two new tests, one that checks selected and selectedIndex right after the </select> and one right before it. On HTMLOptionElement_selected2.html, we previously did not match FFx 3.5 or IE8. With this patch we match them both (with the correct behavior). On HTMLOptionElement_selected3.html, we previously matched IE8 but had a very weird behavior (the 'selected' attribute was out of date until 'selectedIndex' was queried). FFx 3.5 completely chokes on this test (it shows 'selected' as false on every <option> and shows 'selectedIndex' as -1 until the </select> is parsed). With this patch we show the correct values for 'selected' and 'selectedIndex' before the </select> which seems like the most sane behavior. We also now match FFx 3.5 on fast/forms/add-remove-option-modification-event.html, which was previously marked as an expected failure. Plus, this patch doesn't depend on recalcStyle() side effects.
Comment on attachment 46080 [details] Much better patch > static void menuListDefaultEventHandler(SelectElementData&, Element*, Event*, HTMLFormElement*); > static void listBoxDefaultEventHandler(SelectElementData&, Element*, Event*, HTMLFormElement*); > + static void setOptionsChangedOnRenderer(SelectElementData& data, Element* element); Argument names are not needed here. r=me
I'm a little concerned about diverging from IE more in some cases, but only a little.
Created attachment 46084 [details] Addresses style nit
Comment on attachment 46084 [details] Addresses style nit Rejecting patch 46084 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11889 test cases. fast/forms/HTMLOptionElement_selected3.html -> failed Exiting early after 1 failures. 6380 tests run. 96.02s total testing time 6379 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 2 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/166678
Created attachment 46108 [details] Removes trailing newline in HTMLOptionElement_selected3-expectation.txt
Comment on attachment 46108 [details] Removes trailing newline in HTMLOptionElement_selected3-expectation.txt Rejecting patch 46108 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Alexey Proskuryakov', '--force']" exit_code: 2 Last 500 characters of output: ment_selected2-expected.txt patching file LayoutTests/fast/forms/HTMLOptionElement_selected2.html patching file LayoutTests/fast/forms/HTMLOptionElement_selected3-expected.txt patch: **** malformed patch at line 13: fatal: pathspec 'LayoutTests/fast/forms/HTMLOptionElement_selected3-expected.txt' did not match any files Failed to git add LayoutTests/fast/forms/HTMLOptionElement_selected3-expected.txt. at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 466, <> line 380. Full output: http://webkit-commit-queue.appspot.com/results/166744
Created attachment 46156 [details] Valid (this time!) patch Sorry about all the churn, guys. Apparently that last patch was malformed. I've uploaded a new patch generated from a clean SVN repo so hopefully all is good now.
Comment on attachment 46156 [details] Valid (this time!) patch I'm not sure I understand this comment: 5 // Avoid our selected() getter since it will recalculate list items incorrectly for us. 226 if (m_data.selected()) I assume it's time based. That it would recalculate them incorrectly at that particular point in the code? If so, do we need some ASSERTS to make sure someone doesn't call selected() at the worng plaes? In general this looks good. Would just like to hear the answer to the above question.
I agree, it would be nice to rephrase this a little, to make it more clear what this incorrect recalculation is about.
Comment on attachment 46156 [details] Valid (this time!) patch Let's finally get this landed though.
Comment on attachment 46156 [details] Valid (this time!) patch Clearing flags on attachment: 46156 Committed r53248: <http://trac.webkit.org/changeset/53248>
Comment on attachment 46156 [details] Valid (this time!) patch Rejecting patch 46156 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '46156']" exit_code: 1 Last 500 characters of output: ache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_mechanize.py", line 505, in select_form File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 546, in __getattr__ File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 559, in forms File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 228, in forms mechanize._html.ParseError
(In reply to comment #23) > (From update of attachment 46156 [details]) > Rejecting patch 46156 from commit-queue. > > Unexpected failure when landing patch! Please file a bug against webkit-patch. Filed bug 33659. I'm not sure how all these attachments ended up with only cq- and no r flag.
Did this really sit in the queue for a day without getting landed? I'm confused by the history: https://bugs.webkit.org/show_activity.cgi?id=32641 Did this end up landed by hand because the commit-queue was giving you trouble? Or just because. It's *totally fine* of course that it was landed by hand, but I just want to make sure if there are commit-queeu bugs which need fixing that I know about them. Thanks!
Oh, nevermind. I misread above. The commit-queue *did* land this. It was not landed by hand. The commit-queue just also pooped on this bug after landing for unknown reasons. I've filed bug 33659 about the commit-queue's strange behavior post-landing. Sorry for all the noise!
I think this took so long to land because the commit-queue was stuck due to bug 33638!
This caused tests to start crashing on multiple bots. I'm so sorry the commit-queue was stuck today and thus this got landed so much later than when you set the flag! You can see the crashes here: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r53248%20(9248)/results.html Looks like an ASSERT: ASSERTION FAILED: !m_optionsChanged (/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/rendering/RenderListBox.cpp:165 virtual void WebCore::RenderListBox::calcPrefWidths()) Rolling out the change for now.
webkit-patch failed to leave a comment on this bug due to bug 33659. But I rolled this out in: http://trac.webkit.org/changeset/53252
Failure is definitely mine, investigating.
OK, the problem was that some of the element->setNeedsStyleRecalc() calls had to stay. They aren't used to actually recalculate any styles, but instead they result in HTMLFormControlElement::recalcStyle() being called which posts a task to call updateFromElement() on the element's RenderObject. I think this is wrong and want to fix it in a follow up patch but since this patch fixes other bugs I'll add the setNeedsStyleRecalc() calls back in for now. This is kind of a mess. What happens is: *) Something notices a change but can't handle it in the current callstack so it calls element->setNeedsStyleRecalc() as a way to get a callback sometime in the future (but not too far out) *) The recalcStyle() override runs, but it also can't handle the update in its current callstack so it calls queuePostAttachCallback() with another callback *) This callback actually handles the state change (by calling updateFromElement() *) The implementation of updateFromElement() ends up marking the element as needing layout. Yucky.
Created attachment 46633 [details] Patch
Comment on attachment 46633 [details] Patch The only difference between this and the last patch is that this one leaves in the element->setNeedsRecalcStyle() calls in SelectedElement::reset() and SelectedElement::typeAheadFind(), which are needed so that RenderObject::updateFromElement() is (eventually) called. I agree that the comment in HTMLOptionElement::insertedIntoTree() is a bit confused. The function depends on the (possibly stale) selected state cached on the m_data field, it's incorrect for this function to cause an immediately recalculation of the list items. I'm not sure how to phrase this in a less confusing way but I am hopeful I'll be able to clean this bit to be less dodgy looking.
Ping? I've confirmed that all tests pass in both debug and release mode with this patch applied.
Comment on attachment 46633 [details] Patch Clearing flags on attachment: 46633 Committed r53761: <http://trac.webkit.org/changeset/53761>
Comment on attachment 46633 [details] Patch Rejecting patch 46633 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '46633']" exit_code: 1 Last 500 characters of output: ache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_mechanize.py", line 505, in select_form File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 546, in __getattr__ File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 559, in forms File "./autoinstall.cache.d/-1555206040/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_html.py", line 228, in forms mechanize._html.ParseError
Despite the above complaints, commit bot has landed this in <http://trac.webkit.org/changeset/53761>.
Sorry, this is bug 33659. The fix is simple, we just need to do it. The commit-queue will properly land, but fail to actually post to bugzilla on any bug with html tags in it (which is a lot of them). In this case, I expect the </select> in one of the attachment names triggered this bug.