Closed Bug 1607834 Opened 5 years ago Closed 3 years ago

Separate HTML descriptions in calendar events

Categories

(Calendar :: Dialogs, enhancement)

Product:
Calendar
Component:
Dialogs
Type:
enhancement

Tracking

(thunderbird_esr78 wontfix)

Status:
RESOLVED FIXED
Milestone:
91 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: neil, Assigned: neil)

References

(Regressed 3 open bugs)

Details

Attachments

(2 files, 9 obsolete files)

Screenshot: Creating a new meeting
(deleted), image/png
Details
With more globals imported
(deleted), patch
neil
: review+
Details | Diff | Splinter Review

Exchange users can send HTML in their invitations. This translates to the X-ALT-DESC property in the iCal object. Displaying this to a Lightning recipient would look nicer than the default plain text conversion. This could be done in a sandboxed iframe, with links being redirected to the user's default browser.

I gave this a try while working on bug 1659363. From what I found online[1] the contents of outlook's X-ALT-DESC looks like this:

<!DOCTYPE HTML PUBLIC ""-W3CDTD HTML 3.2//EN""><HTML><BODY>\nhtml goes here\n</BODY></HTML>

But when I add that input to the existing mochitest, it gets converted to an empty string (by the code in the patches for bug 1659363). To see what we're dealing with, I set up a free outlook.com account but sending a calendar invite from there to a gmail account did not work for me. I'll attach a patch with my WIP on this.

[1] https://www.limilabs.com/blog/html-formatted-content-in-the-description-field-of-an-icalendar

Attached patch wip-bug-1607834-x-alt-desc-property.diff (obsolete) (deleted) — — Splinter Review

Work in progress.

Assignee: nobody → paul
Status: NEW → ASSIGNED
Summary: Support displaying HTML invitations → Support X-ALT-DESC property for displaying calendar item descriptions with HTML formatting

Last I looked at this, Paenglab and aleca helped by sending me invitations from Outlook with HTML formatting in the description, to see exactly what we would be getting from Outlook. There were other custom X-MICROSOFT-... properties, but I did not see an X-ALT-DESC property for the event in the invitation.

When I looked at the email with the invite and did "view source" at the bottom there is the "text/calendar" part, but it seems to be encoded in "base64":

Content-Type: text/calendar; charset="utf-8"; method=REQUEST
Content-Transfer-Encoding: base64

QkVHSU46VkNBTEVOREFSDQpNRVRIT0Q6UkVRVUVTVA0KUFJPRElEOk1pY3Jvc29mdCBFeGNoYW5n
[...]

Accepting the invitation and then exporting it as ICS, I see various X-MICROSOFT-NNN properties, but not X-ALT-DESC. The description property is just plain text.

The WIP patch should work for invitations/events with an X-ALT-DESC property, but the test, based on the example I found of what Outlook does, is not passing. So it's just the test part that is WIP.

neil, did you have a proper test case you can attach?

Assignee: paul → neil
Blocks: 1666296
Severity: normal → S2
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Attached patch Proposed patch (obsolete) (deleted) — — Splinter Review

This patch brings an HTML editing experience to the Lightning description box. HTML is saved in the X-ALT-DESC property but also downconverted to plain text for the DESCRIPTION property, so third parties will be no worse off than before. Google Calendar invitations are automatically demangled so that their HTML is placed in X-ALT-DESC, again with plain text downconversion for the DESCRIPTION property.

Attachment #9207169 - Flags: review?(philipp)
Attachment #9207169 - Flags: review?(geoff)
Attachment #9172440 - Attachment is obsolete: true
Attached image Screenshot: Creating a new meeting (deleted) —
Attached patch Tweaked patch (obsolete) (deleted) — — Splinter Review
Attachment #9207169 - Attachment is obsolete: true
Attachment #9207169 - Flags: review?(philipp)
Attachment #9207169 - Flags: review?(geoff)
Attachment #9208727 - Flags: review?(philipp)
Attachment #9208727 - Flags: review?(geoff)

Comment on attachment 9208727 [details] [diff] [review]
Tweaked patch

r=BenB

Attachment #9208727 - Flags: review+

Are there better reviewers I should be using for this?

No, geoff is the right reviewer.

