Bug 29363 - Support for <output> element
: Support for <output> element
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://www.whatwg.org/specs/web-apps/...
: HTML5
: 47810 47812
: 19264 40829 47813
  Show dependency treegraph
 
Reported: 2009-09-18 00:44 PST by
Modified: 2010-11-04 17:58 PST (History)


Attachments
A trial patch V0 (109.62 KB, patch)
2010-10-16 04:41 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch V1 (80.08 KB, patch)
2010-11-01 10:54 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch V2 (77.90 KB, patch)
2010-11-01 20:46 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch V3 (77.85 KB, patch)
2010-11-02 01:04 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch V4 (79.62 KB, patch)
2010-11-03 23:41 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch V5 (89.97 KB, patch)
2010-11-04 00:33 PST, Kenichi Ishibashi
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fixed windows build (93.51 KB, patch)
2010-11-04 03:18 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (94.53 KB, patch)
2010-11-04 05:50 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (97.80 KB, patch)
2010-11-04 07:07 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-18 00:44:25 PST
- intorduce HTMLOutputElement
- parsing support
- default CSS definition, etc.
------- Comment #1 From 2010-10-16 04:41:59 PST -------
Created an attachment (id=70950) [details]
A trial patch V0
------- Comment #2 From 2010-10-16 04:43:00 PST -------
Hi, 
This patch is my first attempt to support HTML5 output element. SInce I'm not familiar with adding elements to WebKit, this patch would do something bad, or at least have problems. So It would be definitely helpful for me if you can take a look at the patch and pointed out the problems. This patch also doesn't contain CSS default property for the element. I'll add it later.

