629801 - Implement HTML5 <time> element

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Adam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8) Gecko/20100729 Iceweasel/3.6.7 (like Firefox/3.6.7)
Build Identifier: 

http://dev.w3.org/html5/spec/text-level-semantics.html#the-time-element says:
"The time element represents either a time on a 24 hour clock, or a precise date in the proleptic Gregorian calendar, optionally with a time and a time-zone offset." 

Initial support might simply include time elements indicating they are publication dates in page or article information dialogues (as well as potentially in places data), along with tooltips displaying the actual date and time on hovering time elements that use the datetime attribute.

Reproducible: Always




Added initial page/documentation at https://developer.mozilla.org/en/HTML/Element/time with header for unimplemented.  Needs better examples.  Will need any application-specific notes and gecko-minversion header when support is added.

Comment 1

[Norman Paschke]

<time> seems to be replaced by <data> http://www.w3.org/Bugs/Public/show_bug.cgi?id=13240

Comment 2

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Norman Paschke from comment #1)
> <time> seems to be replaced by <data>
> http://www.w3.org/Bugs/Public/show_bug.cgi?id=13240

<time> is still present in both the W3C and WHATWG specs:

http://www.w3.org/TR/html5/the-time-element.html#the-time-element
http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-time-element

Comment 3

[David Humphrey (:humph)]

I've started working on this, and have the DOM stuff done.  I'm now looking at the date parsing, but I'm a bit confused about what datetime should return in JS.  I see Opera has the old .valueAsDate stuff implemented still (see http://my.opera.com/ODIN/blog/2011/05/31/dom-scripting-and-the-time-element).  My first thought was that I should be taking whatever is on datetime (or textContent) and parsing it.  But the spec doesn't seem to indicate the format of the string you get back--it just says that it's machine readable.

Looking at http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-time-element there are lots of great examples of valid date/times; however, it never says what you'd get by calling datetime from script on any of these.  Given that they lay out parsing steps, it's clear they expect the machine-readable date to be read by machine.  It says:


"The datetime attribute may be present. If present, its value must be a representation of the element's contents in a machine-readable format.

"A time element that does not have a datetime content attribute must not have any element descendants.

"The datetime value of a time element is the value of the element's datetime content attribute, if it has one, or the element's textContent, if it does not."


So my question is whether I simply validate the datetime value and return it unchanged and nothing if invalid, or do I take what they give, validate and parse it, and return some new string.

Thanks for helping me understand what to do next.

Comment 4

[David Humphrey (:humph)]

From IRC:

14:29 <@smaug> Hixie: is the "datetime validation" with <time> authoring thing only?
14:29 <@smaug> humph: what does Opera do?
14:30 < humph> http://people.opera.com/miket/2011/5/time.html they implement an old version of the spec
14:30 < humph> and use .valueAsDate to get something useful out of it
14:30 <@smaug> ah, right so if the attribute is there, it must be the machine readable version of the contents...
14:31 <@smaug> <time class="dtstart" datetime="2005-10-05">October 5</time>
14:31 <@smaug> so I don't see anything but authoring requirements 
14:31 < Hixie> smaug: what "datetime validation"?
14:32 < Hixie> smaug: the "must match one of the following syntaxes"?
14:32 <@smaug> right
14:32 < Hixie> smaug: then yes, though if you do anything that ends up needing the "machine-readable equivalent of the element's contents" then you end up having to do that work anyway
14:32 < Hixie> smaug: (currently only the table sorting model uses that, and that's a WIP)
14:33 <@smaug> Hixie: so this is all authoring thing
14:34 <@smaug> <time> is just a dummy element which happens to have .datetime to reflect datetime content attribute
14:35 < Hixie> smaug: it's a way to represent a time in a machine-readable way, which is useful in microdata and in the table sorting model, but otherwise is of limited use, ues
14:35 < Hixie> s/ues/yes/
14:36 <@smaug> Hixie: and browser doesn't need to validate anything
14:36 <@smaug> ?
14:36 < Hixie> smaug: unless you implement the table sorting model or any specific microdata vocabularies, there's very little to do to support <time>, correct.
...
14:38 < Hixie> there was a <time> element in the past that did localisation of rendering
14:38 < Hixie> but that <time> element was removed
14:38 < Hixie> and a new one put in its place which had very little relation to the old one
14:38 < Hixie> they might be talking about the old one
...
14:38 < Hixie> the new theory is that any localisation would be driven by CSS

So datetime needs to be the value of datetime, or if absent, the textContent, and browsers don't need to validate this.

Comment 5

[David Humphrey (:humph)]

Attachment: v1

Here's a first pass at this.  I'm unclear if I've properly understood the things we discussed on irc about how datatime reflects, so I thought I'd post a patch and let you guide me that way.

This code relies on bug 830879 to get around the need for classinfo.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 708382 [details] [diff] [review]
v1

>+  virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
Nit, nsINodeInfo* and nsINode**

>+  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope,
>+                             bool *aTriedToWrap) MOZ_OVERRIDE;
ditto

