Closed Bug 319557 Opened 14 years ago Closed 14 years ago

Make exporting to html use hCalendar classes

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Assigned: mvl)

References

()

Details

Attachments

(2 files, 2 obsolete files)

It would be nice if exporting to html added hCalendar classes.
Attached patch patch v1 (obsolete) — Splinter Review
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)
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.
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 &lt;
  > with &gt;
  " with &quot;
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
...
(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 &amp;
>   < with &lt;
>   > with &gt;
>   " with &quot;
> 

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.
(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?
Attachment #205301 - Flags: first-review?(dmose)
(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.
Depends on: 322230
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attached patch use utf8Splinter Review
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)
Attached file example output
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+
patch checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.