Bug 27756

Summary: [HTML5][Forms] Part 2 of datalist&list: Support for HTMLInputElement::list and HTMLInputElement::selectedOption
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: 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 Flags
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.
none
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.
eric: review-
Proposed patch (rev.3)
eric: review-
Proposed patch (rev.4)
eric: review+, eric: commit-queue-
Proposed patch (rev.5)
eric: review+, eric: commit-queue-
bugzilla-tool failure log
none
Proposed patch (rev.6)
none
(seemingly unrelated) test failure when trying to land this none

Description Kent Tamura 2009-07-27 21:36:49 PDT
Add support for two DOM attributes, list and selectedOption, of INPUT element.

I already have a patch.
Comment 1 Kent Tamura 2009-07-27 23:35:10 PDT
Created attachment 33604 [details]
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.


---
 10 files changed, 274 insertions(+), 1 deletions(-)
Comment 2 Eric Seidel (no email) 2009-07-28 10:46:28 PDT
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         }
Comment 3 Kent Tamura 2009-07-28 18:37:43 PDT
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?
Comment 4 Kent Tamura 2009-07-28 22:52:18 PDT
Created attachment 33694 [details]
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.


---
 10 files changed, 265 insertions(+), 1 deletions(-)
Comment 5 Eric Seidel (no email) 2009-07-29 09:33:47 PDT
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()
Comment 6 Kent Tamura 2009-07-31 02:10:07 PDT
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.
Comment 7 Kent Tamura 2009-07-31 02:21:27 PDT
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
Comment 8 Peter Kasting 2009-08-04 15:54:18 PDT
dhyatt was working on <datalist> support, he should be CCed on all bugs about it
Comment 9 Eric Seidel (no email) 2009-08-06 19:29:29 PDT
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.
Comment 10 Kent Tamura 2009-08-06 20:53:08 PDT
(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.
Comment 11 Kent Tamura 2009-08-18 02:58:35 PDT
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 12 Eric Seidel (no email) 2009-08-18 08:43:51 PDT
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-.
Comment 13 Eric Seidel (no email) 2009-08-18 08:44:10 PDT
(The ChangeLog bit is the part of the diff I worry about svn-apply handling wrong.)
Comment 14 Kent Tamura 2009-08-18 16:49:51 PDT
Created attachment 35087 [details]
Proposed patch (rev.5)

Commit-queue-friendly patch :-)
Comment 15 Eric Seidel (no email) 2009-08-20 22:43:20 PDT
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.
Comment 16 Kent Tamura 2009-08-21 07:33:48 PDT
> 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 17 Eric Seidel (no email) 2009-08-21 14:55:08 PDT
Comment on attachment 35087 [details]
Proposed patch (rev.5)

Ok.  Probably a flakey test then.
Comment 18 Eric Seidel (no email) 2009-08-21 17:13:44 PDT
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
Comment 19 Eric Seidel (no email) 2009-08-21 17:16:44 PDT
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.
Comment 20 Kent Tamura 2009-08-23 18:50:42 PDT
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 21 Eric Seidel (no email) 2009-08-25 16:19:10 PDT
Comment on attachment 38463 [details]
Proposed patch (rev.6)

OK.
Comment 22 Eric Seidel (no email) 2009-08-25 17:01:54 PDT
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
Comment 23 Eric Seidel (no email) 2009-08-25 17:03:26 PDT
http/tests/appcache/different-scheme.html -> failed

This seems to have failed with both attempts I just made.

I'll attach the failure diff.
Comment 24 Eric Seidel (no email) 2009-08-25 17:04:01 PDT
Created attachment 38577 [details]
(seemingly unrelated) test failure when trying to land this
Comment 25 Eric Seidel (no email) 2009-08-25 17:18:06 PDT
Comment on attachment 38463 [details]
Proposed patch (rev.6)

Nevermind.  This was a local config issue on my part.
Comment 26 Eric Seidel (no email) 2009-08-25 17:51:14 PDT
Comment on attachment 38463 [details]
Proposed patch (rev.6)

Clearing flags on attachment: 38463

Committed r47767: <http://trac.webkit.org/changeset/47767>
Comment 27 Eric Seidel (no email) 2009-08-25 17:51:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Sam Weinig 2009-08-26 06:39:51 PDT
> +#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.
Comment 29 Kent Tamura 2009-08-27 02:25:47 PDT
(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.