Bug 27756 – [HTML5][Forms] Part 2 of datalist&list: Support for HTMLInputElement::list and HTMLInputElement::selectedOption

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Add support for two DOM attributes, list and selectedOption, of INPUT element.

I already have a patch.

[Kent Tamura]

Created attachment 33604 [details]
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.


---
 10 files changed, 274 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

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         }

[Kent Tamura]

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?

[Kent Tamura]

Created attachment 33694 [details]
Support for HTMLInputElement::list and HTMLInputElement::selectedOption.


---
 10 files changed, 265 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

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

[Kent Tamura]

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.

[Kent Tamura]

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

[Peter Kasting]

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

[Eric Seidel (no email)]

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.

[Kent Tamura]

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

[Kent Tamura]

Created attachment 35032 [details]
Proposed patch (rev.4)

- The new code is guarded by ENABLE(DATALIST)
- Change the type of HTMLInputElement::list; HTMLElement -> HTMLDataListElement

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Kent Tamura]

Created attachment 35087 [details]
Proposed patch (rev.5)

Commit-queue-friendly patch :-)

[Eric Seidel (no email)]

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 [e-mail]...
Rejecting patch 35087 from commit-queue.  This patch will require manual commit.

[Kent Tamura]

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

[Eric Seidel (no email)]

Comment on attachment 35087 [details]
Proposed patch (rev.5)

Ok.  Probably a flakey test then.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Kent Tamura]

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.

[Eric Seidel (no email)]

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

OK.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

http/tests/appcache/different-scheme.html -> failed

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

I'll attach the failure diff.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

Nevermind.  This was a local config issue on my part.

[Eric Seidel (no email)]

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

Clearing flags on attachment: 38463

Committed r47767: <http://trac.webkit.org/changeset/47767>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

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

[Kent Tamura]

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