1204818 - Move readerized content into a sandboxed iframe

Status: NEW

(Toolkit :: Reader Mode, defect, P3)

Comment 1

[Cameron McCormack (:heycam)]

This blocks the wholesale removal of <style scoped> support.

Comment 2

[Bobby Holley (:bholley)]

In bug 1354966, we're going to whitelist the reader stuff to run with the old style system. This bug blocks styling everything with stylo.

Comment 3

[Cameron McCormack (:heycam)]

Using an iframe for the content might be a bit awkward, since we would need it to be sized appropriately (so we would want some script to check the height of the content and update the iframe's height in response).

IF we can rely on it, then using Shadow DOM might be a nice solution here.  That will give us scoping of the style sheets from the content to just the content.  For the aboutReader.css sheet, we can just adjust the rules to only match the controls in the page.

Alternatively, we could add a mode to the sanitizer that can add some prefix to every style rule's selector (and use "#moz-reader-content " as that prefix).

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3)
> Alternatively, we could add a mode to the sanitizer that can add some prefix
> to every style rule's selector (and use "#moz-reader-content " as that
> prefix).

I am trying this approach.  (Although with a new function, not using nsTreeSanitizer.)

Comment 5

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3)
> Using an iframe for the content might be a bit awkward, since we would need
> it to be sized appropriately (so we would want some script to check the
> height of the content and update the iframe's height in response).

I guess this isn't a big problem. We've already been doing something like this for WebExtension popups, where we dynamically check the dimension of popup page and size the popup window accordingly.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Actually... since in reader mode, width of the article area is somehow fixed, we just need to measure the height of the document, which should be quite trivial, e.g. setting the inner document to have overflow-y: hidden, then measure the scrollHeight of the document.

I would imagine that in reader mode, the document itself is unlikely to change dynamically, so a single update after the document is loaded should be good enough in general?

Comment 8

[:Gijs (he/him)]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3)
> Alternatively, we could add a mode to the sanitizer that can add some prefix
> to every style rule's selector (and use "#moz-reader-content " as that
> prefix).

I'm unclear what you think this would fix?

Comment 9

[Cameron McCormack (:heycam)]

That would have preserved the style scoping by ensuring all the rules only ever matched something within the moz-reader-content element.

But now I understand that this bug was filed with other advantages in mind from the use of a sandboxed iframe, not just related to scoped style.

Comment 10

[:Gijs (he/him)]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #9)
> That would have preserved the style scoping by ensuring all the rules only
> ever matched something within the moz-reader-content element.

Yeah, though we'd then also need to have style pseudo-scoping for the controls CSS file, all the way to root to avoid it interacting with content, etc. :-(

> But now I understand that this bug was filed with other advantages in mind
> from the use of a sandboxed iframe, not just related to scoped style.

Right, I think there are other reasons why a (sandboxed) iframe would be a good idea.

Comment 11

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] UTC-8 (less responsive Nov 5 ~ Dec 16) from comment #7)
> Actually... since in reader mode, width of the article area is somehow
> fixed, we just need to measure the height of the document, which should be
> quite trivial, e.g. setting the inner document to have overflow-y: hidden,
> then measure the scrollHeight of the document.
> 
> I would imagine that in reader mode, the document itself is unlikely to
> change dynamically, so a single update after the document is loaded should
> be good enough in general?

Maybe this is a terribly dumb question, but why do we need to do that? Can't we just make the iframe width/height: 100% (ie take up the entire viewport) and simply make the iframe scroll instead of the parent page? In really really terrible cases the sidebar would independently also scroll or get cut-off and that is maybe sad, but it's also such an edgecase (content height < 188px in default zoom, rendering 7 lines off text and also *currently* just completely cut-off anyway!) that I don't think it should force us to choose a much more complicated implementation.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Ah, you're probably right that scrolling iframe should be enough...

Comment 13

[Cameron McCormack (:heycam)]

If we put the readerized content in an iframe, then tapping links in the iframe will navigate the frame rather than the top level window.  Is there a way to avoid that problem?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> If we put the readerized content in an iframe, then tapping links in the
> iframe will navigate the frame rather than the top level window.  Is there a
> way to avoid that problem?

Changing the links target attribute to "_top" or so?

Comment 15

[Cameron McCormack (:heycam)]

A bit annoying, since we'd now need to fetch the target document, add those target="_top" attributes, then reserialize and set it as the iframe's srcdoc, but yes that's doable.

Comment 16

[Cameron McCormack (:heycam)]