>+interface HTMLTimeElement : HTMLElement {
>+           [SetterThrows]
>+           attribute DOMString datetime;

Why SetterThrows? I don't see any reason for that.


This needs tests. At least add the element to content/html/content/test/test_bug389797.html

Also, don't you need to teach html5 parser about this new element?
What about editor/libeditor/base/nsEditPropertyAtomList.h or editor/libeditor/html/nsHTMLEditUtils.cpp (around line 615)


(we should make adding new elements easier)

Comment 7

[David Humphrey (:humph)]

Attachment: v2

Thanks for the fast review.  I've tried to cover all that you pointed out.  I ran into a few snags with test_bug389797.html and the lack of classinfo in my implementation--bz helped me sort out a few things.  I'm also unclear if the flags I've chosen in nsHTMLEditUtils.cpp are correct.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(Henri reminded me yesterday that <time> doesn't probably need changes to html5 parser.)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 709075 [details] [diff] [review]
v2

peterv should look at this too, since he knows the current story behind classinfos.
And based on bug 153998, peterv knows the editor bits well too :)

Comment 10

[Henri Sivonen (:hsivonen)]

Parser changes are not needed. In fact, there is already a pre-interned name for time. In any case, it would work even without one, since there is no special parsin rules for time.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 709075 [details] [diff] [review]
v2

Review of attachment 709075 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLTimeElement.cpp
@@ +13,5 @@
> +NS_NewHTMLTimeElement(already_AddRefed<nsINodeInfo> aNodeInfo,
> +                      mozilla::dom::FromParser aFromParser)
> +{
> +  return new mozilla::dom::HTMLTimeElement(aNodeInfo);
> +}

