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.

Sign up for GitHub

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

Jump to bottom

Render each mime-part into an individual iframe #9519

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Render each mime-part into an individual iframe #9519

wants to merge 9 commits into from

Conversation

pabzm
Copy link
Member

@pabzm pabzm commented Jun 25, 2024 •
edited
Loading

Implementing #9465

TODO:

  • reading emails using mailvelope
  • check/change larry skin
  • Don't "wash" HTML content twice
  • Check reading emails with enigma
  • Fix hide_blockquote plugin

@pabzm pabzm force-pushed the message-render-iframe branch 2 times, most recently from ba35bc4 to 2804d9d Compare July 15, 2024 12:44
@pabzm pabzm self-assigned this Jul 16, 2024
@pabzm pabzm force-pushed the message-render-iframe branch 7 times, most recently from 3a8353c to edf0cc8 Compare August 30, 2024 12:06
@pabzm pabzm marked this pull request as ready for review August 30, 2024 12:15
@pabzm pabzm requested a review from alecpl August 30, 2024 12:15
@pabzm pabzm changed the title Render each mime-part into an individual iframe Draft: Render each mime-part into an individual iframe Aug 30, 2024
@pabzm pabzm marked this pull request as draft August 30, 2024 15:22
@pabzm pabzm changed the title Draft: Render each mime-part into an individual iframe Render each mime-part into an individual iframe Aug 30, 2024
@pabzm
Copy link
Member Author

pabzm commented Aug 30, 2024

@alecpl I didn't find any problems, but still want to check some more plugins. Also one test is missing. But I think the general approach won't change anymore, so a review would already make sense.

@pabzm
Copy link
Member Author

pabzm commented Sep 3, 2024

@alecpl Do you think we should keep "larry" working for future releases?

I'm not sure yet how much work it would be to make larry fit for this change, but before I even try I want to be sure that you consider it necessary. (I would deprecate it and keep it maintained for v1.6.x, but not later versions.)

@pabzm pabzm force-pushed the message-render-iframe branch 3 times, most recently from 7e519b5 to 0f5f90d Compare September 4, 2024 13:03
@pabzm
Copy link
Member Author

pabzm commented Sep 4, 2024

I'm finished with cleaning the commit history now.

@pabzm
html class: Allow to pass array as content
6631f0c 
This allows for a little cleaner code
@pabzm
Make codestyle check happy
6097159
@pabzm
Allow 'sandbox' attribute for html::iframe
288d37a
@pabzm
Fix washing html from rcube_attachment_handler
5e941b7 
This probably wasn't implemented previously because HTML-parts usually
didn't run through get.php.
@pabzm
Exit through sendExit()
a982603 
That way it's testable.
@pabzm
Render each mime part in an individual, sandboxed iframe
8becd3f
@pabzm
Styled message loading notice without meta refresh
953433a 
Meta refresh would require unsafe-inline in a CSP, which we want to
avoid.
@pabzm
Copy link
Member Author

pabzm commented Sep 5, 2024

(Rebased to latest of "master".)

@pabzm
Copy link
Member Author

pabzm commented Sep 27, 2024

@alecpl Do you think we should keep "larry" working for future releases?

I'm not sure yet how much work it would be to make larry fit for this change, but before I even try I want to be sure that you consider it necessary. (I would deprecate it and keep it maintained for v1.6.x, but not later versions.)

@alecpl Could you give me your opinion on this?

Please also review the code.

@pabzm
Copy link
Member Author

pabzm commented Oct 14, 2024

@alecpl Could you please let me know why you don't react on this PR? Are there technical concerns? Or just a lack of time? Or something different?

@alecpl
Copy link
Member

alecpl commented Oct 14, 2024

I'm sorry. It's a lack of time amplified by the significance of this change.

@pabzm
Copy link
Member Author

pabzm commented Oct 21, 2024

Would it maybe help to have renown community members review this to give a better standing ground and ease the burden?

@pabzm
Copy link
Member Author

pabzm commented Oct 23, 2024

If you're concerned about the implications for existing plugins or other code that would be hard to adapt: How about we decide to go forward with breaking or structural changes targeting versions 2.x, and at the same time commit to maintaining a version 1.x by providing bug fixes (and maybe relevant improvements) without an EOL?
I think this might help us break the deadlock of not wanting to break running setups (or businesses) but still modernising the code and project.

@pabzm pabzm marked this pull request as ready for review October 23, 2024 14:19
@pabzm
Copy link
Member Author

pabzm commented Oct 30, 2024

@johndoh @mvorisek If you have the time and headspace, I'd appreciate a review or any comment on this PR.

@alecpl
Copy link
Member

alecpl commented Nov 19, 2024

How about we decide to go forward with breaking or structural changes targeting versions 2.x, and at the same time commit to maintaining a version 1.x by providing bug fixes (and maybe relevant improvements) without an EOL?

This is exactly the plan, we do not backport new features (with a few exceptions) into old version branches. So, I'm not afraid of breaking stable versions. I'm afraid of supporting them being much harder with architectural changes like this. And I don't see that to be the main concern anyway.

I need more time for this PR. This one is the hardest one. It's not about the code quality of the PR nor the idea itself. There are obvious pros, but there are also cons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecpl alecpl Awaiting requested review from alecpl

Assignees

@pabzm pabzm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pabzm @alecpl