Skip to content

Fix editor height issue#4954

Closed
webzwo0i wants to merge 4 commits intodevelopfrom
fix-editor-height-issue
Closed

Fix editor height issue#4954
webzwo0i wants to merge 4 commits intodevelopfrom
fix-editor-height-issue

Conversation

@webzwo0i
Copy link
Copy Markdown
Member

@webzwo0i webzwo0i commented Mar 17, 2021

This fixes the problem of the clientHeight in ace_inner iframe being as large as the whole document instead of only the visible viewport. Please don't squash as it also fixes some other things.

@rhansen
I deleted the poll condition as it was the source of loading problems in ~20% with chromium. The debug output was:

["Ace2Editor.init()"]
["Ace2Editor.init() waiting for outer frame"]
(2) ["Ace2Editor.init() load event on", Window]
["Ace2Editor.init() outer frame ready"]
["Ace2Editor.init() waiting for inner frame"]
["Ace2Editor.init() polling"]
["Ace2Editor.init() poll condition became true"]
["Ace2Editor.init() inner frame ready"]
["Ace2Editor.init() waiting for require kernel load"]

In the other 80% "poll condition became true" never happened. Do you know which browsers needed that, as our frontend tests are passing?

@webzwo0i
Copy link
Copy Markdown
Member Author

webzwo0i commented Mar 17, 2021 •
edited
Loading

I had no luck with test coverage. All I can do is check the clientHeight, but I'm unable to find a test for "Is all the pad content visible". (After reverting the auto height commit, clientHeight is correct, but it won't grow the editor, so it would be good to have a regression test for this, too. I seem to be unable to mouse click anywhere inside the iframe)

@webzwo0i webzwo0i force-pushed the fix-editor-height-issue branch from e30c65d to f9568e9 Compare March 17, 2021 06:02
@JohnMcLear
Copy link
Copy Markdown
Member

JohnMcLear commented Mar 17, 2021

Looks great work man

@webzwo0i webzwo0i force-pushed the fix-editor-height-issue branch from f9568e9 to 9f61197 Compare March 17, 2021 15:43
webzwo0i
webzwo0i commented Mar 17, 2021
View reviewed changes
Comment thread src/static/js/ace.js
script.src = absUrl(`../javascripts/lib/ep_etherpad-lite/static/js/${module}.js` +
`?callback=require.define&v=${clientVars.randomVersionString}`);
innerDocument.head.appendChild(script);
}
Copy link
Copy Markdown
Member Author

@webzwo0i webzwo0i Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it here is not good, as we now fetch those resources automatically (without version string). So we need to find a proper fix for the "require not found" caused by those lines.

@webzwo0i webzwo0i marked this pull request as draft March 17, 2021 15:48
@webzwo0i
Copy link
Copy Markdown
Member Author

webzwo0i commented Mar 17, 2021

Making it draft, because deleting ace2_inner/ace2_common results in those packages get downloaded automatically via require-kernel and thus they don't have a version string anymore, which is bad for caching. So we need another fix, maybe loading them after require has been defined.

@rhansen
Copy link
Copy Markdown
Member

rhansen commented Mar 18, 2021 •
edited
Loading

In the other 80% "poll condition became true" never happened. Do you know which browsers needed that, as our frontend tests are passing?

At least Safari, but I think Chrome too. However, with your iframe.srcdoc = '<!DOCTYPE html>'; change it looks like the load event now fires on those browsers so polling is unnecessary.

@webzwo0i webzwo0i mentioned this pull request Mar 19, 2021
Closed
@webzwo0i
Copy link
Copy Markdown
Member Author

webzwo0i commented Mar 19, 2021 •
edited
Loading

this depends on ether/etherpad-require-kernel#3 now.
If it is merged we need to update package.json and then this can be merged without squashing.

The one "bad" thing is still, that I can't find a good way to regression test:
when the "not growing editor" problem reappears we don't have a test for it, mainly because I cannot get "mouse clicks" to work. My idea was, that when there are a lot of lines and we scroll down, we would not be able to click anywhere, so the caret will always stay in it's old position. Directly using selenium (so the test won't work in local browser env) is possible.

@webzwo0i webzwo0i force-pushed the fix-editor-height-issue branch from 9f61197 to 61c899b Compare March 19, 2021 17:42
@webzwo0i webzwo0i marked this pull request as ready for review March 19, 2021 18:49
@JohnMcLear
Copy link
Copy Markdown
Member

JohnMcLear commented Mar 19, 2021

published require kernel update

webzwo0i added 2 commits March 19, 2021 23:19
@webzwo0i
Don't prefetch ace2_inner/ace2_common. It has no real performance gain
cd9342c 
and can result in benign console errors ('require not found')

bump require-kernel
@webzwo0i
add test for wrong clientHeight
1eaa293
@webzwo0i webzwo0i force-pushed the fix-editor-height-issue branch from 4a041f4 to 1eaa293 Compare March 19, 2021 22:19
@rhansen
Copy link
Copy Markdown
Member

rhansen commented Mar 20, 2021

In the revert commit, would you update the commit message to explain why it needs to be reverted?

@webzwo0i
Copy link
Copy Markdown
Member Author

webzwo0i commented Mar 20, 2021

I wanted to add the message now, but then I realized the srcdoc patch was enough for scrolling and line number links to work. It works with and without 470f40d, so I gonna keep it as I can't see any disadvantage.

I'm closing this and do a PR with the other commits in a separate branch.

@webzwo0i webzwo0i closed this Mar 20, 2021
@webzwo0i webzwo0i mentioned this pull request Mar 20, 2021
Merged
@rhansen rhansen deleted the fix-editor-height-issue branch March 20, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@webzwo0i @JohnMcLear @rhansen