Bug 32641 – The selected property of <option> elements is not always up to date

• Component: Forms
• Status: RESOLVED FIXED

[James Robinson]

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.

[James Robinson]

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

[Alexey Proskuryakov]

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).

[James Robinson]

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.

[Alexey Proskuryakov]

> 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).

[Darin Adler]

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*.

[Alexey Proskuryakov]

> 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.

[James Robinson]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

(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".

[James Robinson]

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.

[James Robinson]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

I'm a little concerned about diverging from IE more in some cases, but only a little.

[James Robinson]

Created attachment 46084 [details]
Addresses style nit

[WebKit Commit Bot]

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

[James Robinson]

Created attachment 46108 [details]
Removes trailing newline in HTMLOptionElement_selected3-expectation.txt

[WebKit Commit Bot]

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

[James Robinson]

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.

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

I agree, it would be nice to rephrase this a little, to make it more clear what this incorrect recalculation is about.

[Alexey Proskuryakov]

Comment on attachment 46156 [details]
Valid (this time!) patch

Let's finally get this landed though.

[WebKit Commit Bot]

Comment on attachment 46156 [details]
Valid (this time!) patch

Clearing flags on attachment: 46156

Committed r53248: <http://trac.webkit.org/changeset/53248>

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

I think this took so long to land because the commit-queue was stuck due to bug 33638!

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[James Robinson]

Failure is definitely mine, investigating.

[James Robinson]

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.

[James Robinson]

Created attachment 46633 [details]
Patch

[James Robinson]

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.

[James Robinson]

Ping?  I've confirmed that all tests pass in both debug and release mode with this patch applied.

[WebKit Commit Bot]

Comment on attachment 46633 [details]
Patch

Clearing flags on attachment: 46633

Committed r53761: <http://trac.webkit.org/changeset/53761>

[WebKit Commit Bot]

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

[Alexey Proskuryakov]

Despite the above complaints, commit bot has landed this in <http://trac.webkit.org/changeset/53761>.

[Eric Seidel (no email)]

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.