Bug 12123 - HTML notices can break the notice viewer
Summary: HTML notices can break the notice viewer
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-22 13:55 UTC by Kyle M Hall
Modified: 2015-12-03 22:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12123 - HTML notices can break the notice viewer (1.02 KB, patch)
2014-04-22 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.00 KB, patch)
2014-04-22 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.62 KB, patch)
2014-11-24 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12123 - HTML notices can break the notice viewer (1.76 KB, patch)
2014-11-24 17:45 UTC, Owen Leonard
Details | Diff | Splinter Review
Screenshot (73.97 KB, image/png)
2014-11-24 21:01 UTC, Katrin Fischer
Details
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (2.20 KB, patch)
2014-11-24 23:06 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.67 KB, patch)
2014-11-25 03:53 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.84 KB, patch)
2014-11-26 13:30 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.92 KB, patch)
2014-11-27 07:25 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-04-22 13:55:12 UTC
Depending on the content of an html notice, it can cause the notice to either not be collapsible, be uncollapsible, or to be permanently collapsed.
Comment 1 Kyle M Hall 2014-04-22 13:57:51 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-04-22 13:58:33 UTC
I do not consider this patch to be an optimal solution, but it
Comment 3 Kyle M Hall 2014-04-22 13:59:39 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2014-04-22 16:31:44 UTC
Please provide a proper commit message with a test plan.
Comment 5 Kyle M Hall 2014-11-24 15:02:28 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2014-11-24 17:45:13 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2014-11-24 21:01:23 UTC
Created attachment 33882 [details]
Screenshot

I am not sure about this - it seems to work, but should it look like on the screenshot? Do we really need an iframe here?
Comment 8 Liz Rea 2014-11-24 23:06:58 UTC Comment hidden (obsolete)
Comment 9 Liz Rea 2014-11-25 03:53:50 UTC Comment hidden (obsolete)
Comment 10 David Cook 2014-11-25 03:53:57 UTC
Comment on attachment 33885 [details] [review]
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer

Review of attachment 33885 [details] [review]:
-----------------------------------------------------------------

I definitely prefer Liz's patch over Kyle's. I don't think iframes are needed at all. It looks like the problem is that we were trying to stuff HTML into a "p" element which couldn't take them as children.

The only thing stopping my sign off would be the inline CSS. I find the CSS isn't necessary when I test it, and I suppose if it is necessary for other browsers, I would just prefer it to be in a stylesheet somewhere else so we can control our styles better.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
@@ +41,4 @@
>  <h1>Sent notices for [% INCLUDE 'patron-title.inc' %]</h1>
>  
>  [% IF ( QUEUED_MESSAGES ) %]
> +    <table id="noticestable" style="table-layout: fixed">

Using Chrome on Windows, I don't find this CSS does anything. If anything, it makes the presentation slightly worse.

However, if we do include it, my only request would be to not use inline CSS and to put this in a separate stylesheet somewhere like staff-global.css or something like that.

@@ +55,4 @@
>  	    <tr>
>  		<td>
>              <a class="notice-title" href="#">[% QUEUED_MESSAGE.subject %]</a>
> +            <div class="notice">

Switching to a div fixes this problem nicely.
Comment 11 Owen Leonard 2014-11-26 13:30:52 UTC Comment hidden (obsolete)
Comment 12 David Cook 2014-11-27 01:51:31 UTC
Thinking again... I can see the merit of using an iframe instead of a div. 

After all, the div only works because "p" and "blockquote" can be children of a "div".

If a person put "html" and "body" tags into the message, those the HTML would be invalid.

*does a test*

Mmm, instead of busting the notice viewer, Chrome removes the "html" and "body" tags. (So the viewer continues to work fine, although you'll still notice the "html" and "body" tags in the message_queue.)

I've noticed libraries put "html" and "body" tags in their notices before, which is why this comes into my mind.

Chrome is pretty rad at cleaning up tag soup, but I'm not sure about Firefox or Internet Explorer...

Microsoft Outlook seems capable of handling a HTML email with "html" and "body" tags inside of a "body" tag. I suppose most email clients are pretty flexible when it comes to invalid HTML markup.

In any case, I think Liz's solution works. If any problems arise in the future, I suppose we can address them then. (Or possibly strip "body" and "html" tags when saving notice templates.)
Comment 13 Katrin Fischer 2014-11-27 07:25:26 UTC
Created attachment 33976 [details] [review]
[PASSED QA] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer

Depending on the content of an html notice, it can cause the notice to
either not be collapsible, be uncollapsible, or to be permanently
collapsed.

Test Plan:
1) Set your CHECKOUT notice to the following ( with HTML Message checked ) :
<p>The following items have been checked out:</p>

----
<blockquote> <<biblio.title>> </blockquote>
----

<p>Thank you for visiting the <<branches.branchname>> of HMCPL.</p>
2) Check out some items to a patron
3) View the patrn's notices
4) Note the notice viewer is broken ( message is not collapsed, and
   con't be collapsed ).
5) Apply this patch
6) Reload the page
7) Note the notice viewer is no longer broken

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

This patch appears to fix the problem with the minimum required change.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works nicely, no problems found.
Passes tests and QA script.
Comment 14 Tomás Cohen Arazi 2014-12-03 14:55:15 UTC
Patch pushed to master.

Thanks Kyle and Liz!
Comment 15 Chris Cormack 2014-12-03 20:17:54 UTC
Pushed to 3.18.x will be in 3.18.01