Thanks,
------- Comment #3 From 2010-10-17 21:06:16 PST -------
(From update of attachment 70950 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=70950&action=review

Thank you for working on this.

The patch is too large to review.  I recommend to split this into
 - DOMSettableTokenList implementation and JSC binding,
 - V8 binding for DOMSettableTokenList,
 - "form" attribute support for form control elements, and
 - <output> element support.

> WebCore/WebCore.xcodeproj/project.pbxproj:14519
> +				4AD7D6631267D56B0020D6E8 /* HTMLOutputElement.cpp */,

You should run WebKitTools/Scripts/sort-Xcode-project-file when you add files to an Xcode project.
------- Comment #4 From 2010-10-18 04:04:28 PST -------
Hi Kent-san,

> The patch is too large to review.  I recommend to split this into
>  - DOMSettableTokenList implementation and JSC binding,
>  - V8 binding for DOMSettableTokenList,
>  - "form" attribute support for form control elements, and
>  - <output> element support.

Thank you for your advice. I'll split this following your advice.

> You should run WebKitTools/Scripts/sort-Xcode-project-file when you add files to an Xcode project.

I didn't know that. Thank you letting me know that!

Regards,
------- Comment #5 From 2010-11-01 10:54:04 PST -------
Created an attachment (id=72521) [details]
Patch V1
------- Comment #6 From 2010-11-01 10:55:22 PST -------
(In reply to comment #5)

Hi,

I've revised the patch. Although the form attribute support, which blocks this issue, is not addressed yet, I think that adding output element before adding form attribute would be more convenient for writing tests (since there is no elements which have the form attribute for now). I'm planning to add form attribute support after this patch is successfully landed.

Thanks,
------- Comment #7 From 2010-11-01 17:45:04 PST -------
(From update of attachment 72521 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72521&action=review

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:40
> +element.htmlFor.add('x'); shouldBeEqualToString('element.htmlFor.toString()', 'x');

Insert a line-break after ";"

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:78
> +debug('Ensure that we can handle empty form attribute correctly');

Would you add debug('') before this line, or add "-" at the beginning of the message like other tests for readability of the test result please?

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:80
> +list = element.htmlFor;

Should prepend "var "

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:91
> +element.htmlFor.add('x')

What's the expected result of these remove() and add()?

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-htmlfor.js:1
> +description('Tests for the htmlFor attribute of the output element.');

Many test cases in this file look equivalent to tests in dom-settable-token-list.js.  Are they needed?

> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:7
> +output  = document.createElement('output');

Redundant spaces before "="

> WebCore/html/HTMLOutputElement.cpp:83
> +void HTMLOutputElement::childrenChanged(bool createdByParser, Node* /*beforeChange*/, Node* /*afterChange*/, int /*childCountDelta*/)

You don't need to keep unused parameter names.

> WebCore/html/HTMLOutputElement.cpp:133
> +    setTextContent(value, ec);

Better to have ASSERT(!m_isSetTextContentInProgress)

> WebCore/html/HTMLOutputElement.idl:35
> +        readonly attribute boolean willValidate;

You need a test for willValidate behavior.
------- Comment #8 From 2010-11-01 18:16:17 PST -------
Also, we need to add a default style for <output> to WebCore/css/html.css.
Probably display:inline or display:inilne-block is sufficient.
Firefox seems to use display:inline.
------- Comment #9 From 2010-11-01 20:07:53 PST -------
(From update of attachment 72521 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72521&action=review

Hi Kent-san,

Thank you for your review. I'll send revised patch following your comments.

>> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:40
>> +element.htmlFor.add('x'); shouldBeEqualToString('element.htmlFor.toString()', 'x');
> 
> Insert a line-break after ";"

done.

>> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:78
>> +debug('Ensure that we can handle empty form attribute correctly');
> 
> Would you add debug('') before this line, or add "-" at the beginning of the message like other tests for readability of the test result please?

I've added "-" at the beginning of all debug messages.

>> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:91
>> +element.htmlFor.add('x')
> 
> What's the expected result of these remove() and add()?

I forgot to add expectations. Added expectations. Sorry for confusing.

>> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-htmlfor.js:1
>> +description('Tests for the htmlFor attribute of the output element.');
> 
> Many test cases in this file look equivalent to tests in dom-settable-token-list.js.  Are they needed?

Removed.

>> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:7
>> +output  = document.createElement('output');
> 
> Redundant spaces before "="

Fixed.

>> WebCore/html/HTMLOutputElement.cpp:83
>> +void HTMLOutputElement::childrenChanged(bool createdByParser, Node* /*beforeChange*/, Node* /*afterChange*/, int /*childCountDelta*/)
> 
> You don't need to keep unused parameter names.

Thank you for letting me know that. Removed unused parameter names.

>> WebCore/html/HTMLOutputElement.idl:35
>> +        readonly attribute boolean willValidate;
> 
> You need a test for willValidate behavior.

Thank you for the advice. I've added a test.
------- Comment #10 From 2010-11-01 20:46:59 PST -------
Created an attachment (id=72626) [details]
Patch V2
------- Comment #11 From 2010-11-01 20:52:28 PST -------
> > WebCore/html/HTMLOutputElement.cpp:133
> > +    setTextContent(value, ec);
>
> Better to have ASSERT(!m_isSetTextContentInProgress)

Done.

> Also, we need to add a default style for <output> to WebCore/css/html.css.
> Probably display:inline or display:inilne-block is sufficient.
> Firefox seems to use display:inline.

Thank you for notifying that. I forgot it. I've added display:inline for <output>.

In addition, I also forgot to add an attribute for HTMLOutputElement to dom/page/Window.idl and added it in the revised patch.

Thanks,
------- Comment #12 From 2010-11-01 21:06:25 PST -------
(From update of attachment 72626 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review

> LayoutTests/fast/dom/HTMLOutputElement/htmloutputelement-validity-expected.txt:1
> +CONSOLE MESSAGE: line 25: true

Something wrong.

> WebCore/html/HTMLOutputElement.h:44
> +    virtual bool isEnumeratable() const { return true; }

Need a test for this.
However, I couldn't find existing tests for from control enumeration.  We might need a new test for all of form control types.

These override functions (isEnumeratable, willValidate, reset, childrenChanged) should be moved to private:.
------- Comment #13 From 2010-11-01 21:09:47 PST -------
(From update of attachment 72626 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review

> WebCore/ChangeLog:7
> +        Support for &lt;output&gt; element

No need to use entity references :-)
------- Comment #14 From 2010-11-01 22:33:46 PST -------
Should this be inside #if?
------- Comment #15 From 2010-11-01 22:50:30 PST -------
(In reply to comment #14)
> Should this be inside #if?

Does Apple have any special reasons to do so?

If not, I don't think we need to enclose the code by #if ENABLE().  The implementation doesn't need platform-specific code, and will be completed by this patch except "form" attribute support.
------- Comment #16 From 2010-11-01 23:19:51 PST -------
(From update of attachment 72626 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review

Kent-san,

Thank you your quick review. I'll revise the patch but I have a question on tests for form control enumeration. If I understand the spec correctly, the spec <http://www.w3.org/TR/html5/forms.html#category-listed> says that following elements would be listed in elements property of form elements: <button>, <fieldset>, <input>, <keygen>, <object>, <output>, <select>, and <textarea>. But for the current implementation, <fieldset>, <keygen> and <object> are not contained in the elements property. Which behavior should I add in the tests?

>> LayoutTests/fast/dom/HTMLOutputElement/htmloutputelement-validity-expected.txt:1
>> +CONSOLE MESSAGE: line 25: true
> 
> Something wrong.

I'm sorry I forgot delete this debug information. I'll remove this.

>> WebCore/ChangeLog:7
>> +        Support for &lt;output&gt; element
> 
> No need to use entity references :-)

Oh, thank you notifying this. I'll fix this.

>> WebCore/html/HTMLOutputElement.h:44
>> +    virtual bool isEnumeratable() const { return true; }
> 
> Need a test for this.
> However, I couldn't find existing tests for from control enumeration.  We might need a new test for all of form control types.
> 
> These override functions (isEnumeratable, willValidate, reset, childrenChanged) should be moved to private:.

I've moved these override functions to private except for willValidate() since it is called by JSHTMLOutputElement class.

I'm going to try to write tests for form control enumeration but I have some questions, as I mentioned above. It is great if you could give some advice on this.
------- Comment #17 From 2010-11-01 23:36:54 PST -------
(In reply to comment #16)
> Thank you your quick review. I'll revise the patch but I have a question on tests for form control enumeration. If I understand the spec correctly, the spec <http://www.w3.org/TR/html5/forms.html#category-listed> says that following elements would be listed in elements property of form elements: <button>, <fieldset>, <input>, <keygen>, <object>, <output>, <select>, and <textarea>. But for the current implementation, <fieldset>, <keygen> and <object> are not contained in the elements property. Which behavior should I add in the tests?

ok, we need to discuss what we should do.  Let's discuss at new bug.  You don't need to add an enumeration test in this patch.
------- Comment #18 From 2010-11-01 23:56:22 PST -------
(In reply to comment #17)
> ok, we need to discuss what we should do.  Let's discuss at new bug.  You don't need to add an enumeration test in this patch.

I've posted this as https://bugs.webkit.org/show_bug.cgi?id=48821. Thank you for your suggestion!
------- Comment #19 From 2010-11-02 01:04:29 PST -------
Created an attachment (id=72638) [details]
Patch V3
------- Comment #20 From 2010-11-02 01:14:49 PST -------
(In reply to comment #19)
> Created an attachment (id=72638) [details] [details]
> Patch V3

Looks good.  I'll set r+ on the next work day if Apple people has no comments on #if guard.
------- Comment #21 From 2010-11-03 22:15:41 PST -------
(From update of attachment 72638 [details])
Rejecting patch 72638 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
...
fast/doctypes ............
fast/dom ..................................................................................................................................................................................................................................
fast/dom/prototype-inheritance-2.html -> failed

Exiting early after 1 failures. 6883 tests run.
269.32s total testing time

6882 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5078009
------- Comment #22 From 2010-11-03 23:41:23 PST -------
Created an attachment (id=72910) [details]
Patch V4
------- Comment #23 From 2010-11-03 23:45:16 PST -------
(From update of attachment 72910 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review

> LayoutTests/fast/dom/prototype-inheritance-2-expected.txt:-443
> -

Why is the last line removed?

> LayoutTests/fast/dom/prototype-inheritance-expected.txt:-837
> -

ditto.
------- Comment #24 From 2010-11-03 23:49:41 PST -------
(From update of attachment 72910 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review

> LayoutTests/ChangeLog:28
> +        * fast/dom/Window/window-properties-expected.txt: Updated expectation.
> +        * fast/dom/Window/window-property-descriptors-expected.txt: Updated expectation.
> +        * fast/dom/prototype-inheritance-2-expected.txt: Updated expectation.
> +        * fast/dom/prototype-inheritance-expected.txt: Updated expectation.

We need to update the followings too.

LayoutTests/platform/chromium/fast/dom/prototype-inheritance-expected.txt
LayoutTests/platform/gtk/fast/dom/prototype-inheritance-expected.txt
LayoutTests/platform/qt/fast/dom/prototype-inheritance-expected.txt
LayoutTests/platform/win/fast/dom/prototype-inheritance-2-expected.txt
LayoutTests/platform/win/fast/dom/prototype-inheritance-expected.txt
LayoutTests/platform/gtk/fast/dom/Window/window-properties-expected.txt
LayoutTests/platform/gtk/fast/dom/Window/window-property-descriptors-expected.txt
LayoutTests/platform/qt/fast/dom/Window/window-properties-expected.txt
LayoutTests/platform/qt/fast/dom/Window/window-property-descriptors-expected.txt
LayoutTests/platform/win/fast/dom/Window/window-property-descriptors-expected.txt
------- Comment #25 From 2010-11-03 23:54:39 PST -------
(From update of attachment 72910 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review

>> LayoutTests/fast/dom/prototype-inheritance-2-expected.txt:-443
>> -
> 
> Why is the last line removed?

I'm sorry for confusing, but I've just run new-run-webkit-tests and copied from -actual.txt to -expected.txt. Is this a bug of the new-run-webkit-tests? Anyway, I'll re-send the patch. Sorry again the inconvenience.
------- Comment #26 From 2010-11-04 00:01:18 PST -------
(In reply to comment #25)
> I'm sorry for confusing, but I've just run new-run-webkit-tests and copied from -actual.txt to -expected.txt. Is this a bug of the new-run-webkit-tests? Anyway, I'll re-send the patch. Sorry again the inconvenience.

If you work on Mac, I recommend to use run-webkit-tests (not new-).
  % run-webkit-tests  --debug/--release --reset fast/dom/prototype-inheritance.html
updates the expectation.  new-run-webkit-tests on Mac is not popular yet.
------- Comment #27 From 2010-11-04 00:33:47 PST -------
Created an attachment (id=72914) [details]
Patch V5
------- Comment #28 From 2010-11-04 00:36:00 PST -------
(From update of attachment 72914 [details])
ok
------- Comment #29 From 2010-11-04 00:40:44 PST -------
(In reply to comment #26)
> If you work on Mac, I recommend to use run-webkit-tests (not new-).
>   % run-webkit-tests  --debug/--release --reset fast/dom/prototype-inheritance.html
> updates the expectation.  new-run-webkit-tests on Mac is not popular yet.

I see. Thank you for your advice. I'll do in this way next time:-)

I've posted revised patch, which will modify platform-dependent expectation files. Thank you for letting me know that.
------- Comment #30 From 2010-11-04 01:40:08 PST -------
Attachment 72638 [details] did not build on win:
Build output: http://queues.webkit.org/results/5186010
------- Comment #31 From 2010-11-04 03:03:31 PST -------
(From update of attachment 72914 [details])
We need to update WebCore/WebCore.vcproj/WebCore.vcproj
------- Comment #32 From 2010-11-04 03:18:42 PST -------
Created an attachment (id=72922) [details]
Fixed windows build
------- Comment #33 From 2010-11-04 03:20:41 PST -------
(In reply to comment #31)
> We need to update WebCore/WebCore.vcproj/WebCore.vcproj

Sorry for bothering you.. I've updated it.
------- Comment #34 From 2010-11-04 03:43:09 PST -------
(From update of attachment 72914 [details])
Rejecting patch 72914 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
 ...............
fast/inspector-support ......
fast/invalid ...................................
fast/js .......................................................................................................................................................................
fast/js/global-constructors.html -> failed

Exiting early after 1 failures. 8885 tests run.
186.23s total testing time

8884 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5122019
------- Comment #35 From 2010-11-04 04:26:03 PST -------
(From update of attachment 72922 [details])
Rejecting patch 72922 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
 ...............
fast/inspector-support ......
fast/invalid ...................................
fast/js .......................................................................................................................................................................
fast/js/global-constructors.html -> failed

Exiting early after 1 failures. 8885 tests run.
163.49s total testing time

8884 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5183009
------- Comment #36 From 2010-11-04 05:11:39 PST -------
(From update of attachment 72922 [details])
Rejecting patch 72922 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
 ...............
fast/inspector-support ......
fast/invalid ...................................
fast/js .......................................................................................................................................................................
fast/js/global-constructors.html -> failed

Exiting early after 1 failures. 8885 tests run.
187.80s total testing time

8884 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5173011
------- Comment #37 From 2010-11-04 05:50:27 PST -------
Created an attachment (id=72930) [details]
Patch
------- Comment #38 From 2010-11-04 05:54:54 PST -------
(From update of attachment 72930 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=72930&action=review

> LayoutTests/ChangeLog:29
> +        * fast/js/global-constructors-expected.txt: Updated expectation.

You need to update the followings too.

LayoutTests/platform/gtk/fast/js/global-constructors-expected.txt
LayoutTests/platform/qt/fast/js/global-constructors-expected.txt
LayoutTests/platform/win/fast/js/global-constructors-expected.txt
------- Comment #39 From 2010-11-04 07:07:34 PST -------
Created an attachment (id=72937) [details]
Patch
------- Comment #40 From 2010-11-04 17:52:32 PST -------
The commit-queue encountered the following flaky tests while processing attachment 72937 [details]:

http/tests/appcache/manifest-redirect.html

Please file bugs against the tests.  These tests were authored by ap@webkit.org.  The commit-queue is continuing to process your patch.
------- Comment #41 From 2010-11-04 17:58:11 PST -------
(From update of attachment 72937 [details])
Clearing flags on attachment: 72937

Committed r71373: <http://trac.webkit.org/changeset/71373>
------- Comment #42 From 2010-11-04 17:58:20 PST -------
All reviewed patches have been landed.  Closing bug.