Auto-height fix for text area which is contained in a scroll container #1933

felixhayashi · 2015-09-14T09:31:25Z

Finally found the time to implement this (see #1575). Your suggested way for fixing was the way to go.

Description

Allows text areas to be auto-height while being wrapped in an
element that has scrollbars. The wrapper element with the
scroll bar does not need to be a direct parent of the text area.

Demo

For a demo of the new behaviour see:
http://wkpr.de/hosting/tmp/tw5/scrolling/new.html

The old (bad) behaviour can be viewed here:
http://wkpr.de/hosting/tmp/tw5/scrolling/old.html

-Felix

pmario · 2015-09-14T11:03:46Z

I'm not sure, but I don't see a difference in behaviour between old and new. using win10, ff 40.0.3.

If I use the scroll wheel, both versions scroll. ... ~~there is a little difference: The new versions scrolls a little bit later.~~

pmario · 2015-09-14T11:08:36Z

probably, I use your test cases in a wrong way. How to reproduce?

felixhayashi · 2015-09-14T12:56:36Z

Hi @pmario

I'm not sure, but I don't see a difference in behaviour between old and new. using win10, ff 40.0.3.

The behaviour for tiddlers without scroll containers is for both the same (I am referring to writing text in "A dummy Tiddler").

But old and new differ for text areas with scrollable parents. To see a difference,

go to "Inside a scrollable parent".
Click into the text area and scroll down till you see "junk TV quiz." (By the way, I just noticed the blind text I copied from a generator is rather stupid, sorry).
Type a letter with your keyboard
1. For the old wiki, it will jump/scroll to the top (like I demonstrated in this video a few weeks ago: https://www.youtube.com/watch?v=Fw-_zlJzuDg&feature=youtu.be).
2. For the new wiki, it will behave just like when you are inside in a tiddler edit mode.

-Felix

pmario · 2015-09-14T14:45:57Z

ahh, I see. Sorry for the trouble. so

+1 for the fix. tested with ff latest and MS Edge on win 10 :)

felixhayashi · 2015-09-14T15:00:55Z

ahh, I see. Sorry for the trouble. so
+1 for the fix. tested with ff latest and MS Edge on win 10 :)

No problem, glad you tested it and it works! :)

-Felix

felixhayashi · 2015-09-20T09:46:48Z

Hi @Jermolene

Could you please signal me, whether you are satisfied with the PR or not? It is important for TiddlyMap, otherwise working in fullscreen is not really possible, so I hope it is included in the next TiddlyWiki release.

Thanks
-Felix

tobibeer · 2015-09-20T10:45:34Z

@felixhayashi, when I test "A dummy tiddler" in the new version, the behavior seems to have actually deteriorated from working fine in the old one to jumpy in the new one. Hit enter at the bottom to add some new lines. Can you confirm?

felixhayashi · 2015-09-20T10:52:47Z

Can you confirm?

Whoops. Yes. Not sure what is causing this, I'll need to fix that.

felixhayashi · 2015-09-20T10:55:12Z

It only happens for the very last tiddler in the river. If you open/create other tiddlers in the river and do as you described above, it works fine. Strange…

felixhayashi · 2015-09-20T11:51:29Z

Ok found the problem and will upload a new version in a minute.

element that has scrollbars. The wrapper element with the scroll bar does not need to be a direct parent of the text area. **update:** fixed a bug This is fixed now: TiddlyWiki#1933 (comment) The problem was the check in getScrollContainer()

felixhayashi · 2015-09-20T12:31:22Z

core/modules/widgets/edit-text.js

+		var container = this.getScrollContainer(domNode),
+			scrollTop = container.scrollTop;
+                // Measure the specified minimum height
+		domNode.style.height = this.editMinHeight;


@Jermolene that is an interesting way to allow the user to use calc() and percent units… I mean you assign the minHeight to the domNode just to measure it and the override it again with "auto", that seems strange at first sight – but considering, that the user may enter values that are not pixels, it is very clever to calculate the height that way.

The purpose of the manoeuvre is actually to measure the 'natural' size of the content of the textarea so that we can then explicitly set the textarea itself to that size. If the user is manually sizing the textarea then none of this is needed.

Ah, then I misinterpreted this line. Thanks for the clarification.

felixhayashi · 2015-09-20T12:35:59Z

@tobibeer thanks for telling me. I updated the patch. Should work now :)

See the demo.

Jermolene · 2015-09-20T12:39:47Z

Sorry @felixhayashi I'm only just having a look at this now. In principle I'm very keen on a fix, but I haven't yet investigated the approach you've taken or explored any unintended impact.

felixhayashi · 2015-09-20T12:44:00Z

Sorry @felixhayashi I'm only just having a look at this now. In principle I'm very keen on a fix, but I haven't yet investigated the approach you've taken or explored any unintended impact.

Ah ok, thanks for telling me. Then I know you are aware of it. :)

tobibeer · 2015-09-20T12:59:29Z

@felixhayashi, works like a charm now, thanks.

Jermolene · 2015-09-23T21:23:30Z

@felixhayashi I haven't forgotten this PR! I will make some time to test it properly over the next couple of days

felixhayashi · 2015-09-23T21:43:35Z

Thanks @Jermolene. I appreciate very much that you are informing me. Please take your time, I agree this needs some thorough testing as it is an important widget :)

