aaronpk / Monocle

Monocle is a reader app that talks to a Microsub server
https://monocle.p3k.io
MIT License
49 stars 8 forks source link

Horizontally overflowing content #39

Closed jackjamieson2 closed 5 years ago

jackjamieson2 commented 5 years ago

One of my feeds contains the following post https://www.microsoft.com/en-us/research/blog/robust-language-representation-learning-via-multi-task-knowledge-distillation/, which is taken from the feed: https://www.microsoft.com/en-us/research/feed/

This post contains a wide image inside a div with style="width: 1034px;" (see below)

<div id="attachment_588043" style="width: 1034px;" class="wp-caption aligncenter"><a href="https://www.microsoft.com/en-us/research/uploads/prod/2019/05/Robust-Language-F1_Site_05_2019_1400x788.png"><img aria-describedby="caption-attachment-588043" class="wp-image-588043 size-large" src="https://www.microsoft.com/en-us/research/uploads/prod/2019/05/Robust-Language-F1_Site_05_2019_1400x788-1024x577.png" alt="Figure 1: Architecture of the MT-DNN student model." width="1024" height="577"></a><p id="caption-attachment-588043" class="wp-caption-text">Figure 1: Architecture of the MT-DNN student model.</p></div>

In monocle, this div is rendered 1034 px wide, and so is cut off Screen Shot 2019-05-23 at 11 41 07 AM

It would be nice to handle this without cutting off any content. Two ideas:

  1. Create CSS to fit any element to the width of monocle's .e-content element. I think the following would work

    .entry .e-content * {
    max-width:100%;
    }
  2. Otherwise, adding overflow-x:scroll; to .entry .content could work (though would be a bit awkward looking)

Option 1 would look the neatest in most cases (especially photos), though in this case would shrink the image to be difficult to read. Option 2 is a bit awkward IMO, but would preserve readability for images like in this example.

aaronpk commented 5 years ago

hmm is this using Aperture as the backend or your own Microsub server? I made Aperture do quite a lot of sanitization of the HTML it pulls from feeds to avoid issues like this (as well as security issues like having script tags injected in the reader). Aperture doesn't allow style attributes through, and Monocle already has this CSS rule to set the image width to the max:

.entry .content img {
    max-width: 100%;
}
jackjamieson2 commented 5 years ago

This is using my own Microsub server. In which case, it's probably a good idea that I adopt similar sanitization as Aperture on the server-side, which I'll add to my to-do list.

What do you think about documenting sanitization practices in the Microsub spec? Maybe not necessary, but if could be helpful to be more specific that style attributes should be blocked, since clients may not account for them.

(I also still think this is something that could be added to Monocle, if only for more flexibility with different servers, but I should also handle it better on the server side)

aaronpk commented 5 years ago

I do think we should make some recommendations on sanitization that servers should do, but it's probably not something that should be a hard requirement.

Can you tell why my existing max width rule isn't doing what you want?

jackjamieson2 commented 5 years ago

Agreed re: sanitization requirements.

As for the max-width rule, it doesn't work in this case because the img is in a div with style="1034px".

Sinceimg {max-width:100%;} allows the image to be 100% relative to its parent, and the parent div has a width greater than Monocle's .content container, the image still extends past the container.

A similar, hypothetical problem occurred to me. How would Monocle handle a feed item containing a wide table? I assume it would be cut off? That would likely only be an issue in articles presenting statistics or related data, which is presumably an edge case, so maybe not worth further attention unless it comes up in practice.

jackjamieson2 commented 5 years ago

This is now addressed in Yarns, by way of changes to https://github.com/dshanske/Parse-This that sanitize divs. (Not yet pushed to Yarns Master branch but I'll close this issue since it's not a Monocle issue)