Remove enable-frame-flattening and iframe for message body #732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ibuclaw
force-pushed
the
remove_iframes
branch
4 times, most recently
from
08170aa
to
5cc22e2
Compare
As of WebKitGTK 2.40, enable-frame-flattening is no longer supported, the property does nothing, resulting in all messages in the thread viewer being cut off at around 150px, making all long messages unreadable. I initially had a look into seeing whether the iframe height could be set programmatically with `webkit_dom_html_iframe_element_set_height` from the content of the iframe via `webkit_dom_element_get_scroll_height`. However nothing worked at page load time of an email thread, the content scroll height remained at 150 throughout the entire `ThreadView_on_load_changed` pass (because iframes are rendered lazily after the main window thread has finished? Can't think of any other reason why this was observed). The scroll height was only found to update after the thread had finished loading, and triggering a hide/show toggle on each message. Another option would be to use the JavaScriptCore API, and have all the logic to manipulate the height of the iframe in JS. Two possible problems with this: Firstly `enable-javascript` is explicitly disabled in Astroid's WebKit settings; second, potentially all of the `webextension` code may need to be rewritten in JS - given the huge amount of deprecation warnings when building Astroid, this likely will have to be done in anger at some point in the future anyway. I gave up going down the path of manipulating the iframe after setting `srcdoc`, and replaced the iframe with a div instead. CSS has been fixed up and theme version bumped as it would be a breaking change for all existing users that override the theme of the UI. I can't see any concernable difference between before and after when viewing both text/html and text/plain messages, though it is difficult to gauge as my starting point is broken messages. The `AstroidExtension::reload_images` code path is the least tested of all the changes. The minimal check I did to see whether the logic is still fine was to send myself an email with a CID attachment, and observe it render correctly after pressing C-i in thread view. As far as this change is concerned, this is one way to fix astroidmail#720 that suits my use of Astroid. I'm not convinced that it is the correct way to go about it though, as I have no reason to dismiss the rationale for using iframes in the first place - see comments delete by this patch that make reference to style issues, deadlocks and security concerns by not having the content contained to its own iframe.
|
Thank you. The idea with the iframe was that it was easier to separate the content from the other content, so that it would be possible to disable javascript etc and the content of one email would not be able to access the other content. In my other failed attempt at redoing astroid in typescript / javascript / rust (whatever runs in the browser) I had to give that up, and rather sanitize the email. Since this is such a common problem I think there are good packages for doing that, but maybe not in C++.. ? https://github.com/gauteh/ragnarok/blob/master/astroid/src/components/thread-view/PartView.tsx#L50 ( I see that I somehow managed to use iframe's here after all -- it is obviously not possible to disable javascript in that case ) |
|
@ibuclaw Can we merge this? |
jorsn
added a commit
to jorsn/astroid
that referenced
this pull request
Sep 16, 2023
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
yzhs
pushed a commit
to yzhs/astroid
that referenced
this pull request
Mar 30, 2024
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
jorsn
added a commit
to jorsn/astroid
that referenced
this pull request
May 28, 2024
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
jorsn
added a commit
to jorsn/astroid
that referenced
this pull request
Jun 3, 2024
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
jorsn
added a commit
to jorsn/astroid
that referenced
this pull request
Jun 4, 2024
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
yzhs
pushed a commit
to yzhs/astroid
that referenced
this pull request
Sep 22, 2024
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consider this a WIP / broken change. It's enough to fix issues relating to the removal of frame flattening in #720 for me that does not detriment the ability for Astroid to function. Based on the comments in the code, there looks to be a very real reason for using iframes for the message content, however I wasn't able to get it to render correctly with the very little knowledge and means of using the C++ API of WebKitGTK, and I can't possibly devote any more time to working out the control flow of the various parts (it's difficult enough stepping through two or more gdb sessions for all the separate processes spawned just to view an email thread).
Maybe this might inspire someone to make a proper fix. :-)
As of WebKitGTK 2.40, enable-frame-flattening is no longer supported, the property does nothing, resulting in all messages in the thread viewer being cut off at around 150px, making all messages longer than 2-3 lines unreadable.
I initially had a look into seeing whether the iframe height could be set programmatically with
webkit_dom_html_iframe_element_set_heightfrom the content of the iframe viawebkit_dom_element_get_scroll_height. However nothing worked at page load time of an email thread, the content scroll height remained at 150 throughout the entireThreadView_on_load_changedpass (because iframes are rendered lazily after the main window thread has finished? Can't think of any other reason why this was observed). The scroll height was only found to update after the thread had finished loading, and triggering a hide/show toggle on each message.Another option would be to use the JavaScriptCore API, and have all the logic to manipulate the height of the iframe in JS. Two possible problems with this: Firstly
enable-javascriptis explicitly disabled in Astroid's WebKit settings; second, potentially all of thewebextensioncode may need to be rewritten in JS - given the huge amount of deprecation warnings when building Astroid, this likely will have to be done in anger at some point in the future anyway.I gave up going down the path of manipulating the iframe after setting
srcdoc, and replaced the iframe with a div instead. CSS has been fixed up and theme version bumped as it would be a breaking change for all existing users that override the theme of the UI.I can't see any concernable difference between before and after when viewing both text/html and text/plain messages, though it is difficult to gauge as my starting point is broken messages.
The
AstroidExtension::reload_imagescode path is the least tested of all the changes. The minimal check I did to see whether the logic is still fine was to send myself an email with a CID attachment, and observe it render correctly after pressing C-i in thread view.As far as this change is concerned, this is one way to fix #720 that suits my use of Astroid. I'm not convinced that it is the correct way to go about it though, as I have no reason to dismiss the rationale for using iframes in the first place - see comments delete by this patch that make reference to style issues, deadlocks and security concerns by not having the content contained to its own iframe.