Summary: | [HTML5][Forms] Part 2 of datalist&list: Support for HTMLInputElement::list and HTMLInputElement::selectedOption | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | eric, hyatt, sam | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
URL: | http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-input-list | ||||||||||||||||||||
Bug Depends on: | 26915 | ||||||||||||||||||||
Bug Blocks: | 19264, 27247 | ||||||||||||||||||||
Attachments: |
|
Description
Kent Tamura
2009-07-27 21:36:49 PDT
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. |