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

Scrolling fixes for inline editors#557

Merged
jasonsanjose merged 13 commits into
masterfrom
nj/inline-hscroll-fixes
Apr 6, 2012
Merged

Scrolling fixes for inline editors#557
jasonsanjose merged 13 commits into
masterfrom
nj/inline-hscroll-fixes

Conversation

@njx

@njx njx commented Apr 5, 2012

Copy link
Copy Markdown

@jason-sanjose

Note: this requires the CodeMirror branch https://github.com/adobe/CodeMirror2/pull/51

This pull request fixes two issues.

  • Makes it so that the scrollable width of the host editor is always large enough to encompass the width of the code in the inline widget plus the width of the rule list, so you can always scroll to see the entire code in the inline widget.

The main bit that achieves this is the addition of padding to the inline widget to account for the space taken up by the rule list. However, in order for that to work, we can no longer just have min-width set to 100% on the inline widget, because the padding is added onto that width, which would then push out the width of the parent, which would then increase what the 100% is referring to. So we have to explicitly calculate the min-width based on the parent's code width.

Note that in some cases we add too much horizontal space to the scroll area, because we always tack on the width of the rule list to the width of the host editor's code. I tried to make it so that we subtract the width of the rule list from the host editor's code width, but for some reason that led to problems with the calculation of the overall scroll width (the scroll width would bounce back and forth as you type). Just using box-sizing: border-box to make the min-width include the padding had a similar issue. I'm guessing there are some bugs with min-width in the browser. At this point I think we should treat the extra scroll area as a low-priority bug.

There are a couple of cases here. Horizontally, the cursor needs to stay visible even if you arrow underneath the rule list. In order to deal with that, we add the width of the rule list to the current horizontal cursor position, and ask the host editor to ensure that's visible. That has the effect of pushing everything over so that the actual cursor position is visible to the left of the rule list.

Vertically, we just need to translate the cursor position into a position in the host editor's scroll region.

Narciso Jaramillo added 5 commits April 4, 2012 11:43
@njx

njx commented Apr 5, 2012

Copy link
Copy Markdown
Author

Also--make sure you get the latest CodeMirror.

@njx

njx commented Apr 5, 2012

Copy link
Copy Markdown
Author

Merged with latest version of the inline-cleanup branch

@njx

njx commented Apr 5, 2012

Copy link
Copy Markdown
Author

@jason-sanjose Hold off on reviewing this for now. I'm working on a fix for #533 that overlaps this code somewhat, so probably better to review it all together.

@njx

njx commented Apr 5, 2012

Copy link
Copy Markdown
Author

@jason-sanjose Okay, now this is really ready to review :) I updated the description with both of the issues it fixes. Note that you need the CodeMirror changes in https://github.com/adobe/CodeMirror2/pull/51 as well.

@jasonsanjose

Copy link
Copy Markdown
Member

Reviewing now. Needs another merge though.

Comment thread src/editor/Editor.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we prefix with "_" to denote internal and discourage use?

Comment thread src/editor/Editor.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style-wise, do we care prefer the above or

$(this.getScrollerElement()).find(".CodeMirror-lines")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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.

…encouraged to use. Made it so we only scroll on cursorActivity in the focused editor.
@njx

njx commented Apr 6, 2012

Copy link
Copy Markdown
Author

Responded to code review. Also fixed a bug I noticed where we were responding to cursor activity in non-focused editors, so each inline editor would try to scroll itself into view whenever any editor was touched.

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Author

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.

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