element that has scrollbars. The wrapper element with the scroll bar does not need to be a direct parent of the text area. **update:** fixed a bug that came up in the discussion This is fixed now: TiddlyWiki#1933 (comment) The problem was the check in getScrollContainer()

felixhayashi · 2015-11-05T02:38:51Z

Just as a sidenote: I have tested this PR in all my wikis (Chrome + FF) the past weeks and it works without any problems.

Jermolene · 2015-11-05T12:43:48Z

core/modules/widgets/edit-text.js

 			// Make sure that the dimensions of the textarea are recalculated
 			$tw.utils.forceLayout(domNode);
-			// Check that the scroll position is still visible before trying to scroll back to it
-			scrollTop = Math.min(scrollTop,self.document.body.scrollHeight - window.innerHeight);


Hi @felixhayashi my apologies. I'm going to focus on the backlog of pull requests for a few days, and wanted to start with this one!

Anyhow, your changes appear to have lost the logic here that checks that the scroll position is still visible. Is that intentional?

Hi @Jermolene

This is intentional because I use scrollTop and not window.scrollTo()

scrollTop can be set to any integer value, with certain caveats:

If the element can't be scrolled (e.g. it has no overflow or if the element is non-scrollable), scrollTop is set to 0.
If set to a value less than 0, scrollTop is set to 0.
If set to a value greater than the maximum that the content can be scrolled, scrollTop is set to the maximum.

https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTop

So I do not need to do this check.

Moreover, the new code that accounts for situation with nested elements that act as scroll containers, the parent may not be the window object (as in the original/old code; also reflected in $tw.utils.getScrollPosition only looking at the window object) and the scrollHeight of relevance may not be the one of the body element.

But feel free to thoroughly test it :)

Auto-height fix for text area which is contained in a scroll container

felixhayashi · 2015-12-22T14:21:25Z

Yay! 😃 thanks @Jermolene

Jermolene · 2015-12-22T16:35:47Z

No problem @felixhayashi

I've just updated the prerelease too: http://tiddlywiki.com/prerelease

felixhayashi mentioned this pull request Sep 14, 2015

Scrolling issue when editing tiddlers in fullscreen felixhayashi/TW5-TiddlyMap#158

Closed

felixhayashi force-pushed the fix/scrolling branch from dee8c2d to f33b33f Compare September 20, 2015 12:17

felixhayashi force-pushed the fix/scrolling branch from f33b33f to ab92ea4 Compare September 20, 2015 12:22

felixhayashi reviewed Sep 20, 2015
View reviewed changes

felixhayashi force-pushed the fix/scrolling branch from ab92ea4 to d3ab414 Compare October 26, 2015 20:23

Jermolene reviewed Nov 5, 2015
View reviewed changes

felixhayashi mentioned this pull request Dec 22, 2015

Release 5.1.10 before Christmas #2143

Closed

Jermolene added a commit that referenced this pull request Dec 22, 2015

Merge pull request #1933 from felixhayashi/fix/scrolling

ccf6edb

Auto-height fix for text area which is contained in a scroll container

Jermolene merged commit ccf6edb into TiddlyWiki:master Dec 22, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-height fix for text area which is contained in a scroll container #1933

Auto-height fix for text area which is contained in a scroll container #1933

felixhayashi commented Sep 14, 2015

pmario commented Sep 14, 2015

pmario commented Sep 14, 2015

felixhayashi commented Sep 14, 2015

pmario commented Sep 14, 2015

felixhayashi commented Sep 14, 2015

felixhayashi commented Sep 20, 2015

tobibeer commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi Sep 20, 2015

Jermolene Sep 20, 2015

felixhayashi Sep 20, 2015

felixhayashi commented Sep 20, 2015

Jermolene commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

tobibeer commented Sep 20, 2015

Jermolene commented Sep 23, 2015

felixhayashi commented Sep 23, 2015

felixhayashi commented Nov 5, 2015

Jermolene Nov 5, 2015

felixhayashi Nov 5, 2015

felixhayashi Nov 5, 2015

felixhayashi commented Dec 22, 2015

Jermolene commented Dec 22, 2015

Auto-height fix for text area which is contained in a scroll container #1933

Auto-height fix for text area which is contained in a scroll container #1933

Conversation

felixhayashi commented Sep 14, 2015

Description

Demo

pmario commented Sep 14, 2015

pmario commented Sep 14, 2015

felixhayashi commented Sep 14, 2015

pmario commented Sep 14, 2015

felixhayashi commented Sep 14, 2015

felixhayashi commented Sep 20, 2015

tobibeer commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

felixhayashi Sep 20, 2015

Choose a reason for hiding this comment

Jermolene Sep 20, 2015

Choose a reason for hiding this comment

felixhayashi Sep 20, 2015

Choose a reason for hiding this comment

felixhayashi commented Sep 20, 2015

Jermolene commented Sep 20, 2015

felixhayashi commented Sep 20, 2015

tobibeer commented Sep 20, 2015

Jermolene commented Sep 23, 2015

felixhayashi commented Sep 23, 2015

felixhayashi commented Nov 5, 2015

Jermolene Nov 5, 2015

Choose a reason for hiding this comment

felixhayashi Nov 5, 2015

Choose a reason for hiding this comment

felixhayashi Nov 5, 2015

Choose a reason for hiding this comment

felixhayashi commented Dec 22, 2015

Jermolene commented Dec 22, 2015