This repository was archived by the owner on Sep 6, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Scrolling fixes for inline editors #557
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
c082256
Add padding to right edge of inline widget to account for rule list
5b03a0a
Merge branch 'nj/inline-cleanup' into nj/inline-hscroll-fixes
59634f7
Merge branch 'nj/inline-cleanup' into nj/inline-hscroll-fixes
0d08fbb
Explicitly set the minimum width of the inline widget based on the ac…
bb99b50
tentative fix for issue 533, not ready yet
dad2700
Merge from master
11617e0
Merge from master
14e8e15
Merge branch 'nj/inline-hscroll-fixes' into nj/issue-533
a832e09
Keep cursor visible as you arrow around horizontally in an inline edi…
3983386
Merge from master
1d61219
Ensure that cursor stays visible when moving vertically within inline…
89651d2
Merge from master
9ed6abc
Add _ to getLineSpaceElement() to make it clear this isn't generally …
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -333,7 +333,7 @@ define(function (require, exports, module) { | |
| Editor.prototype.destroy = function () { | ||
| // CodeMirror docs for getWrapperElement() say all you have to do is "Remove this from your | ||
| // tree to delete an editor instance." | ||
| $(this._codeMirror.getWrapperElement()).remove(); | ||
| $(this.getRootElement()).remove(); | ||
|
|
||
| // Disconnect from Document | ||
| this.document.releaseRef(); | ||
|
|
@@ -716,6 +716,25 @@ define(function (require, exports, module) { | |
| return this._codeMirror.getWrapperElement(); | ||
| }; | ||
|
|
||
| /** | ||
| * Gets the lineSpace element within the editor (the container around the individual lines of code). | ||
| * FUTURE: This is fairly CodeMirror-specific. Logic that depends on this may break if we switch | ||
| * editors. | ||
| * @returns {Object} The editor's lineSpace element. | ||
| */ | ||
| Editor.prototype._getLineSpaceElement = function () { | ||
| var lineSpaceParent = $(".CodeMirror-lines", this.getScrollerElement()).get(0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style-wise, do we care prefer the above or
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the former is maybe slightly more efficient since it doesn't have to create a separate jQuery object for the scroller element, but I'm sure it doesn't make a difference in practice. |
||
| return $(lineSpaceParent).children().get(0); | ||
| }; | ||
|
|
||
| /** | ||
| * Returns the current scroll position of the editor. | ||
| * @returns {{x:number, y:number}} The x,y scroll position. | ||
| */ | ||
| Editor.prototype.getScrollPos = function () { | ||
| return this._codeMirror.scrollPos(); | ||
| }; | ||
|
|
||
| /** | ||
| * Adds an inline widget below the given line. If any inline widget was already open for that | ||
| * line, it is closed without warning. | ||
|
|
@@ -775,7 +794,7 @@ define(function (require, exports, module) { | |
| /** | ||
| * Sets the height of an inline widget in this editor. | ||
| * @param {!InlineWidget} inlineWidget The widget whose height should be set. | ||
| * @param {!height} height The height of the widget. | ||
| * @param {!number} height The height of the widget. | ||
| * @param {boolean} ensureVisible Whether to scroll the entire widget into view. | ||
| */ | ||
| Editor.prototype.setInlineWidgetHeight = function (inlineWidget, height, ensureVisible) { | ||
|
|
@@ -829,7 +848,7 @@ define(function (require, exports, module) { | |
| /** Returns true if the editor has focus */ | ||
| Editor.prototype.hasFocus = function () { | ||
| // The CodeMirror instance wrapper has a "CodeMirror-focused" class set when focused | ||
| return $(this._codeMirror.getWrapperElement()).hasClass("CodeMirror-focused"); | ||
| return $(this.getRootElement()).hasClass("CodeMirror-focused"); | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -845,7 +864,7 @@ define(function (require, exports, module) { | |
| * @param {boolean} show true to show the editor, false to hide it | ||
| */ | ||
| Editor.prototype.setVisible = function (show) { | ||
| $(this._codeMirror.getWrapperElement()).css("display", (show ? "" : "none")); | ||
| $(this.getRootElement()).css("display", (show ? "" : "none")); | ||
| this._codeMirror.refresh(); | ||
| if (show) { | ||
| this._inlineWidgets.forEach(function (inlineWidget) { | ||
|
|
@@ -859,7 +878,7 @@ define(function (require, exports, module) { | |
| * visible, and has a non-zero width/height. | ||
| */ | ||
| Editor.prototype.isFullyVisible = function () { | ||
| return $(this._codeMirror.getWrapperElement()).is(":visible"); | ||
| return $(this.getRootElement()).is(":visible"); | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
This file contains hidden or 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something odd is going on when the inline widget is scrolled out of the viewport and you have an hscroll in the host editor. The scrollWidth gets shorter when the widget is scrolled out of view, and larger when it comes back into vie
If I comment out line 279, scrolling and cursor visibility seem to work ok. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I forgot to mention that limitation as well. The issue is that if you scroll the widget far enough out of the view, it gets hidden by CodeMirror, so it no longer pushes out the width. There isn't really a good way to fix that without keeping all the inline widgets around--not a major problem, since you probably won't have too many open at once, but that would require significantly changing the inline widget implementation in CM. I'd be inclined to file that as a bug that we revisit when we extract the inline widget implementation for submission to the main CM master.
If you comment out 279, this is the case that will break: https://gist.github.com/3b1a77200e08cb326f15 -- put your cursor on
<div>and open the editor, then try to scroll all the way to the right. You won't be able to get to the end of the background property.