document.lastModified should be localized

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: Koike Kazuhiko, Assigned: masayuki)

Tracking

({fixed1.8, intl})

Trunk
mozilla1.8beta5
fixed1.8, intl
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
document.lastModified returns GMT, but it should return local time.

IE and N4 return local time.
(Reporter)

Updated

16 years ago
Keywords: 4xp
(Reporter)

Comment 1

16 years ago
Created attachment 48971 [details]
Testcase
This is true for http documents, for file documents we get it right.

Targetting for mozilla1.0.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

Comment 3

16 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 4

16 years ago
Severity = LOW [No Crash, Functional failure, No Cosmetic failure]
Visibility = HIGH [Lot of real world website usage, Gets one point of 
compatibility with other browsers since it works on other browsers. gets one 
more point on compliance with adopted techonology, that is JS] 

Priority = Visibility * Severity

Priority = p3

adding word "qawanted" because I'm setting this priority on available data & if 
someone feels otherwise then please investigate this more & feel free to change 
this priority.
Keywords: qawanted
Priority: -- → P3

Comment 5

16 years ago
Comment on attachment 48971 [details]
Testcase

This testcase is useless since it doesn't actually show a document's real
lastModified date; only the hardcoded 'unknown' one.
Attachment #48971 - Attachment is obsolete: true

Comment 6

15 years ago
if this bug is ASSIGNED, should bug 107445 be WONTFIX ?

Comment 7

15 years ago
missed moz 1.01 deadline
move target milestone -> future
Keywords: qawanted
Target Milestone: mozilla1.0.1 → Future
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
Status: ASSIGNED → NEW

Comment 9

13 years ago
no one is working on this ??  

firefox uses server timezone while shows document.lastModified.

it should follow the client timezone regardless of the server side. IE works 
fine and this bug should be fixed.

visit http://www.easyya.com and see the bottom of the page. It shows GMT 
always.

Comment 10

13 years ago
shijialeee@yahoo.com: thank you for volunteering, can you tell us when you'll
have this implemented?
Assignee: general → shijialeee

Updated

13 years ago
Assignee: shijialeee → general
QA Contact: desale → ian

Comment 11

13 years ago
I have no idea why i was assigned the bug. sorry, guys. 
now it's back to the public.
(Assignee)

Updated

12 years ago
Assignee: general → smontagu
Component: DOM: Level 0 → Internationalization
Keywords: intl
QA Contact: ian → amyy
Target Milestone: Future → mozilla1.9alpha
Created attachment 190579 [details] [diff] [review]
Patch rv1.0

This patch has a problem. But the problem already exists current Trunk.

If document.lastModified value is localized (currently, that is local file and
HTTP header's last-modified header doesn't exist only), the value cannot parsed
by Javascript's Date object.

E.g., if the patch is checked-in, we can see "Not specified" in Modified field
on PageInfo dialog's General tab.
Assignee: smontagu → masayuki
Status: NEW → ASSIGNED
Attachment #190579 - Flags: superreview?(jst)
Attachment #190579 - Flags: review?(jst)
Currently:
When                        | Localized?
----------------------------+-------------
Local file                  | YES
Existing last-modified      | NO
Not existing last-modified  | YES

After the patch is checked-in:
When                           | Localized?
-------------------------------+-------------
Local file                     | YES
Existing last-modified         | YES
Existing invalid last-modified | NO
Not existing last-modified     | YES
Comment on attachment 190579 [details] [diff] [review]
Patch rv1.0

r+sr=jst

