[Overlay] Split out AutoLockScrolling #3690

cgestes · 2016-03-14T09:22:01Z

One component, one feature.

AutoLockScrolling is useful on it's own.

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

alitaheri · 2016-03-14T10:43:21Z

This is very interesting 👍 👍

oliviertassinari · 2016-03-14T19:48:12Z

src/AutoLockScrolling.jsx

+    }
+  }
+
+  _preventScrolling() {


Can we remove the _ from our methods and properties?

chrismcv · 2016-03-14T22:00:08Z

There are issues with AutoLockScrolling in the current implementation with nested dialogs, when they unmount out of order. This component doesn't make it better or worse, but would be great to consider it as well when thinking about this.

cgestes · 2016-03-14T22:45:03Z

@chrismcv tried a fix :)

cgestes · 2016-03-16T16:50:03Z

gentle ping :)

oliviertassinari · 2016-03-16T19:37:57Z

@chrismcv Is right, the AutoLockScrolling is not handling correctly multiple mounted instances. Sorry @cgestes but I don't think that your fix is perfect 😁. I think that the scroll should be locked as soon as one <AutoLockScrolling /> is asking for it.

This really remembers me issues with concurrent access to resources like memory in a multi-thread environment.

What do you think about using a semaphore?
Each token would represent the access to the lock resource. We would only unlocked the screen once all tokens are returned.

oliviertassinari · 2016-03-16T19:39:34Z

@cgestes What do you think about just extracting the AutoLockScrolling component in this PR, then tackling the nested issue? (too hard to review like this)

alitaheri · 2016-03-16T19:53:57Z

What do you think about using a semaphore?

That's really interesting 👍 I think it's the best approach. just keep an integer ( increment with mount decrement with unmount ) and then set with non-zero and set with zero 👍

cgestes · 2016-03-16T23:22:55Z

@alitaheri & @oliviertassinari I did not know the locker was not supposed to be the unlocker.

Will implement it with a counter, thanks for the idea :)

chrismcv · 2016-03-16T23:32:13Z

sorry - been a busy day... I agree, in that I don't think the suggested fix would work. In my head, moving the lock variable outside the component this scope would be adequate, but counter approach works well too - again - I'm pretty sure it needs to be outside the this scope.

cgestes · 2016-03-17T09:10:38Z

updated the PR with a semaphore :)

chrismcv · 2016-03-17T10:33:58Z

This looks good to me :)

cgestes · 2016-03-17T10:45:25Z

Reviewing the commit again...

I dont think it is...

I'll add inline comments

cgestes · 2016-03-17T10:46:07Z

src/AutoLockScrolling.jsx

+  }
+
+  allowScrolling() {
+    lockingCounter = lockingCounter - 1;


we should decrement only if we incremented.

I have a bad feeling that if we switch the lock property on/off while the component is mounted it might go wrong. not sure.

I have a plane to take, so I'll have to check it again carefully later.

why? I can't see a problem with always incrementing?

I fear that we can double increment while decrementing only once. or the reverse.

cgestes · 2016-03-17T14:36:59Z

I'll add an extra security.

I believe if we have lock = false we decrease the counter without having incremented it.

Please dont merge now.

cgestes · 2016-03-17T15:45:49Z

I believe this time everything is ok.

It will only unlock if it has been locked.

What do you think?

oliviertassinari · 2016-03-17T18:15:18Z

I'm having a new look at this PR.
I don't think that we need the this.locked variable. I think that lockingCounter is enough.

Can we add some unit test to this component?
Each line of code matters here, regressions are likely.

cgestes · 2016-03-17T22:05:42Z

from what I understand we do.

if lock is false the counter get decremented in CWU but never incremented
the code is safer to reason about

and better be safe when we play with a global semaphore.

cgestes · 2016-03-17T22:09:56Z

The code is not gonna evolve a lot. it's just a simple component doing one thing with no possible side effect.
It's nice to have test, but that's not critical for that kind of code from my own experience.

I'll try to add some when I'll come back from ski in 10 days.

I'am not sure how to do unittest which have impact on the DOM.

oliviertassinari · 2016-03-24T20:49:10Z

I'll come back from ski in 10 days.

@cgestes Enjoy ❄️!

cgestes · 2016-03-28T10:19:25Z

rebased :)

