Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2184: Allow the use of the HTML <details> tag #2184

Open
wants to merge 1 commit into
base: master
from

Conversation

@ananace
Copy link

commented Jul 16, 2019

Rendered

Fixes #2182

@turt2live turt2live changed the title Allow the use of the HTML <details> tag MSC2184: Allow the use of the HTML <details> tag Jul 16, 2019

@turt2live
Copy link
Member

left a comment

Thank for the detailed MSC! It's worth mentioning that the HTML restrictions in the spec are just recommendations (albeit strong ones) rather than requirements. This does change the language of the MSC slightly, but not drastically enough to warrant fixing I don't think.

proposals/2184-allow-html-details.md Outdated Show resolved Hide resolved
Allow the use of the HTML <details> tag
Signed-off-by: Alexander Olofsson <ace@haxalot.com>

@ananace ananace force-pushed the ananace:html_details branch from 2921772 to 99bd46c Jul 16, 2019

@ananace

This comment has been minimized.

Copy link
Author

commented Aug 12, 2019

I was a bit bored, so I did a quick mockup of the functionality by modifying the rendered Riot-web HTML for a message, just to see how it could look without anything more than a change in the allowed tag list.

MCS2182

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I'm getting myself into the habit of queuing things into the FCP queue:

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@ara4n

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I am torn - on one hand, i really like the idea. on the other hand, i can see it being quite painful for those (e.g. Riot/iOS and Riot/Android and Seaglass) who are wrapping their own HTML renderer currently. And as @ananace quite rightly notes in the MSC, it does collide with #1767, which in theory could provide equivalent behaviour without depending on HTML to do it, and be more flexible in terms of allowing all sorts of different content/fallbacks to be togglable on & off.

What do others think?

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

my perspective is that MSC1767 is a great thing we should definitely do, however in the interest of progress we should shove this one through. The spec does not have a requirement on any of the HTML rendering, it just leaves it as a strong recommendation. This does technically mean that implementations could just add support if they wanted to without the spec, however this seems trivial enough to just merge into the spec's recommendation.

@ananace

This comment has been minimized.

Copy link
Author

commented Aug 12, 2019

I also realized that I forgot to mention that one difference between this MSC and MSC1767 is that <details> would also support recursive usage - as well as the mentioned act of just using it multiple times in a single message.

As a small example of how that could be used for bot responses; (here as an imaginary message in response to some kind of monitoring query - or a notification)

<details>
  <summary>Host: <b>UP</b>, CPU: <b>CRIT</b>, Load: <b>WARN</b></summary>
  <b>CPU</b>: 100%<br/>
  <b>Load</b>: 15.04 7.4 4.2<br/>
  <b>RAM</b>: 24%<br/>
  <details>
    <summary><b>Disk</b>: 20%</summary>
    <ul>
      <li><b>/</b>: 12% 500GB</li>
      <li><b>/home</b>: 24% 1TB</li>
      <li><b>/var/log</b>: 74% 32MB</li>
    </ul>
  </details>
</details>
@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I agree with @turt2live in that I don't want to block this on extensible events like so many other things.

We shouldn't block clients from using this just because some have chosen technologies which make ti difficult to implement.

@KitsuneRal

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

I, too, am torn on this. I fully see the value of that tag but, aside from overlapping (I wouldn't say "colliding") with extensible events, this tag only appeared in HTML5 (I know, I know...) and, as far as I can see, is not supported by IE. Most shamefully, HTML5 is not represented in the "Supported HTML subset" for rich text in Qt5 either, which means that - on top of poor things @ara4n mentioned - Nheko, Spectral, and Quaternion will have to go through unusual pains to render <details>.

With that said, the fallback is pretty clear: stripping the <details> tag and the <summary> contents entirely allows to still properly render the message. It's probably worth it to add this consideration to MSC.

One alternative to consider is to allow an attribute in m.room.message events that advises clients to render those events initially "collapsed" with a given content shown instead. This would emulate <details> functionality even if HTML is not used at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.