Bug 29363 - Support for <output> element
: Support for <output> element
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
http://www.whatwg.org/specs/web-apps/...
: HTML5
Depends on: 47810 47812
Blocks: HTML5Forms html5test 47813
  Show dependency treegraph
 
Reported: 2009-09-18 00:44 PDT by Kent Tamura
Modified: 2010-11-04 17:58 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-09-18 00:44:25 PDT
- intorduce HTMLOutputElement
- parsing support
- default CSS definition, etc.
Comment 1 Kenichi Ishibashi 2010-10-16 04:41:59 PDT
Created attachment 70950 [details]
A trial patch V0
Comment 2 Kenichi Ishibashi 2010-10-16 04:43:00 PDT
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 Kent Tamura 2010-10-17 21:06:16 PDT
Comment on attachment 70950 [details]
A trial patch V0

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 Kenichi Ishibashi 2010-10-18 04:04:28 PDT
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 Kenichi Ishibashi 2010-11-01 10:54:04 PDT
Created attachment 72521 [details]
Patch V1
Comment 6 Kenichi Ishibashi 2010-11-01 10:55:22 PDT
(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 Kent Tamura 2010-11-01 17:45:04 PDT
Comment on attachment 72521 [details]
Patch V1

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 Kent Tamura 2010-11-01 18:16:17 PDT
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 Kenichi Ishibashi 2010-11-01 20:07:53 PDT
Comment on attachment 72521 [details]
Patch V1

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 Kenichi Ishibashi 2010-11-01 20:46:59 PDT
Created attachment 72626 [details]
Patch V2
Comment 11 Kenichi Ishibashi 2010-11-01 20:52:28 PDT
> > 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 Kent Tamura 2010-11-01 21:06:25 PDT
Comment on attachment 72626 [details]
Patch V2

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 Kent Tamura 2010-11-01 21:09:47 PDT
Comment on attachment 72626 [details]
Patch V2

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 Alexey Proskuryakov 2010-11-01 22:33:46 PDT
Should this be inside #if?
Comment 15 Kent Tamura 2010-11-01 22:50:30 PDT
(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 Kenichi Ishibashi 2010-11-01 23:19:51 PDT
Comment on attachment 72626 [details]
Patch V2

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 Kent Tamura 2010-11-01 23:36:54 PDT
(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 Kenichi Ishibashi 2010-11-01 23:56:22 PDT
(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 Kenichi Ishibashi 2010-11-02 01:04:29 PDT
Created attachment 72638 [details]
Patch V3
Comment 20 Kent Tamura 2010-11-02 01:14:49 PDT
(In reply to comment #19)
> Created an attachment (id=72638) [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 WebKit Commit Bot 2010-11-03 22:15:41 PDT
Comment on attachment 72638 [details]
Patch V3

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 Kenichi Ishibashi 2010-11-03 23:41:23 PDT
Created attachment 72910 [details]
Patch V4
Comment 23 Kent Tamura 2010-11-03 23:45:16 PDT
Comment on attachment 72910 [details]
Patch V4

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 Kent Tamura 2010-11-03 23:49:41 PDT
Comment on attachment 72910 [details]
Patch V4

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 Kenichi Ishibashi 2010-11-03 23:54:39 PDT
Comment on attachment 72910 [details]
Patch V4

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 Kent Tamura 2010-11-04 00:01:18 PDT
(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 Kenichi Ishibashi 2010-11-04 00:33:47 PDT
Created attachment 72914 [details]
Patch V5
Comment 28 Kent Tamura 2010-11-04 00:36:00 PDT
Comment on attachment 72914 [details]
Patch V5

ok
Comment 29 Kenichi Ishibashi 2010-11-04 00:40:44 PDT
(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 WebKit Review Bot 2010-11-04 01:40:08 PDT
Attachment 72638 [details] did not build on win:
Build output: http://queues.webkit.org/results/5186010
Comment 31 Kent Tamura 2010-11-04 03:03:31 PDT
Comment on attachment 72914 [details]
Patch V5

We need to update WebCore/WebCore.vcproj/WebCore.vcproj
Comment 32 Kenichi Ishibashi 2010-11-04 03:18:42 PDT
Created attachment 72922 [details]
Fixed windows build
Comment 33 Kenichi Ishibashi 2010-11-04 03:20:41 PDT
(In reply to comment #31)
> We need to update WebCore/WebCore.vcproj/WebCore.vcproj

Sorry for bothering you.. I've updated it.
Comment 34 WebKit Commit Bot 2010-11-04 03:43:09 PDT
Comment on attachment 72914 [details]
Patch V5

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 WebKit Commit Bot 2010-11-04 04:26:03 PDT
Comment on attachment 72922 [details]
Fixed windows build

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 WebKit Commit Bot 2010-11-04 05:11:39 PDT
Comment on attachment 72922 [details]
Fixed windows build

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 Kenichi Ishibashi 2010-11-04 05:50:27 PDT
Created attachment 72930 [details]
Patch
Comment 38 Kent Tamura 2010-11-04 05:54:54 PDT
Comment on attachment 72930 [details]
Patch

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 Kenichi Ishibashi 2010-11-04 07:07:34 PDT
Created attachment 72937 [details]
Patch
Comment 40 WebKit Commit Bot 2010-11-04 17:52:32 PDT
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 WebKit Commit Bot 2010-11-04 17:58:11 PDT
Comment on attachment 72937 [details]
Patch

Clearing flags on attachment: 72937

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