From c082256a1e627a6a9d8f0d77b8d0207ee9266cb0 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Wed, 4 Apr 2012 11:43:26 -0700 Subject: [PATCH 1/6] Add padding to right edge of inline widget to account for rule list --- src/editor/CSSInlineEditor.js | 3 +++ src/editor/Editor.js | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index 7ba387429cc..03d89f78dd7 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -216,6 +216,9 @@ define(function (require, exports, module) { } else { this.$relatedContainer.css("clip", ""); } + + // Add extra padding to the right edge of the widget to account for the rule list. + this.$htmlContent.css("padding-right", this.$relatedContainer.outerWidth() + "px"); }; /** diff --git a/src/editor/Editor.js b/src/editor/Editor.js index 68bdeaa57e7..474d666710c 100644 --- a/src/editor/Editor.js +++ b/src/editor/Editor.js @@ -742,14 +742,13 @@ 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) { this._codeMirror.setInlineWidgetHeight(inlineWidget.id, height, ensureVisible); }; - /** Gives focus to the editor control */ Editor.prototype.focus = function () { this._codeMirror.focus(); From 0d08fbbd9caa3c82c18c727f283d4e166e7b65c5 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Wed, 4 Apr 2012 16:52:57 -0700 Subject: [PATCH 2/6] Explicitly set the minimum width of the inline widget based on the actual width of the code in the host editor, so that the padding added by the rule list will push out past that actual width. Relies on the fix in the nj/maxline-fix branch in the CodeMirror2 submodule. --- src/editor/CSSInlineEditor.js | 9 +++++++++ src/styles/brackets.less | 1 - src/thirdparty/CodeMirror2 | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index 7e8744fd263..57211e953c7 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -219,6 +219,15 @@ define(function (require, exports, module) { // Add extra padding to the right edge of the widget to account for the rule list. this.$htmlContent.css("padding-right", this.$relatedContainer.outerWidth() + "px"); + + // Set the minimum width of the widget (which doesn't include the padding) to the width + // of CodeMirror's linespace, so that the total width will be at least as large as the + // width of the host editor's code plus the padding for the rule list. This is a bit of a + // hack since it relies on knowing some detail about the innards of CodeMirror. + var lineSpaceParent = $(".CodeMirror-lines", this.hostEditor._codeMirror.getScrollerElement()).get(0), + lineSpace = $(lineSpaceParent).children().get(0), + minWidth = $(lineSpace).offset().left - this.$htmlContent.offset().left + $(lineSpace).width(); + this.$htmlContent.css("min-width", minWidth + "px"); }; /** diff --git a/src/styles/brackets.less b/src/styles/brackets.less index 7a9e1a688eb..65bc975bb41 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -321,7 +321,6 @@ a, img { .InlineWidget { border-top: 1px solid #C0C0C0; border-bottom: 1px solid #C0C0C0; - min-width: 100%; background-color: #eaeaea; .filename { diff --git a/src/thirdparty/CodeMirror2 b/src/thirdparty/CodeMirror2 index 2ff65afa99a..556cd26c9d4 160000 --- a/src/thirdparty/CodeMirror2 +++ b/src/thirdparty/CodeMirror2 @@ -1 +1 @@ -Subproject commit 2ff65afa99a09296e0c03578c402da664f5a83b7 +Subproject commit 556cd26c9d4c8eb3ae02ec1f52643204475693ec From bb99b5051574571a7fac3f913463b645008816f7 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Wed, 4 Apr 2012 18:43:12 -0700 Subject: [PATCH 3/6] tentative fix for issue 533, not ready yet --- src/editor/CSSInlineEditor.js | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index 57211e953c7..7ce970a034b 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -56,6 +56,7 @@ define(function (require, exports, module) { // Bind event handlers this._updateRelatedContainer = this._updateRelatedContainer.bind(this); + this._checkHorizontalScroll = this._checkHorizontalScroll.bind(this); // Create DOM to hold editors and related list this.$editorsDiv = $(document.createElement('div')).addClass("inlineEditorHolder"); @@ -149,6 +150,9 @@ define(function (require, exports, module) { // but in this case we're specifically concerned with changes in the given // editor, not general document changes. $(this.editors[0]).on("change", this._updateRelatedContainer); + + // Cursor activity in the inline editor may cause us to horizontally scroll. + $(this.editors[0]).on("cursorActivity", this._checkHorizontalScroll); this.sizeInlineWidgetToContents(true); this._updateRelatedContainer(); @@ -190,6 +194,7 @@ define(function (require, exports, module) { // remove resize handlers for relatedContainer $(this.hostEditor).off("change", this._updateRelatedContainer); $(this.editors[0]).off("change", this._updateRelatedContainer); + $(this.editors[0]).off("cursorActivity", this._checkHorizontalScroll); $(this.hostEditor).off("scroll", this._updateRelatedContainer); }; @@ -204,7 +209,7 @@ define(function (require, exports, module) { // Because we're using position: fixed, we need to explicitly clip the rule list if it crosses // out of the top or bottom of the scroller area. - var hostScroller = this.hostEditor._codeMirror.getScrollerElement(), + var hostScroller = this.hostEditor.getScrollerElement(), rcTop = this.$relatedContainer.offset().top, rcHeight = this.$relatedContainer.outerHeight(), rcBottom = rcTop + rcHeight, @@ -222,13 +227,29 @@ define(function (require, exports, module) { // Set the minimum width of the widget (which doesn't include the padding) to the width // of CodeMirror's linespace, so that the total width will be at least as large as the - // width of the host editor's code plus the padding for the rule list. This is a bit of a - // hack since it relies on knowing some detail about the innards of CodeMirror. - var lineSpaceParent = $(".CodeMirror-lines", this.hostEditor._codeMirror.getScrollerElement()).get(0), + // width of the host editor's code plus the padding for the rule list. We need to do this + // rather than just setting min-width to 100% because adding padding for the rule list + // actually pushes out the width of the container, so we would end up continuously + // growing the overall width. + // This is a bit of a hack since it relies on knowing some detail about the innards of CodeMirror. + var lineSpaceParent = $(".CodeMirror-lines", this.hostEditor.getScrollerElement()).get(0), lineSpace = $(lineSpaceParent).children().get(0), minWidth = $(lineSpace).offset().left - this.$htmlContent.offset().left + $(lineSpace).width(); this.$htmlContent.css("min-width", minWidth + "px"); }; + + /** + * Based on the position of the cursor in the inline editor, determine whether we need to change the + * scroll position of the host editor to ensure that the cursor is visible. + */ + CSSInlineEditor.prototype._checkHorizontalScroll = function () { + var cursorCoords = this.editors[0]._codeMirror.cursorCoords(), + widgetCoords = $(this.htmlContent).offset(); + this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - widgetCoords.left, + cursorCoords.y - widgetCoords.top, + cursorCoords.x - widgetCoords.left, + cursorCoords.yBot - widgetCoords.top); + }; /** * From a832e09f28d1b7b14fe9726bd9435c9221c74554 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Thu, 5 Apr 2012 15:27:08 -0700 Subject: [PATCH 4/6] Keep cursor visible as you arrow around horizontally in an inline editor (doesn't handle vertical scrolling yet) --- src/editor/CSSInlineEditor.js | 24 +++++++++++++++++------- src/editor/Editor.js | 19 +++++++++++++++---- src/thirdparty/CodeMirror2 | 2 +- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index fc4b43cfade..529083195aa 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -254,8 +254,7 @@ define(function (require, exports, module) { // actually pushes out the width of the container, so we would end up continuously // growing the overall width. // This is a bit of a hack since it relies on knowing some detail about the innards of CodeMirror. - var lineSpaceParent = $(".CodeMirror-lines", this.hostEditor.getScrollerElement()).get(0), - lineSpace = $(lineSpaceParent).children().get(0), + var lineSpace = this.hostEditor.getLineSpaceElement(), minWidth = $(lineSpace).offset().left - this.$htmlContent.offset().left + $(lineSpace).width(); this.$htmlContent.css("min-width", minWidth + "px"); }; @@ -266,11 +265,22 @@ define(function (require, exports, module) { */ CSSInlineEditor.prototype._checkHorizontalScroll = function () { var cursorCoords = this.editors[0]._codeMirror.cursorCoords(), - widgetCoords = $(this.htmlContent).offset(); - this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - widgetCoords.left, - cursorCoords.y - widgetCoords.top, - cursorCoords.x - widgetCoords.left, - cursorCoords.yBot - widgetCoords.top); + lineSpaceOffset = $(this.editors[0].getLineSpaceElement()).offset(), + hostLineSpaceOffset = $(this.hostEditor.getLineSpaceElement()).offset(), + ruleListOffset = this.$relatedContainer.offset(); + // If we're off the left-hand side, we just want to scroll it into view normally. But + // if we're underneath the rule list on the right, we want to ask the host editor to + // scroll far enough that the current cursor position is visible to the left of the rule + // list. (Because we always add extra padding for the rule list, this is always possible.) + if (cursorCoords.x > ruleListOffset.left) { + cursorCoords.x += this.$relatedContainer.outerWidth(); + } + // Vertically, we want to set the scroll position relative to the overall host editor, not + // the lineSpace of the widget itself. + this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - lineSpaceOffset.left, + cursorCoords.y - hostLineSpaceOffset.top, + cursorCoords.x - lineSpaceOffset.left, + cursorCoords.yBot - hostLineSpaceOffset.top); }; /** diff --git a/src/editor/Editor.js b/src/editor/Editor.js index d3b5f8fc474..6c2422be5ce 100644 --- a/src/editor/Editor.js +++ b/src/editor/Editor.js @@ -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(); @@ -707,6 +707,17 @@ 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); + return $(lineSpaceParent).children().get(0); + }; + /** * Adds an inline widget below the given line. If any inline widget was already open for that * line, it is closed without warning. @@ -773,7 +784,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"); }; /** @@ -789,7 +800,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) { @@ -803,7 +814,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"); }; /** diff --git a/src/thirdparty/CodeMirror2 b/src/thirdparty/CodeMirror2 index 6e8e7e3d468..b4e347087cc 160000 --- a/src/thirdparty/CodeMirror2 +++ b/src/thirdparty/CodeMirror2 @@ -1 +1 @@ -Subproject commit 6e8e7e3d4680a9223ecfa678f38b25c2b2d2a4b8 +Subproject commit b4e347087cc20eedeea7808593a3759b9069a6bb From 1d612190d5f574ffe95828f5c8396de8fead0286 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Thu, 5 Apr 2012 16:18:30 -0700 Subject: [PATCH 5/6] Ensure that cursor stays visible when moving vertically within inline editor --- src/editor/CSSInlineEditor.js | 20 ++++++++++++-------- src/editor/Editor.js | 8 ++++++++ src/thirdparty/CodeMirror2 | 2 +- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index c32e3079918..46e8f2f6390 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -55,7 +55,7 @@ define(function (require, exports, module) { // Bind event handlers this._updateRelatedContainer = this._updateRelatedContainer.bind(this); - this._checkHorizontalScroll = this._checkHorizontalScroll.bind(this); + this._ensureCursorVisible = this._ensureCursorVisible.bind(this); this._onClick = this._onClick.bind(this); // Create DOM to hold editors and related list @@ -159,7 +159,7 @@ define(function (require, exports, module) { $(this.editors[0]).on("change", this._updateRelatedContainer); // Cursor activity in the inline editor may cause us to horizontally scroll. - $(this.editors[0]).on("cursorActivity", this._checkHorizontalScroll); + $(this.editors[0]).on("cursorActivity", this._ensureCursorVisible); this.sizeInlineWidgetToContents(true); this._updateRelatedContainer(); @@ -201,7 +201,7 @@ define(function (require, exports, module) { // remove resize handlers for relatedContainer $(this.hostEditor).off("change", this._updateRelatedContainer); $(this.editors[0]).off("change", this._updateRelatedContainer); - $(this.editors[0]).off("cursorActivity", this._checkHorizontalScroll); + $(this.editors[0]).off("cursorActivity", this._ensureCursorVisible); $(this.hostEditor).off("scroll", this._updateRelatedContainer); $(window).off("resize", this._updateRelatedContainer); }; @@ -294,10 +294,9 @@ define(function (require, exports, module) { * Based on the position of the cursor in the inline editor, determine whether we need to change the * scroll position of the host editor to ensure that the cursor is visible. */ - CSSInlineEditor.prototype._checkHorizontalScroll = function () { + CSSInlineEditor.prototype._ensureCursorVisible = function () { var cursorCoords = this.editors[0]._codeMirror.cursorCoords(), lineSpaceOffset = $(this.editors[0].getLineSpaceElement()).offset(), - hostLineSpaceOffset = $(this.hostEditor.getLineSpaceElement()).offset(), ruleListOffset = this.$relatedContainer.offset(); // If we're off the left-hand side, we just want to scroll it into view normally. But // if we're underneath the rule list on the right, we want to ask the host editor to @@ -306,12 +305,17 @@ define(function (require, exports, module) { if (cursorCoords.x > ruleListOffset.left) { cursorCoords.x += this.$relatedContainer.outerWidth(); } + // Vertically, we want to set the scroll position relative to the overall host editor, not - // the lineSpace of the widget itself. + // the lineSpace of the widget itself. Also, we can't use the lineSpace here, because its top + // position just corresponds to whatever CodeMirror happens to have rendered at the top. So + // we need to figure out our position relative to the top of the virtual scroll area, which is + // the top of the actual scroller minus the scroll position. + var scrollerTop = $(this.hostEditor.getScrollerElement()).offset().top - this.hostEditor.getScrollPos().y; this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - lineSpaceOffset.left, - cursorCoords.y - hostLineSpaceOffset.top, + cursorCoords.y - scrollerTop, cursorCoords.x - lineSpaceOffset.left, - cursorCoords.yBot - hostLineSpaceOffset.top); + cursorCoords.yBot - scrollerTop); }; /** diff --git a/src/editor/Editor.js b/src/editor/Editor.js index 6c2422be5ce..12037a33d66 100644 --- a/src/editor/Editor.js +++ b/src/editor/Editor.js @@ -717,6 +717,14 @@ define(function (require, exports, module) { var lineSpaceParent = $(".CodeMirror-lines", this.getScrollerElement()).get(0); 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 diff --git a/src/thirdparty/CodeMirror2 b/src/thirdparty/CodeMirror2 index b4e347087cc..abbea169b5b 160000 --- a/src/thirdparty/CodeMirror2 +++ b/src/thirdparty/CodeMirror2 @@ -1 +1 @@ -Subproject commit b4e347087cc20eedeea7808593a3759b9069a6bb +Subproject commit abbea169b5bd306e1a6748cda09ea4083eb24c0a From 9ed6abc458f8af536848575bff3739cf54c3aae0 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Thu, 5 Apr 2012 21:17:10 -0700 Subject: [PATCH 6/6] Add _ to getLineSpaceElement() to make it clear this isn't generally encouraged to use. Made it so we only scroll on cursorActivity in the focused editor. --- src/editor/CSSInlineEditor.js | 44 ++++++++++++++++++----------------- src/editor/Editor.js | 2 +- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/editor/CSSInlineEditor.js b/src/editor/CSSInlineEditor.js index 4179600c683..1a73d509bac 100644 --- a/src/editor/CSSInlineEditor.js +++ b/src/editor/CSSInlineEditor.js @@ -285,7 +285,7 @@ define(function (require, exports, module) { // actually pushes out the width of the container, so we would end up continuously // growing the overall width. // This is a bit of a hack since it relies on knowing some detail about the innards of CodeMirror. - var lineSpace = this.hostEditor.getLineSpaceElement(), + var lineSpace = this.hostEditor._getLineSpaceElement(), minWidth = $(lineSpace).offset().left - this.$htmlContent.offset().left + $(lineSpace).width(); this.$htmlContent.css("min-width", minWidth + "px"); }; @@ -295,27 +295,29 @@ define(function (require, exports, module) { * scroll position of the host editor to ensure that the cursor is visible. */ CSSInlineEditor.prototype._ensureCursorVisible = function () { - var cursorCoords = this.editors[0]._codeMirror.cursorCoords(), - lineSpaceOffset = $(this.editors[0].getLineSpaceElement()).offset(), - ruleListOffset = this.$relatedContainer.offset(); - // If we're off the left-hand side, we just want to scroll it into view normally. But - // if we're underneath the rule list on the right, we want to ask the host editor to - // scroll far enough that the current cursor position is visible to the left of the rule - // list. (Because we always add extra padding for the rule list, this is always possible.) - if (cursorCoords.x > ruleListOffset.left) { - cursorCoords.x += this.$relatedContainer.outerWidth(); + if ($.contains(this.editors[0].getRootElement(), document.activeElement)) { + var cursorCoords = this.editors[0]._codeMirror.cursorCoords(), + lineSpaceOffset = $(this.editors[0]._getLineSpaceElement()).offset(), + ruleListOffset = this.$relatedContainer.offset(); + // If we're off the left-hand side, we just want to scroll it into view normally. But + // if we're underneath the rule list on the right, we want to ask the host editor to + // scroll far enough that the current cursor position is visible to the left of the rule + // list. (Because we always add extra padding for the rule list, this is always possible.) + if (cursorCoords.x > ruleListOffset.left) { + cursorCoords.x += this.$relatedContainer.outerWidth(); + } + + // Vertically, we want to set the scroll position relative to the overall host editor, not + // the lineSpace of the widget itself. Also, we can't use the lineSpace here, because its top + // position just corresponds to whatever CodeMirror happens to have rendered at the top. So + // we need to figure out our position relative to the top of the virtual scroll area, which is + // the top of the actual scroller minus the scroll position. + var scrollerTop = $(this.hostEditor.getScrollerElement()).offset().top - this.hostEditor.getScrollPos().y; + this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - lineSpaceOffset.left, + cursorCoords.y - scrollerTop, + cursorCoords.x - lineSpaceOffset.left, + cursorCoords.yBot - scrollerTop); } - - // Vertically, we want to set the scroll position relative to the overall host editor, not - // the lineSpace of the widget itself. Also, we can't use the lineSpace here, because its top - // position just corresponds to whatever CodeMirror happens to have rendered at the top. So - // we need to figure out our position relative to the top of the virtual scroll area, which is - // the top of the actual scroller minus the scroll position. - var scrollerTop = $(this.hostEditor.getScrollerElement()).offset().top - this.hostEditor.getScrollPos().y; - this.hostEditor._codeMirror.scrollIntoView(cursorCoords.x - lineSpaceOffset.left, - cursorCoords.y - scrollerTop, - cursorCoords.x - lineSpaceOffset.left, - cursorCoords.yBot - scrollerTop); }; /** diff --git a/src/editor/Editor.js b/src/editor/Editor.js index fd20aeaf531..dfab768738d 100644 --- a/src/editor/Editor.js +++ b/src/editor/Editor.js @@ -722,7 +722,7 @@ define(function (require, exports, module) { * editors. * @returns {Object} The editor's lineSpace element. */ - Editor.prototype.getLineSpaceElement = function () { + Editor.prototype._getLineSpaceElement = function () { var lineSpaceParent = $(".CodeMirror-lines", this.getScrollerElement()).get(0); return $(lineSpaceParent).children().get(0); };