I've spent far too long looking at this so I'm just going to dump my thoughts in a comment.

  • I've got reservations about encouraging the use off an off-spec property. It's fine in some situations but as soon as you leave the Outlook ecosystem most tools will just ignore it. However as long as you're making a reasonable equivalent plain text description that probably doesn't matter. We should be very restrictive about what formatting is allowed.

  • I think we should avoid displaying descriptions in iframes. They just make things more complicated. For example in the ICS import dialog the iframe's document is being loaded with the description then replaced with about:blank. At least I think that's what's happening, nothing is displayed. If we do have an HTML description we can take a subset of the HTML and put it in the parent document. (You're already doing this for tool tips in the main window.) If we have a plain text description we can do with it what we do now.

  • Plus, displaying stuff in an iframe is really ugly, particularly if there's a big difference between the HTML font and the UI font. Even our defaults look bad, e.g. on my system with the default fonts the HTML is 16px and the UI is 13.33px.

  • It doesn't play nicely with Google Calendar's inclusion of some HTML in the description property. Yes, that's not spec either but with your patch we'd display as plain text what are clearly HTML tags. That would be a backwards step.

  • The minimum size of the description editor is way too big. It overpowers the event dialog. The user can always make the window bigger, but if they never use descriptions it's just ridiculous.

I'm not opposed to your proposed changes, but they need more work.

Attachment #9208727 - Flags: review?(philipp)
Attachment #9208727 - Flags: review?(geoff)

I think we should also get clarity on to what degree X-ALT-DESC is used even by Microsoft. According to the investigations above it would appear it's not used for the normal case even by them. Is it only for some specific versions of Outlook? Under which special circumstances does it appear?

I've spent far too long looking at this

Let's go over this together in real time. There's a lot of thoughts in this patch, we've been thinking long and hard how to do this properly, and I think this is a clean and straight-forward implementation. I'd like to walk you through it.

However as long as you're making a reasonable equivalent plain text description that probably doesn't matter

Yes, we're taking specific care that the plaintext description always matches the HTML description, and vise versa. We use the same code as for HTML emails and their plaintext counterparts.

Same as in email, it's important to differentiate what is plaintext and what is HTML. Heuristics will not work: What about a software developer or HTML designer that writes sample HTML code in the event description? What Google does - putting HTML in the plaintext description - is directly violating the spec.

Google Calendar's inclusion of some HTML in the description property. Yes, that's not spec either

Exactly. (See above)

We do support Google Calendar's HTML descriptions, but it's in a different place in the code.

we should avoid displaying descriptions in iframes

The <browser content=primary> is critically important for security reasons, because this is third-party content from emails, which may be malicious from an attacker. It's true that we sanitize the HTML, but the sanitizer is not 100% tamper-proof, and it's explicitly not designed to be the only security barrier, but only as an additional protection. I should know, because I invented and wrote the first version of this sanitizer in Gecko that we're using. ;-) It needs to be used in addition to the browser protection in untrusted iframes, not as replacement.

This is critically important, because we're displaying HTML from a potential attacker with system privileges.

We're lucky, the spec actually has a way to store HTML descriptions, see RFC 2445 Section 4.2.1 and the example in Section 4.2

The following is conforming to the spec:

DESCRIPTION;ALTREP="data:text/html;<h1>Some text</h1>":Some text