nathanmarks · 2016-03-28T13:47:19Z

@cgestes Looks like rebasing went a bit haywire, can you try again?

cgestes · 2016-03-28T13:50:55Z

fixed. sorry for the inconvenient.

oliviertassinari · 2016-03-29T21:29:48Z

@cgestes That seems to work correctly 👍. I'm fine with merging this PR.
But I have some remarks:

I think that we can handle the logic without the locked variable. I can't see cases where allowScrolling or disallowScrolling would not increment / decrement the counter correctly.
We could be using the ES7 class properties for the locked and not calling the constructor()
Some test would be awesome if we want to easily refactor this component later.

oliviertassinari · 2016-03-29T21:32:09Z

src/internal/Overlay.js

@@ -131,7 +94,9 @@ const Overlay = React.createClass({
    const styles = getStyles(this.props, this.state);

    return (
-      <div {...other} ref="overlay" style={prepareStyles(Object.assign(styles.root, style))} />
+      <div {...other} ref="overlay" style={prepareStyles(Object.assign(styles.root, style))}>
+        {this.props.autoLockScrolling && <AutoLockScrolling lock={this.props.show} />}


The show variable is already destructured. We can use it. It would be good to do the same with autoLockScrolling.

{autoLockScrolling && <AutoLockScrolling lock={show} />}

cgestes · 2016-03-29T23:16:39Z

this.locked is extra safety and code readability.

For example when you mount with lock=false and you unmount this ensure that the counter is not decremented.

cgestes · 2016-03-29T23:17:58Z

rebased on @mbrookes changes :)

oliviertassinari · 2016-03-30T19:59:10Z

For example when you mount with lock=false and you unmount this ensure that the counter is not decremented.

True, we had this line to prevent this issue.

rebased on @mbrookes changes :)

Thanks.

cgestes · 2016-04-02T23:53:02Z

second rebase done :)

alitaheri added the PR: Needs Review label Mar 14, 2016

oliviertassinari reviewed Mar 14, 2016
View reviewed changes

cgestes force-pushed the cg-autolockscrolling branch from 58c4803 to a9e8786 Compare March 14, 2016 22:55

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 16, 2016

cgestes force-pushed the cg-autolockscrolling branch from a9e8786 to 03b9ce9 Compare March 17, 2016 09:09

oliviertassinari added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 17, 2016

cgestes reviewed Mar 17, 2016
View reviewed changes

cgestes force-pushed the cg-autolockscrolling branch from 03b9ce9 to 0b79dca Compare March 17, 2016 15:44

cgestes force-pushed the cg-autolockscrolling branch from 0b79dca to 4972513 Compare March 17, 2016 15:47

cgestes force-pushed the cg-autolockscrolling branch from 4972513 to b59350b Compare March 17, 2016 22:04

oliviertassinari self-assigned this Mar 18, 2016

cgestes force-pushed the cg-autolockscrolling branch from b59350b to 23eceb4 Compare March 28, 2016 10:19

cgestes force-pushed the cg-autolockscrolling branch from 23eceb4 to 9e0a915 Compare March 28, 2016 13:49

mbrookes added the Refactoring label Mar 28, 2016

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Mar 29, 2016

oliviertassinari reviewed Mar 29, 2016
View reviewed changes

oliviertassinari added PR: Needs Review and removed PR: Review Accepted labels Mar 29, 2016

mbrookes added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 29, 2016

cgestes force-pushed the cg-autolockscrolling branch from 9e0a915 to b84ffa5 Compare March 29, 2016 23:17

