-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
getLineHTMLForExport - Fixes #2486 but breaks plugins #3268
Conversation
I'd merge this but it would make too much work for me. I'd be more inclined to merge if you can also patch my plugins for me please? :) |
Will do that |
@Gared I think it's ok, it doesn't change anything in this fixes behaviour so I added it |
@JohnMcLear Could you merge this now? |
@ilmartyrk can you resolve the merge conflicts please? |
Thank you @JohnMcLear !!! Can you please make a new release on it too. I've also listed all the issues that I know of, that this update fixes. |
If releasing this change breaks some plugins we should probably have an upgrade strategy (for example, we could invite the authors to issue a new release for their plugin, starting with the ones for which ilmartyrk has already wrote a pull request). Current Etherpad version is 1.6.6. Since this is a change that somewhat breaks compatibility, I would propose to bump Etherpad version to 1.7.x (a plugin with a sane dependency spec could make good use of this). |
@muxator I have to say that I downloaded all the plugins, that I could find from the official plugins lisy, that had html export hook and most of them were not working anyway. But I believe that 1.7.x would be the correct way to move forward because update isn't backwards compatible. |
This test was broken by #3268 (getLineHTMLForExport - Fixes #2486 but breaks plugins). This change aims to facilitate debugging. Expected: <!doctype html><html><body><ul class="bullet"><li>one</li><li>2</li></ul><br><ul><ul class="bullet"><li>ul2</li></ul></ul></body></html> Received: <!doctype html><html><body><ul class="bullet"><li>one</li><li>2</ul><br><ul class="bullet"><li><ul class="bullet"><li>ul2</ul></li></ul></body></html>
Let's investigate #3425 before unleashing 1.7 :) |
This puts issue: #3383, PR: #3413 (Use keydown instead of keypress on Firefox) directly on top of bacc37c, which is the last commit before fe08d2a merged #3268 (getLineHTMLForExport - Fixes #2486 but breaks plugins). This is necessary for showing that: - bacc37c was passing client side tests on firefox Visit `http://<yourhost>/tests/frontend/` using firefox. - 2be873e forgot to update the client side tests. You cannot test it since that commit was mad on top of other changes, hence this graft - in this commit there are 20 failures with firefox: passes: 82 failures: 20 duration: 261.84s
@muxator Sorry for not reacting, now I finally created a new test that will blow up in the previous version and works fine in the updated one. This test shows how bad the final result was when two different types of lists were inserted one after another. Hope that you are good with it. |
@ilmartyrk, thank you for your work. The new test is clear, and - as desired - breaks before #3268 (fe08d2a), and works afterwards. Perfect. For clarity, the test we are talking about is contained in 06cf3ff and is currently part of #3408. Some nits:
|
… lists This commit replaces an old test with a new, different one. Reasons for removing the old test: - the old list test was introduced in cadb83a with an explicit attempt to import an invalid HTML and see if it could recover from it - in 5967e08 the code base finally managed to export a meaningful HTML (but we do not know if other bugs were introduced in pursuing this) - the old test seemed to aim at attaining resiliency, but correctness should be addressed before that Modified by muxator. See discussion in: #3268 (comment)
@ilmartyrk, I went on and committed 718b175 on your behalf (merged with b466acd), so this PR is deservedly covered by a test. Thank you again for the help! |
The documentation for http://etherpad.org/doc/v1.7.0/#index_exporthtmladditionaltags It's also unclear now how (for example) one might even fix the (By the time the hook is called the |
With the merge of ether/etherpad-lite#3268, `getLineHTMLForExport` should not return anything, but instead set the HTML content of the line on `context.lineContent`. Fix https://trello.com/c/31tPK2O3/1680.
With the merge of ether/etherpad-lite#3268, `getLineHTMLForExport` should not return anything, but instead set the HTML content of the line on `context.lineContent`. Fix https://trello.com/c/31tPK2O3/1680.
The key of this fix is fixing the wrong usage of
context.lineContent
andgetLineHTMLForExport
.Previously all
getLineHTMLForExport
hooks were expected to return lines updated by plugins and all of these lines were concatet into one string resulting mutliple lines with same text but different html wrappers.The correct use would be to update
context.lineContent
inside each of these hooks and return true if a line was changed. Because the context is always passed on each plugin will modify the same value with each one adding its own updates rather than modifing the same line from orignal value.This fix will need updating each plugin using
getLineHTMLForExport
to work properly!!!I updated my previous fix cause I found that returning true from multiple plugins made
lineContentFromHook
value into 'true true true'. But I must add that this fix cannot work if there are updated and not updated plugins in use at the same time.