(In reply to Ben Bucksch (:BenB) from comment #13)

We do support Google Calendar's HTML descriptions, but it's in a different place in the code.

ISTR this didn't work when I tested it, I probably wouldn't have bought it up otherwise.

The <browser content=primary> is critically important for security reasons,

Okay, so what are you doing with the tooltips? The same issues apply and you're just adding the HTML to the document there.

Rebase this, fix the UI issues below, and I'll have another look.

  • The iframe content should match the look of the rest of the dialogs. In particular the font face and size.
  • The editor toolbar has a different set of tools from the compose window. Why? I know that we don't want all of them, but what we do have should be in the same positions and look the same (not for adding a link, that obviously has to be different).
  • The minimum size of the editor is far too big. It may need a bit more height to make the toolbar fit but that's it. If people want it bigger they can make it so.
  • Disable the toolbar tools when the description isn't being edited. As a bonus side-effect this will prevent them drawing attention from the rest of the dialog so much.

I'd also like to get clarity on comment 12.

Geoff wrote:

tooltips? The same issues apply and you're just adding the HTML to the document there

You're right, and that's a potential security bug. Thanks for pointing that out! We'll just display the plaintext version in the tooltip.

The iframe content should match the look of the rest of the dialogs. In particular the font face and size.

Yes.

The editor toolbar ... I know that we don't want all of them, but what we do have should be in the same positions and look the same.

Fair enough.

We will work on the points you mentioned.

Magnus wrote:

to what degree X-ALT-DESC is used even by Microsoft

Yes, we can research that in more detail.

FWIW, the ALT-DESC;FMTTYPE is partially per spec, see RFC 2445 Section 4.2.8 for FMTTYPE. Whereas ALT-DESC is a random custom property.

DESCRIPTION:Some text
ALT-DESC;FMTTYPE=text/html:<h1>Some text<h1>

Magnus wrote:

to what degree X-ALT-DESC is used even by Microsoft

It seems that only Outlook 2013 used ALT-DESC and already Outlook 2016 stopped using it, so indeed, it makes no sense to use that.

As noted in comment 14, the iCal spec actually explicitly specifies a way to include HTML in iCal:

The definition of ALTREP says: "The parameter specifies a URI that points to an alternate representation for a textual property value", so any URI scheme is valid. That means we can also use data: URLs in there.

That means there is actually a specced way to store HTML descriptions in iCal.

The following is conforming to the spec:

DESCRIPTION;ALTREP="data:text/html;<h1>Some text</h1>":Some text

So, we're using that now, fully conformant to the spec.

Summary: Support X-ALT-DESC property for displaying calendar item descriptions with HTML formatting → HTML descriptions in calendar events
Attached patch Fix v3 - Review comments addressed (obsolete) (deleted) — — Splinter Review

Review comments addressed, ready for re-review.
Patch by Neil
r+ BenB

Attachment #9208727 - Attachment is obsolete: true
Attachment #9225336 - Flags: review?(geoff)
Summary: HTML descriptions in calendar events → Separate HTML descriptions in calendar events
Attached patch Updated to trunk, linted, tests fixed (obsolete) (deleted) — — Splinter Review

Also pushed to try: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=689626052d48f545b9c1b929c3524e6cf7550fbe

Attachment #9225336 - Attachment is obsolete: true
Attachment #9225336 - Flags: review?(geoff)
Attachment #9226407 - Flags: review?(geoff)
Comment on attachment 9226407 [details] [diff] [review] Updated to trunk, linted, tests fixed Review of attachment 9226407 [details] [diff] [review]: ----------------------------------------------------------------- On the whole I'm fairly happy with this. ::: calendar/base/content/item-editing/calendar-item-iframe.js @@ +17,5 @@ > /* global MozElements */ > > /* import-globals-from ../calendar-ui-utils.js */ > /* import-globals-from ../dialogs/calendar-dialog-utils.js */ > +/* import-globals-from ../../../../mail/components/compose/content/editorUtilities.js */ Our convention is files that are further away go higher up the list. The CI linter seems to choke on this patch. I'm unsure why and there's probably nothing you can do about it, but it's probably something to do with this line. Just thought I'd mention it. @@ +4268,5 @@ > +function onCustomizeFormatToolbar() { > + let wintype = document.documentElement.getAttribute("windowtype"); > + wintype = wintype.replace(/:/g, ""); > + > + window.openDialog( I don't think there's a need for customising the toolbar. You can't do it in message composition. Besides, it's barely working, I could drag things from the palette to the toolbar but that's it. ::: calendar/base/content/item-editing/calendar-item-iframe.xhtml @@ +675,5 @@ > + <toolbar is="customizable-toolbar" id="format-toolbar" > + class="chromeclass-toolbar themeable-full" > + customizable="true" nowindowdrag="true" > + defaultset="cut-button,copy-button,paste-button,separator,paragraphButton,separator,boldButton,italicButton,underlineButton,separator,ulButton,olButton,outdentButton,indentButton,separator,AlignPopupButton,linkButton,smileButtonMenu" > + context="format-toolbar-context-menu"> Wrapped attributes should be on one line each, except is= and id=, they're fine as they are. This toolbar has some style issues (e.g. on my machine it's dark when it should be light) but I'll tolerate them. Get :Paenglab to have a look when it lands. @@ +679,5 @@ > + context="format-toolbar-context-menu"> > + <toolbarbutton id="cut-button" class="toolbarbutton-1" > + command="cmd_cut" > + tooltiptext="&cutButton.tooltip;" > + observes="cmd_renderedHTMLEnabler"/> We don't need clipboard buttons. @@ +782,5 @@ > + wantdropmarker="true" > + class="formatting-button" > + tooltiptext="&SmileButton.tooltip;" > + observes="cmd_renderedHTMLEnabler"> > + <menupopup id="smilyPopup"> I think we should fix the existing uses instead of perpetuating bad spelling. ::: calendar/base/src/CalIcsParser.jsm @@ +356,5 @@ > + * Fixes up a description of a Google Calendar item > + * > + * @param item The item to check > + */ > +function fixGoogleCalendarDescription(item) { This'd be better in calViewUtils.jsm.
Attachment #9226407 - Flags: review?(geoff)
Attachment #9226407 - Flags: review-
Attachment #9226407 - Flags: feedback?(mkmelin+mozilla)

I forgot to mention: please make the Ctrl+B, Ctrl+I and Ctrl+U keyboard shortcuts work in the editor. People will expect that.

I don't think there's a need for customising the toolbar.

Agreed.

Ctrl+B, Ctrl+I and Ctrl+U keyboard shortcuts

Agreed.

We don't need clipboard buttons.

[EDITED:] OK

(Mistake corrected)

Attached patch Addressed review comments (obsolete) (deleted) — — Splinter Review

I had to change the New Event key from Ctrl+I to Ctrl+E so that I could use Ctrl+I for italic.

I fixed smilyPopup in all the places it appears.

I only included toolbar customisation because the dialog's main toolbar already had it.

The linter worked fine for me locally, so...

Attachment #9226407 - Attachment is obsolete: true
Attachment #9226407 - Flags: feedback?(mkmelin+mozilla)
Attachment #9226541 - Flags: review?(geoff)

I had to change the New Event key from Ctrl+I to Ctrl+E so that I could use Ctrl+I for italic.

Given that we're already in the "New event" (or "Edit event") dialog, adding yet another event from that particularly window is not going to be a common operation, so I think your choice makes sense to change Ctrl-I to italic. I'd even completely remove the keyboard shortcut for the "New event" action from "New event" window.

+  <script src="chrome://messenger/content/customizable-toolbar.js"/>

I forgot to remove this line. This was an oversight on my part. Sorry about that.

Attached patch Addressed review comments (obsolete) (deleted) — — Splinter Review

(removed unnecessary line)

Attachment #9226541 - Attachment is obsolete: true
Attachment #9226541 - Flags: review?(geoff)
Attachment #9226847 - Flags: review?(geoff)
Comment on attachment 9226847 [details] [diff] [review] Addressed review comments Review of attachment 9226847 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog.xhtml @@ +183,1 @@ > command="cmd_item_new_event"/> I agree with Ben in comment 29, there's no need for this, let's remove it and new-task-key.
Attachment #9226847 - Flags: review?(geoff) → review-
Attached patch Removed new event and task keys (obsolete) (deleted) — — Splinter Review
Attachment #9226847 - Attachment is obsolete: true
Attachment #9227910 - Flags: review?(geoff)

Comment on attachment 9227910 [details] [diff] [review]
Removed new event and task keys

Alrighty, let's do this. Thanks.

Attachment #9227910 - Flags: review?(geoff) → review+
Comment on attachment 9227910 [details] [diff] [review] Removed new event and task keys Review of attachment 9227910 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I'm changing my mind, I found some other things that need tidying up. ::: calendar/base/content/item-editing/calendar-item-iframe.xhtml @@ +673,5 @@ > + <tabpanel id="event-grid-tabpanel-description" > + orient="vertical"> > + <toolbox mode="icons"> > + <toolbar class="chromeclass-toolbar themeable-full" > + nowindowdrag="true"> I figured out what's going on with the toolbar colouring. Add the class `inline-toolbar`. Add these styles: `-moz-context-properties: fill; fill: currentColor;` (in the stylesheet, not inline, so you'd better give the toolbar an ID too). @@ +803,5 @@ > + oncommand="goDoCommandParams('cmd_smiley', '&#128567;')"/> > + </menupopup> > + </toolbarbutton> > + </toolbar> > + <toolbarpalette/> We don't need this palette any more. ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css @@ +312,5 @@ > } > > +#cut-button { > + list-style-image: url("chrome://messenger/skin/icons/cut.svg"); > +} These buttons are gone now, no need for the styles.
Attachment #9227910 - Flags: review+ → review-

Comment on attachment 9228348 [details] [diff] [review]
Fixed toolbar and removed more cruft

r+ from my side. (But of course letting Geoff give his review.)

Attachment #9228348 - Flags: review+

Try push: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ff2aa2a674aa6d3d8dc9025d766dff1f670985a1

(Previous push didn't build because its parent was too old. Sigh...)

Comment on attachment 9228348 [details] [diff] [review]
Fixed toolbar and removed more cruft

I made some additional changes to clean up a test failure and the linting death, and made another Try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=bffa02dc0233e023a8833670a5b9617e664726a0

Take my additional changes, add /* import-globals-from ../../../../mail/components/compose/content/editor.js */ to calendar/base/content/item-editing/calendar-item-iframe.js, and you can ship this. Assuming the new Try run has no new failures.

Attachment #9228348 - Flags: review?(geoff) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/8b81371bad8f
Support HTML descriptions for events r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
status-thunderbird_esr78: --- → wontfix
Target Milestone: --- → 91 Branch

Yay! Thanks so much to everybody involved, particularly to Neil and Geoff!

Regressions: 1727061
Regressions: 1726400
Regressions: 1728276
Regressed by: 1738611
No longer regressed by: 1738611
Regressions: 1738611

Is there a way to have text event descriptions by default rather than HTML?
Thunderbird 78 worked fine and it showed Teams links.

Now TB 91 messes with HTML and, when you copy some text from a web page, the event description looks ugly and quite different from what it was originally in Firefox.

Also the links are not correctly transferred (as shown in bug 1737664), probably a side effect of this HTML “enhancement”.

In many situations a textual event description with links well copied, detected and presented is good enough. So I would prefer to have events stored and presented as in TB 78.

Regressions: 1745602
Blocks: 1748002
Regressions: 1767392
Regressions: 1778974
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: