Closed
Bug 319557
Opened 19 years ago
Closed 19 years ago
Make exporting to html use hCalendar classes
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mvl, Assigned: mvl)
References
()
Details
Attachments
(2 files, 2 obsolete files)
6.68 KB,
patch
|
dmosedale
:
first-review+
|
Details | Diff | Splinter Review |
3.17 KB,
text/html
|
Details |
It would be nice if exporting to html added hCalendar classes.
Assignee | ||
Comment 1•19 years ago
|
||
This patch makes it so. It also switches to output to use dl, dt and dd instead of messing with b tags.
Attachment #205301 -
Flags: first-review?(dmose)
Comment 2•19 years ago
|
||
I had a bunch more discussion with Hixie and Tantek, and it turns out that hCalendar can, in fact, be used with HTML. I'll try and review tomorrow.
Comment 3•19 years ago
|
||
I think we should escape the text (title, location, ...) before writing to html to avoid any potential parser errors. With that I mean replacing certain characters like & with & < with < > with > " with "
Comment 4•19 years ago
|
||
I tried the patch and here are my additional comments: 1) missing ; in line 123: str += "</dl>\n" 2) export fails if no description is set in event: Error: desc has no properties Source File: file:///C:/sb/dist/bin/components/calHtmlExport.js Line: 107: if (desc.length > 0) { 3) The exported file specifies UTF-8 character encoding but is not UTF-8 encoded. For example the character 'ö' is displayed as '?' in Firefox when selecting utf-8 encoding. When switching to a non unicode encoding like iso-8859-1 it is properly displayed as 'ö'. 4) My opinion: Using a definition list does not look good as output. The result looks something like: Title Sample All Day Event When 2005/12/17 00:00:00 floating – 2005/12/18 00:00:00 floating Location Somewhere Description Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Curabitur ante ipsum, tristique non, aliquam eget, hendrerit eget, tortor. Title ...
Comment 5•19 years ago
|
||
(In reply to comment #3) > I think we should escape the text (title, location, ...) before writing to html > to avoid any potential parser errors. With that I mean replacing certain > characters like > & with & > < with < > > with > > " with " > In fact, until we do this, we're creating HTML that is subject to cross-site scripting attacks. e.g. someone tricks a user into subscribing to and exporting a calendar which has hostile HTML / JS in it. That said, I'd be ok with filing a separate bug on that, since that problem existed before and independently of this patch.
Comment 6•19 years ago
|
||
(In reply to comment #4) > I tried the patch and here are my additional comments: > > 1) missing ; in line 123: str += "</dl>\n" > > 2) export fails if no description is set in event: > Error: desc has no properties > Source File: file:///C:/sb/dist/bin/components/calHtmlExport.js > Line: 107: if (desc.length > 0) { > > 3) The exported file specifies UTF-8 character encoding but is not UTF-8 > encoded. For example the character 'ö' is displayed as '?' in Firefox when > selecting utf-8 encoding. When switching to a non unicode encoding like > iso-8859-1 it is properly displayed as 'ö'. The above do need to be addressed before the patch is ready for review, I think. > 4) My opinion: Using a definition list does not look good as output. The result > looks something like: > Title > Sample All Day Event > When > 2005/12/17 00:00:00 floating – 2005/12/18 00:00:00 floating > Location > Somewhere > Description > Lorem ipsum dolor sit amet, consectetuer adipiscing elit. > Curabitur ante ipsum, tristique non, aliquam eget, hendrerit eget, tortor. > Title > ... So your objection is that DL has default style rules in most browsers that you don't like? If that is the consensus (I don't have any particular feeling about this), possible alternatives I can think of include: * override those style rules in our HTML * output DIVs with style rules or was there something else you had in mind?
Updated•19 years ago
|
Attachment #205301 -
Flags: first-review?(dmose)
Comment 7•19 years ago
|
||
(In reply to comment #6) > So your objection is that DL has default style rules in most browsers that you > don't like? If that is the consensus (I don't have any particular feeling > about this), possible alternatives I can think of include: > * override those style rules in our HTML No objection, just my privat opinion. (There's no accounting for taste.) Mvl already experimented with some styling rules that will make the output more presentable.
Assignee | ||
Comment 8•19 years ago
|
||
This patch uses e4x, to reduce the need for escaping etc. It also uses divs, instead of dl/dt/dd. It has styles defined, to make the output looka bit better. And it uses calIDateTimeFormatter, for l10n friendlyness.
Attachment #205301 -
Attachment is obsolete: true
Attachment #208475 -
Flags: first-review?(dmose)
Assignee | ||
Comment 9•19 years ago
|
||
Previous patch didn't properly convert to utf8, this patch does. (The conversion is in the exporter, because not all exporters will want to output utf8. we can't force it on them)
Attachment #208475 -
Attachment is obsolete: true
Attachment #208487 -
Flags: first-review?(dmose)
Attachment #208475 -
Flags: first-review?(dmose)
Assignee | ||
Comment 10•19 years ago
|
||
Comment 11•19 years ago
|
||
Comment on attachment 208487 [details] [diff] [review] use utf8 This looks great! A couple of minor nits: >+ // Include the end date anyway, even when empty, because the dtend >+ // class should be there, for hCalendar goodness. >+ var joiner = ""; >+ if (endstr.value) { >+ joiner = " - "; > } "separator" seems like it would be a more meaningful variable name than "joiner". Also, if you could put a comment somewhere in the generated HTML about the intent of the summarykey class, that'd be great. r=dmose
Attachment #208487 -
Flags: first-review?(dmose) → first-review+
Assignee | ||
Comment 12•19 years ago
|
||
patch checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•