(Less annoying than I thought, since we're obviously already fetching the document to sanitize it.)

Comment 17

[Cameron McCormack (:heycam)]

(In reply to :Gijs (slow, PTO recovery mode) from comment #11)
> Maybe this is a terribly dumb question, but why do we need to do that? Can't
> we just make the iframe width/height: 100% (ie take up the entire viewport)
> and simply make the iframe scroll instead of the parent page? In really
> really terrible cases the sidebar would independently also scroll or get
> cut-off and that is maybe sad, but it's also such an edgecase (content
> height < 188px in default zoom, rendering 7 lines off text and also
> *currently* just completely cut-off anyway!) that I don't think it should
> force us to choose a much more complicated implementation.

I think we need to do this in the end to support printing.  Otherwise we'll only print one pageful of content.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

I don't think printing can be resolved in the other way either, I mean, iframe is a replaced element anyway, and it doesn't seem to me it is splittable. We should redirect the print to the inner document instead if possible.

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] UTC-8 (less responsive Nov 5 ~ Dec 16) from comment #18)
> I don't think printing can be resolved in the other way either, I mean,
> iframe is a replaced element anyway, and it doesn't seem to me it is
> splittable.

Ah, good point.

> We should redirect the print to the inner document instead if possible.

We could probably manage that, by changing something in printUtils.js, but I don't think there will be a way to make print preview work.

Comment 20

[Cameron McCormack (:heycam)]

Another reason I think we need to size the iframe: without doing this, we would need to move the #reader-header stuff into the iframe, so that it scrolls up along with the content, but if we do that, we can no longer rely on the iframe for style isolation.

Comment 21

[Cameron McCormack (:heycam)]

One thing that's not great is that we'd need to re-set the explicit height of the iframe whenever the window resizes.

Comment 22

[:Gijs (he/him)]

(In reply to Cameron McCormack (:heycam) from comment #20)
> Another reason I think we need to size the iframe: without doing this, we
> would need to move the #reader-header stuff into the iframe, so that it
> scrolls up along with the content, but if we do that, we can no longer rely
> on the iframe for style isolation.

I think that's OK - the iframe isn't supposed to have styles of its own. It might have classes/ids (which is kind of a bug that we need to address in Readability.js) but I guess it would be OK to insert the header stuff into it ourselves, maybe? To me, that seems preferable over having to adjust the height for window resizes...

Comment 23

[Cameron McCormack (:heycam)]

From the perspective of the <style scoped> usage in aboutReader.html, is the only concern that rules in aboutReaderControls.css might accidentally apply to the content?  And that if Readbility.js were changed to not leave any classes or IDs in its output, that we wouldn't need any scoping, since all of the rules in aboutReaderControls.css have classes or IDs in them?  Is there any reason we can't just strip those classes and IDs now?

For me, I'm just looking to ensure that we can enable Stylo for about:reader so we can remove Gecko's old style system.  I think I'd prefer to leave the iframe-based solution to someone else, since it's getting a bit complex, if I can solve the scoping issues in a simpler way for now.  (If not by stripping the classes and IDs from the readerized content, then by adjusting the rules in aboutReaderControls.css to ensure they never match stuff inside #moz-reader-content.)

Comment 24

[:Gijs (he/him)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> From the perspective of the <style scoped> usage in aboutReader.html, is the
> only concern that rules in aboutReaderControls.css might accidentally apply
> to the content?

Yes.

>  And that if Readbility.js were changed to not leave any
> classes or IDs in its output, that we wouldn't need any scoping, since all
> of the rules in aboutReaderControls.css have classes or IDs in them?  Is
> there any reason we can't just strip those classes and IDs now?

No. It would take work on Readability.js (in the mozilla/readability github repo) to strip everything that readability isn't adding itself, but otherwise I don't believe so.

> For me, I'm just looking to ensure that we can enable Stylo for about:reader
> so we can remove Gecko's old style system.  I think I'd prefer to leave the
> iframe-based solution to someone else, since it's getting a bit complex, if
> I can solve the scoping issues in a simpler way for now.  (If not by
> stripping the classes and IDs from the readerized content, then by adjusting
> the rules in aboutReaderControls.css to ensure they never match stuff inside
> #moz-reader-content.)

This makes sense to me, though it probably wants to happen in a different bug... sorry for the confusion / the amount of work the sandbox thing is turning out to be! It's unfortunate... :-(

Comment 25

[Cameron McCormack (:heycam)]

Filed bug 1417837 for that.

Comment 26

[kabakert]

Attachment: WIP: Bug 1204818 - Moving Readerized Content into a Sandboxed iFrame. r=niklas,mtigley

Comment 27

[Micah [:mtigley] (she/her)]

Unassigning this work until we're able to pick it up again in the future!