Do we need a followup bug filed to fix the page info dialog, or is that already
in bugzilla?
Attachment #190579 - Flags: superreview?(jst)
Attachment #190579 - Flags: superreview+
Attachment #190579 - Flags: review?(jst)
Attachment #190579 - Flags: review+
(In reply to comment #14)
> (From update of attachment 190579 [details] [diff] [review] [edit])
> r+sr=jst
> 
> Do we need a followup bug filed to fix the page info dialog, or is that already
> in bugzilla?
> 

I cannot fine the bugs.
I will file the bugs after check-in (after 1.9 opened).
Maybe we need to change following code.

http://lxr.mozilla.org/mozilla/source/browser/base/content/pageInfo.js#390
/browser/base/content/pageInfo.js, line 390 -- var modifiedText =
formatDate(theDocument.lastModified, gStrings.notSet);

http://lxr.mozilla.org/mozilla/source/editor/ui/dialogs/content/EdPageProps.js#81
/editor/ui/dialogs/content/EdPageProps.js, line 81 -- lastmod =
editor.document.lastModified; // get string of last modified date

http://lxr.mozilla.org/mozilla/source/js/rhino/docs/faq.html#19
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/scopes.html#23
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/scriptjava.html#19
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/serialization.html#23
/js/rhino/docs/faq.html, line 19 -- var d = new Date(document.lastModified);
/js/rhino/docs/scopes.html, line 23 -- var d = new Date(document.lastModified);
/js/rhino/docs/scriptjava.html, line 19 -- var d = new Date(document.lastModified);
/js/rhino/docs/serialization.html, line 23 -- var d = new
Date(document.lastModified);

http://lxr.mozilla.org/mozilla/source/xpfe/browser/resources/content/pageInfo.js#388
/xpfe/browser/resources/content/pageInfo.js, line 388 -- var modifiedText =
formatDate(theDocument.lastModified, gStrings.notSet);
Created attachment 190611 [details] [diff] [review]
Patch rv2.0

I propose this approach.
This patch's result is same as IE6 and the result can convert Date object in JS
code.

lastModified always returns "MM/DD/YYYY hh:mm:ss" format. It can parsed by
current JS implementation. If page author needs previous patch's result, they
can write as following.

var date = new Date(document.lastModified);
document.write(date.toLocaleString());

If you grant for this patch, we can fix this bug in 1.8.
Attachment #190611 - Flags: superreview?(jst)
Attachment #190611 - Flags: review?(jst)
Created attachment 190612 [details]
testcase

In this case, if we use Patch rv2.0, we get more compatibility for IE.
Created attachment 190614 [details]
Screenshot of the testcase

Comment 20

12 years ago
please say there's going to be an annoucement about this api change somewhere
fairly visible.
Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

r=jst, but I'd like brendan to have a say on this one too.
Attachment #190611 - Flags: superreview?(jst)
Attachment #190611 - Flags: superreview?(brendan)
Attachment #190611 - Flags: review?(jst)
Attachment #190611 - Flags: review+
So if we're really just emulating IE and unbreaking sites in major locales, then
I'm all for this.  Do we feel the need to do more testing in all locales? 
Cc'ing some folks in case they have thoughts.  I'll sr shortly.

/be

Comment 23

12 years ago
The change seemed very reasonable to me. Did timeless have any concrete objections?

Comment 24

12 years ago
(In reply to comment #23)
> The change seemed very reasonable to me. Did timeless have any concrete
objections?

See bug 224703 comment #24. 

Anyway, I don't think MSIE 'hard-coded' 'MM/DD/YYYY HH:MM:SS' (have you tried MS
IE under a different locale (say, French)?) . If you want to hard-code anything,
it had better be ISO 8601-compliant (YYYY-MM-DD ......), but what really need to
be done is to format it according to the locale (corresponding to the language
of a doc.)  of a document (if it's not trivial which is indeed the case, the
current locale). (see bug 224703). 

BTW, it's 'ironic' to see IE use 'UTC' now given that we kinda refrained from
using it partly because of IE 'compatibility' (bug 224744 comment #3, bug 107445)


Jungshik:

Can you attach better patch? I think that if you cannot create better patch
soon, we should use my patch. Because my patch makes better behavior than
current behavior.

> I don't think MSIE 'hard-coded' 'MM/DD/YYYY HH:MM:SS'

I don't think so. See
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/lastmodified.asp
Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Unless jshin speaks quickly and has a better patch, I think this should go in
for 1.8b4/Firefox1.5.

/be
Attachment #190611 - Flags: superreview?(brendan)
Attachment #190611 - Flags: superreview+
Attachment #190611 - Flags: approval1.8b5?
Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Requesting explicit second-review+ from jshin.

/be
Attachment #190611 - Flags: review?(jshin1987)
Agreed.

Comment 29

12 years ago
Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Approved for 1.8b5 per bug meeting
Attachment #190611 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Flags: blocking1.8b5+

Updated

12 years ago
Whiteboard: [needs review jshin]

Comment 30

12 years ago
Please get this checked in on the branch and trunk ASAP or this bug will be
removed from the blocking list. Thanks!

Comment 31

12 years ago
just another reminder to get this landed ASAP or it may get taken off the
blocker list.

Comment 32

12 years ago
Is there anything holding this up from getting checked in? 

Comment 33

12 years ago
Time is running out. If there's an advocate for this fix with cvs access, now's
the time to step up.
Brendan and jst:

Do we need jshin's review? We don't have response from him in a half month.

Comment 35

12 years ago
This code-pattern with the formated string in the buffer, while very interesting
and self-documenting, isn't thread-safe. Variable formatedTime doesn't point to
the stack, but to anonyous global memory.

+    char formatedTime[] = "MM/DD/YYYY hh:mm:ss";
+    if (sprintf(formatedTime, "%02d/%02d/%04d %02d:%02d:%02d",
+                prtime.tm_month + 1, prtime.tm_mday, prtime.tm_year,
+                prtime.tm_hour     ,  prtime.tm_min,  prtime.tm_sec)) {
+      CopyASCIItoUTF16(nsDependentCString(formatedTime), mLastModified);
+    }

Maybe this is a bit better :

+    // "MM/DD/YYYY hh:mm:ss";
+    char formatedTime[30];
+    if (sprintf(formatedTime, "%02d/%02d/%04d %02d:%02d:%02d",
+                prtime.tm_month + 1, prtime.tm_mday, prtime.tm_year,
+                prtime.tm_hour     ,  prtime.tm_min,  prtime.tm_sec)) {
+      CopyASCIItoUTF16(nsDependentCString(formatedTime), mLastModified);
+    }

Comment 36

12 years ago
Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Sorry for the delay. I was mistaken that this already had been checked in /
Attachment #190611 - Flags: review?(jshin1987) → review+
checked-in to trunk and 1.8branch.
Thank you everyone!
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [needs review jshin]
Target Milestone: mozilla1.9alpha → mozilla1.8beta5
Created attachment 197996 [details] [diff] [review]
Patch for check-in
Attachment #190579 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.