mbrookes removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 29, 2016

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Mar 30, 2016

mbrookes added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 2, 2016

[Overlay] Split out AutoLockScrolling

da9009a

cgestes force-pushed the cg-autolockscrolling branch from b84ffa5 to da9009a Compare April 2, 2016 23:52

oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 3, 2016

mbrookes merged commit 839a554 into mui:master Apr 3, 2016

cgestes mentioned this pull request Apr 13, 2016

Closing nested dialogs with ESC doesn't reshow scrollbar #3949

Closed

zannager added the package: system Specific to @mui/system label Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Overlay] Split out AutoLockScrolling #3690

[Overlay] Split out AutoLockScrolling #3690

cgestes commented Mar 14, 2016

alitaheri commented Mar 14, 2016

oliviertassinari Mar 14, 2016

cgestes Mar 14, 2016

chrismcv commented Mar 14, 2016

cgestes commented Mar 14, 2016

cgestes commented Mar 16, 2016

oliviertassinari commented Mar 16, 2016

oliviertassinari commented Mar 16, 2016

alitaheri commented Mar 16, 2016

cgestes commented Mar 16, 2016

chrismcv commented Mar 16, 2016

cgestes commented Mar 17, 2016

chrismcv commented Mar 17, 2016

cgestes commented Mar 17, 2016

cgestes Mar 17, 2016

cgestes Mar 17, 2016

chrismcv Mar 17, 2016

cgestes Mar 17, 2016

cgestes commented Mar 17, 2016

cgestes commented Mar 17, 2016

oliviertassinari commented Mar 17, 2016

cgestes commented Mar 17, 2016

cgestes commented Mar 17, 2016

oliviertassinari commented Mar 24, 2016

cgestes commented Mar 28, 2016

nathanmarks commented Mar 28, 2016

cgestes commented Mar 28, 2016

oliviertassinari commented Mar 29, 2016

oliviertassinari Mar 29, 2016

cgestes commented Mar 29, 2016

cgestes commented Mar 29, 2016

oliviertassinari commented Mar 30, 2016

cgestes commented Apr 2, 2016

[Overlay] Split out AutoLockScrolling #3690

[Overlay] Split out AutoLockScrolling #3690

Conversation

cgestes commented Mar 14, 2016

alitaheri commented Mar 14, 2016

oliviertassinari Mar 14, 2016

Choose a reason for hiding this comment

cgestes Mar 14, 2016

Choose a reason for hiding this comment

chrismcv commented Mar 14, 2016

cgestes commented Mar 14, 2016

cgestes commented Mar 16, 2016

oliviertassinari commented Mar 16, 2016

oliviertassinari commented Mar 16, 2016

alitaheri commented Mar 16, 2016

cgestes commented Mar 16, 2016

chrismcv commented Mar 16, 2016

cgestes commented Mar 17, 2016

chrismcv commented Mar 17, 2016

cgestes commented Mar 17, 2016

cgestes Mar 17, 2016

Choose a reason for hiding this comment

cgestes Mar 17, 2016

Choose a reason for hiding this comment

chrismcv Mar 17, 2016

Choose a reason for hiding this comment

cgestes Mar 17, 2016

Choose a reason for hiding this comment

cgestes commented Mar 17, 2016

cgestes commented Mar 17, 2016

oliviertassinari commented Mar 17, 2016

cgestes commented Mar 17, 2016

cgestes commented Mar 17, 2016

oliviertassinari commented Mar 24, 2016

cgestes commented Mar 28, 2016

nathanmarks commented Mar 28, 2016

cgestes commented Mar 28, 2016

oliviertassinari commented Mar 29, 2016

oliviertassinari Mar 29, 2016

Choose a reason for hiding this comment

cgestes commented Mar 29, 2016

cgestes commented Mar 29, 2016

oliviertassinari commented Mar 30, 2016

cgestes commented Apr 2, 2016