Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Remove the use of the viewport hack #7118

Merged
merged 1 commit into from
Mar 7, 2014
Merged

Conversation

TomMalbran
Copy link
Contributor

Since #3115 is now fixed, I removed the use of the viewport hack used to properly calculate the height of the editor. This also fixes scroll adjustment when changing the font, which was using the viewport hack and stopped working when #3115 was fixed.

@RaymondLim
Copy link
Contributor

Reviewing...

This viewport hack is the real culprit of #7093. I verified that removing this hack fix the #7093.


} else if (coords.top + inlineEditor.info.height < scrollInfo.top + editorHeight) {
editorHeight -= inlineEditor.info.height;
linesInView = _getLinesInView(textHeight, scrollTop, editorHeight);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it doesn't use linesInView inside the forEach, I could just calculate the new scrollTop and editorHeight and call _getLinesInView(textHeight, scrollTop, editorHeight); just once after this for each.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explanation. I did try to understand the logic here and did verify and test scrolling with several inline editors and watched the cursor moving to next line when the previous line is moving out of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, wasn't trying to explain it, but mentioning that the calculations here could be simplified. The idea of this for each is to remove the height of the inline editors from the scroll position and the editor height. After that is done I can just divide the left over scroll and height by the text height and get the correct first and last line in view.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misunderstand you. Please create another pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #7120

@RaymondLim
Copy link
Contributor

Looks good and all the integration tests for ViewCommandHandlers pass. Merging.

RaymondLim added a commit that referenced this pull request Mar 7, 2014
Remove the use of the viewport hack
@RaymondLim RaymondLim merged commit 34ff3d8 into master Mar 7, 2014
@RaymondLim RaymondLim deleted the tom/remove-viewport-hack branch March 7, 2014 06:42
@TomMalbran
Copy link
Contributor Author

Oh, I was going to submit a fix for the issue I mentioned... new PR I guess.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants