Bug 27756

Created attachment 33604 [details] Support for HTMLInputElement::list and HTMLInputElement::selectedOption. --- 10 files changed, 274 insertions(+), 1 deletions(-)

Comment on attachment 33604 [details] Support for HTMLInputElement::list and HTMLInputElement::selectedOption. Normally we don't commit commented-out code: 1738 //case DATETIME: 1739 //case DATE: 1740 //case MONTH: 1741 //case WEEK: 1742 //case TIME: 1743 //case DATETIMELOCAL: Why does this need to be PassRefPtr? 1727 PassRefPtr<HTMLElement> HTMLInputElement::list() your'e not returning a newly allocated list. This is very very bad: 1751 return static_cast<HTMLElement*>(element.get()); That would normally crash. Grabbing a pointer out of a RefPtr and returning it is not ok. :( .release() is what you wanted there. But again, I don't think we need this to be a PassRefPtr, or? Also probably doesn't need to be PassRefPtr: 1770 PassRefPtr<HTMLOptionElement> HTMLInputElement::selectedOption() WK style violation: if (element && element->isHTMLElement() && element->hasTagName(datalistTag)) { 1751 return static_cast<HTMLElement*>(element.get()); 1752 }

Thanks for the comments. I have misunderstood the usage of PassRefPtr. I'll update the patch. BTW, this patch depends on the patch in https://bugs.webkit.org/show_bug.cgi?id=26915 . Would you take a look at it?

Created attachment 33694 [details] Support for HTMLInputElement::list and HTMLInputElement::selectedOption. --- 10 files changed, 265 insertions(+), 1 deletions(-)

Comment on attachment 33694 [details] Support for HTMLInputElement::list and HTMLInputElement::selectedOption. You can just check here for dataListTag and cast directly to an HTMLDataListElement: 742 if (element && element->isHTMLElement() && element->hasTagName(datalistTag)) 1743 return static_cast<HTMLElement*>(element) Not required but slightly cleaner. isEmpty() checks null: 697 m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty(); This sseems like the long way around: bool hadNonEmptyList = m_hasNonEmptyList; 697 m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty(); 698 if (hadNonEmptyList != m_hasNonEmptyList && attached()) { 699 // Re-create a renderer because m_hasNonEmptyList affects UI. 700 detach(); 701 attach(); 702 } Do you really need to attach/detach every time? Can't you do a layout or a style recalc? What does the list do to the renderer? list() clearly should return a HTMLDataListElement*, or? Otherwise you certainly shouldn't be blindly casting to one in callers of list()

Thank you for the review. (In reply to comment #5) > (From update of attachment 33694 [details]) > You can just check here for dataListTag and cast directly to an > HTMLDataListElement: done. > isEmpty() checks null: > 697 m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty(); done. > This sseems like the long way around: > bool hadNonEmptyList = m_hasNonEmptyList; > 697 m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty(); > 698 if (hadNonEmptyList != m_hasNonEmptyList && attached()) { > 699 // Re-create a renderer because m_hasNonEmptyList affects UI. > 700 detach(); > 701 attach(); > 702 } > > Do you really need to attach/detach every time? Can't you do a layout or a > style recalc? What does the list do to the renderer? The list attribute will creates a shadow child of the input element. I just followed the code for resultsAttr above in this method. RenderTextControlSingleLine doesn't have a feature to remove a shadow child and re-layout. So detach()&attach() was a simple solution. Anyway, I have removed this code because we don't have the list UI for now. I'll re-implement another way in another patch. > list() clearly should return a HTMLDataListElement*, or? Otherwise you > certainly shouldn't be blindly casting to one in callers of list() HTML5 defines that `list' returns HTMLElement. According to Hixie, we might add other list source elements in the future.

Created attachment 33863 [details] Proposed patch (rev.3) Note: this patch depends on the patch of https://bugs.webkit.org/show_bug.cgi?id=26915

dhyatt was working on <datalist> support, he should be CCed on all bugs about it

Comment on attachment 33863 [details] Proposed patch (rev.3) This is not safe: 1811 RefPtr<HTMLCollection> options = static_cast<HTMLDataListElement*>(sourceElement)->options(); (at least not if we add more list() types). I think until we add more list() types we could just make it return HTMLDataListElement*. But checking at each callsite is OK too. Seems this needs to be guarded by DATAGRID defines, no? Or do we have a DataList? This doesn't look like a complete implementation, or? I can't remember if you're a committer yet or not. so r- assuming you'll need someone else to land after you fix those nits. This doesn't seem very useful w/o UI... but this is OK to land for now if it's guarded by and ENABLE.

(In reply to comment #9) Thanks for the comment. > (From update of attachment 33863 [details]) > This is not safe: > 1811 RefPtr<HTMLCollection> options = > static_cast<HTMLDataListElement*>(sourceElement)->options(); > (at least not if we add more list() types). I think until we add more list() > types we could just make it return HTMLDataListElement*. But checking at each > callsite is OK too. Ok, will do. > Seems this needs to be guarded by DATAGRID defines, no? Or do we have a > DataList? This doesn't look like a complete implementation, or? This patch is not related to datagird though the part 1 (#26915) is. I think it is reasonable to guard by another flag such as ENABLE(INPUT_LIST). I'll update the patch after the part 1 is landed.

Created attachment 35032 [details] Proposed patch (rev.4) - The new code is guarded by ENABLE(DATALIST) - Change the type of HTMLInputElement::list; HTMLElement -> HTMLDataListElement

Comment on attachment 35032 [details] Proposed patch (rev.4) Looks OK. I'm not sure if svn-apply will handle this diff correctly or not. Assuming not and marking cq-.

(The ChangeLog bit is the part of the diff I worry about svn-apply handling wrong.)

Created attachment 35087 [details] Proposed patch (rev.5) Commit-queue-friendly patch :-)

Comment on attachment 35087 [details] Proposed patch (rev.5) Commit queue log from the future! Applying 35087 from bug 27756. patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/forms/input-list-expected.txt patching file LayoutTests/fast/forms/input-list.html patching file LayoutTests/fast/forms/input-selectedoption-expected.txt patching file LayoutTests/fast/forms/input-selectedoption.html patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/html/HTMLAttributeNames.in patching file WebCore/html/HTMLInputElement.cpp patching file WebCore/html/HTMLInputElement.h patching file WebCore/html/HTMLInputElement.idl Fetching http://build.webkit.org/one_box_per_builder Running build-webkit Running build-dumprendertree Running tests from /Users/eseidel/Projects/WebKit2/LayoutTests Testing 11118 test cases. transitions/extra-transition.html -> failed Exiting early after 1 failures. 11013 tests run. 383.25s total testing time 11012 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Logging in as eric@webkit.org... Rejecting patch 35087 from commit-queue. This patch will require manual commit.

> transitions/extra-transition.html -> failed Would you try to commit the patch again please? I have confirmed - all tests passed with WebKit rev.47628 - all tests passed with WebKit rev.47628 + this patch. I'm confident that this patch does not affect transition/extra-transition.html.

Comment on attachment 35087 [details] Proposed patch (rev.5) Ok. Probably a flakey test then.

Comment on attachment 35087 [details] Proposed patch (rev.5) Rejecting patch 35087 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: None

Created attachment 38419 [details] bugzilla-tool failure log Here is the failure log from bugzilla-tool. This is the first per-bug log it's ever made. :) (Feature is in testing.) You'll see from the log that trunk/WebCore/html/HTMLInputElement.idl has a tab in it, which caused the svn pre-commit hook to fail. We really should have svn-create-patch fail for those sorts of things.

Created attachment 38463 [details] Proposed patch (rev.6) > You'll see from the log that trunk/WebCore/html/HTMLInputElement.idl has a tab > in it, which caused the svn pre-commit hook to fail. Oh, I'm sorry for that. I have updated the patch.

Comment on attachment 38463 [details] Proposed patch (rev.6) OK.

Comment on attachment 38463 [details] Proposed patch (rev.6) Rejecting patch 38463 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

http/tests/appcache/different-scheme.html -> failed This seems to have failed with both attempts I just made. I'll attach the failure diff.

Created attachment 38577 [details] (seemingly unrelated) test failure when trying to land this

Comment on attachment 38463 [details] Proposed patch (rev.6) Nevermind. This was a local config issue on my part.

Comment on attachment 38463 [details] Proposed patch (rev.6) Clearing flags on attachment: 38463 Committed r47767: <http://trac.webkit.org/changeset/47767>

All reviewed patches have been landed. Closing bug.

> +#if defined(ENABLE_DATALIST) && ENABLE_DATALIST > + // The type of the list is HTMLElement according to the standard. > + // We intentionally use HTMLDataListElement for it because our implementation > + // always returns an HTMLDataListElement instance. > + readonly attribute HTMLDataListElement list; > +#endif It is not clear to me why the spec has this returning an Element, but I would rather we match it. Except for extended attributes, I would really like to keep these IDLs as close to the specs word as possible.

(In reply to comment #28) > It is not clear to me why the spec has this returning an Element, but I would > rather we match it. Except for extended attributes, I would really like to > keep these IDLs as close to the specs word as possible. Ok, I'll handle it in another bug entry.