NS_IMPL_NS_NEW_HTML_ELEMENT

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Olli Pettay [:smaug] from comment #6)
> >+interface HTMLTimeElement : HTMLElement {
> >+           [SetterThrows]
> >+           attribute DOMString datetime;
> 
> Why SetterThrows? I don't see any reason for that.

SetAttr can fail... Though I'm not sure how this patch even builds, as SetDatetime doesn't seem to take an ErrorResult&.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

We have other reflected attributes without SetterThrows

Comment 14

[David Humphrey (:humph)]

(In reply to :Ms2ger from comment #11)
> Comment on attachment 709075 [details] [diff] [review]
> v2
> 
> Review of attachment 709075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLTimeElement.cpp
> @@ +13,5 @@
> > +NS_NewHTMLTimeElement(already_AddRefed<nsINodeInfo> aNodeInfo,
> > +                      mozilla::dom::FromParser aFromParser)
> > +{
> > +  return new mozilla::dom::HTMLTimeElement(aNodeInfo);
> > +}
> 
> NS_IMPL_NS_NEW_HTML_ELEMENT

I didn't use this because it assumes that elements have an ns* prefix, see:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.h#1873

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to David Humphrey (:humph) from comment #14)
> I didn't use this because it assumes that elements have an ns* prefix, see:

No it doesn't. You should use NS_IMPL_NS_NEW_HTML_ELEMENT, it uses a template which works with either nsHTML*Element or mozilla::dom::HTML*Element.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please note http://html5.org/tools/web-apps-tracker?from=7687&to=7688

Comment 17

[David Humphrey (:humph)]

Attachment: v3 (carrying forward r+ from smaug)

This removes [SetterThrows], switches to NS_IMPL_NS_NEW_HTML_ELEMENT (thanks to Ms2ger for helping me figure out my issue there), and changes .datetime to .dateTime.

Comment 18

[David Humphrey (:humph)]

We'll need to update https://developer.mozilla.org/en-US/docs/HTML/HTML_Elements/time when this lands (it reflects the old spec's pubdate, plus needs to s/datetime/dateTime/).

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 709794 [details] [diff] [review]
v3 (carrying forward r+ from smaug)

Review of attachment 709794 [details] [diff] [review]:
-----------------------------------------------------------------

Can you get Henri to check the parser changes (comment 10 seems to imply you don't need them)?

::: content/html/content/src/HTMLTimeElement.cpp
@@ +28,5 @@
> +NS_IMPL_RELEASE_INHERITED(HTMLTimeElement, Element)
> +
> +NS_INTERFACE_TABLE_HEAD(HTMLTimeElement)
> +  NS_HTML_CONTENT_INTERFACE_TABLE1(HTMLTimeElement,
> +                                   nsIDOMHTMLElement)

NS_HTML_CONTENT_INTERFACE_TABLE0(HTMLTimeElement)

::: content/html/content/test/test_bug389797.html
@@ +256,5 @@
>      is(node instanceof SpecialPowers.Ci[iface], true,
>         tagName(tag) + " does not QI to " + iface);
>    }
>  
> +  // Now see what classinfo reports. NOTE: with the move to WebIDL bindings, not all

Just remove the code from here to the end of the for loop.

::: dom/webidl/HTMLTimeElement.webidl
@@ +8,5 @@
> + * http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-time-element
> + */
> +
> +interface HTMLTimeElement : HTMLElement {
> +           attribute DOMString dateTime;

I think you *should* add SetterThrows.

Comment 20

[Peter Van der Beken [:peterv]]

(In reply to Olli Pettay [:smaug] from comment #13)
> We have other reflected attributes without SetterThrows

There are bugs, yes. We should keep adding SetterThrows for now.

Comment 21

[David Humphrey (:humph)]

Attachment: v4 (r=smaug, peterv)

Thanks Peter.  This changes to use NS_HTML_CONTENT_INTERFACE_TABLE0, removes the "classinfo reports" section of the main loop in content/html/content/test/test_bug389797.html, and adds back SetterThrows for the dateTime attribute.

Henri, could you look at what I've done with parser changes, and make sure this is correct/necessary?

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 710682 [details] [diff] [review]
v4 (r=smaug, peterv)

Review of attachment 710682 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLTimeElement.h
@@ +37,5 @@
> +  {
> +    GetHTMLAttr(nsGkAtoms::datetime, aDateTime);
> +  }
> +
> +  void SetDateTime(const nsAString& aDateTime, mozilla::ErrorResult& aError)

Drop the mozilla::

::: dom/webidl/HTMLTimeElement.webidl
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/html5/text-level-semantics.html#the-time-element
> + * http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-time-element

No need to reference the obsolete W3C spec, and add the WHATWG document license.

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 710682 [details] [diff] [review]
v4 (r=smaug, peterv)

r+ for the parts under parser/

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You should probably also implement the following: <http://www.whatwg.org/html/#dom-itemvalue>

# The itemValue IDL attribute's behavior depends on the element, as follows:
#
# If the element is a time element
#    On getting, if the element has a datetime content attribute,
#    the IDL attribute must return that content attribute's value;
#    otherwise, it must return the element's textContent. On
#    setting, the IDL attribute must act as it would if it was
#    reflecting the element's datetime content attribute.

There's a test in dom/imptests/html/tests/submission/Opera/microdata/test_001.html; removing the relevant line from dom/imptests/failures/html/tests/submission/Opera/microdata/test_001.html.json should make it pass.

Except that the test is wrong. Crap.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Opera's test

Can you check if you pass these tests? :)

Comment 26

[David Humphrey (:humph)]

Attachment: v5 (r=smaug, peterv, hsivonen)

I've removed the W3C spec link, implemented the changes necessary for ItemValue, and updated the list of failing tests mentioned above in order to reflect that they now pass.

I also tested against the Opera tests you added in the other attachment, and I now pass all the <time> tests.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 714520 [details] [diff] [review]
v5 (r=smaug, peterv, hsivonen)

HTMLTimeElement::SetItemValueText is wrong. 
"On setting, the IDL attribute must act as it would if it was reflecting the element's datetime content attribute."
(The spec is nicely inconsistent.)

Add also a test for the case when there isn't datetime attribute but
itemValue is used to set it.

Comment 28

[David Humphrey (:humph)]

Attachment: v6 (r=smaug, peterv, hsivonen)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 716291 [details] [diff] [review]
v6 (r=smaug, peterv, hsivonen)

>+// .itemValue setter uses datetime vs. text content
>+times.properties["a"][0].itemValue = "2009-05-10";
>+is(times.properties["a"][0].itemValue, "2009-05-10", "setting itemValue updates datetime");
Shouldn't you test here .dateTime, not .itemValue

Comment 30

[David Humphrey (:humph)]

Attachment: v7 (r=smaug, peterv, hsivonen) [checkin-needed]

This is ready.  Boris, the changes you need for classinfo-less elements is also made to content/html/content/test/test_bug389797.html.

Comment 31

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05eeb90bbc31

Thanks for pushing this through, David!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was backed out due to one of the changesets in the push making browser_dbg_bug723069_editor-breakpoints.js fail frequently on Windows and OSX opt builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca34c11bf55d

https://tbpl.mozilla.org/php/getParsedLog.php?id=19995636&tree=Mozilla-Inbound

11:12:16     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | editor.getBreakpoints().length is correct
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | remove the second breakpoint using the mouse
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of attributes' specified attribute is deprecated. It always returns true." {file: "chrome://browser/content/orion.js" line: 6341}]
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of removeAttributeNode() is deprecated. Use removeAttribute() instead." {file: "chrome://browser/content/orion.js" line: 6342}]
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:46  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Test timed out
11:12:46  WARNING -  This is a harness error.
11:12:46     INFO -  args: ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/mozilla-test-fail_GzeI8j']
11:12:48     INFO -  SCREENSHOT: <see log>
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ToolbarView
11:12:48     INFO -  DBG-FRONTEND: Destroying the OptionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ChromeGlobalsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the SourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilterView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilteredSourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the StackFramesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the BreakpointsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the WatchExpressionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the GlobalSearchView
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView window
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView panes
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView editor
11:12:48     INFO -  DBG-FRONTEND: SourceScripts is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: StackFrames is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: ThreadState is disconnecting...
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been added
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been removed
11:12:48  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of editor breakpoint changes - Got 3, expected 4
11:12:48  WARNING -  This is a harness error.
11:12:48     INFO -  Stack trace:
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 486
11:12:48     INFO -      JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js :: <TOP_LEVEL> :: line 295
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_nextTest :: line 250
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 419
11:12:48     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
11:12:48     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | finished in 30099ms

Comment 33

[David Humphrey (:humph)]

Well, this seems pretty unrelated to my patch.  How to proceed?  Re-land?

Comment 34

[Boris Zbarsky [:bzbarsky]]

The problem is that it's not really related to any of the patches involved.  And we've had problems with the debugger tests in the past when we've added properties to the global, which this patch does.  :(

I do agree that chances are this is not the problem.  I've done some try pushes of the individual patches in that push to see which one might be causing the problem, and I'll reland this patch once try actually deigns to give me some results....

Comment 35

[Boris Zbarsky [:bzbarsky]]

Confirmed via try that this bug is not responsible for that orange (as far as I can tell) and pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/76f5d877e6f6

Comment 36

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/76f5d877e6f6

Comment 37

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:22.0) Gecko/20130403 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130403 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130402 Firefox/22.0
BuildID: 20130402004013

I also performed some testing around this element on latest Firefox 22 Aurora builds across main platforms and there were no visual issues or errors on pages using the <time> tag.
Based on this and the fact that there is a test page for this element, I'm marking the bug Verified.

Comment 38

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 39

[Jean-Yves Perrier [:teoli]]

Documented:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/22
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/time
https://developer.mozilla.org/en-US/docs/Web/API/HTMLTimeElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLTimeElement.dateTime