Fix top position, height, and scrolling with modals #2544

kristinalim · 2018-08-15T19:10:41Z

What? Why?

Closes Can't scroll to see in modals (profiles, longer product descriptions) #2201
Addresses unclickable button issue in Enterprise Registration - Scroll on first step #1766

These changes will also affect other modals and tall dialogs (smaller screens with height less than 500px).

See the following video:

https://www.useloom.com/share/002a740fbee8436aadcfa41aaae4326a

Notice the following:

Vertical position of the modal is initially at 10px from the top of the screen. After scrolling down, however, the vertical position from the top of the screen varies.
When you are not currently at the top of the page, opening the modal usually scrolls the page up a little bit.
The modal sometimes goes past the bottom of the screen.

For large screens, the modal is actually styled to be 10% of height of the viewport from the top of the screen. There was intention for the modals to start from the same position from the top.

Above behaviours were actually caused by three interconnected problems.

Fix 10% vertical position from the top of the screen

The modal JS library angular-foundation treats a % value for "top" CSS property as px value, so the intended 10% position from the top ends up being treated as 10px.

See details of the bug here: openfoodfoundation/angular-foundation#1

The code uses version v0.8.0 of https://github.com/yalabot/angular-foundation. I planned to submit a pull request addressing the issue, however, the library doesn't seem to be actively maintained anymore.

So, in my fork of the repository, I created a v0.8.1 release with the bug fix, and also a cherry-picked fix for unrelated failing tests.

I refrained from including other changes in the original repository's "master" branch (v0.9.0-SNAPSHOT) because I'm not sure if these are already stable enough.

(Note: The package is compatible with node v0.10.48. To compile, run grunt, and find the compiled and minified files in dist/. Following instructions in the README, these files were added to the "gh-pages" branch.

Make sure the modal does not go past the bottom of the screen

When there is enough content in the modal, the height of the modal plus its offset from the top could exceed the height of the screen.

I changed the maximum height of the modal to 80% for large screens. Considering a top position of 10%, a max height of 80% renders a tall modal vertically centered, with 10% remaining space at the bottom.

Fix scrolling up when the modal opens

Occasionally, the page scrolls up while the modal is being opened. This was causing the final position of the modal to be at the wrong location relative to the top of the screen.

This was happening because of a race condition between the animation that slides the modal from above the screen to the middle, and focus() which the modal does.

The final vertical position of the modal is 10% from the top, so the animation which translates the modal -25% vertically was starting -15% above the screen. The focus() then causes vertical scroll.

This lowers the starting point of the animation, so there will no longer be scrolling upon focus.

Additionally, the animation was updated to happen only on large screens. The CSS property "top" is 0 for smaller screens.

The following bug was also addressed.

Do not exceed screen height in smaller screens less than 500px tall

See the following video:

https://www.useloom.com/share/bf19aca8836142e1b321c484ecb2de25

What should we test?

For large screens:

Go the the product list of an enterprise.
Click on a product without scrolling.
Check that the modal starts at around ~~10%~~ 5% of the screen from the top.
Repeat Items 2 and 3 for a couple more products.
Scroll to the end of the list.
Remember which part of the page is currently visible.
Click on a product without scrolling.
a. Check that the modal starts at around ~~10%~~ 5% of the screen from the top.
b. Close the modal.
c. Check that the visible part of the page is still the same, i.e. the page should not have scrolled up.
Repeat Item 7 for a couple more products.
Click on the a product with a long description.
Check that the modal starts at around ~~10%~~ 5% of the screen from the top, and also has a distance of ~~10%~~ 5% of the screen from the bottom.

For smaller screens with less than 500px height:

Go the the product list of an enterprise.
Scroll down to a product.
Remember which part of the page is currently visible.
Click on the product.
Check that the modal is in full screen mode, starting from the top of the screen and ending at the bottom.
Check that you can scroll all the way to the bottom of the content of the modal.
Close the modal.
Check that the visible part of the page is still the same, i.e. the page should not have scrolled up.

Other modals to check:

Login modal
Signup modal
Accept Cookies modal - Load homepage in incognito window.
Cookies Policy modal
Modal for editing product images, by clicking product image in admin product list.

Release notes

Fix top position and page scrolling with modals
Reduce maximum height of modals and dialogs in large screens
Limit maximum height of modals and dialogs in small screens

Changelog Category: Fixed

luisramos0

Awesome work @kristinalim !
the change makes sense to me, I'll test it tomorrow and let you know.

luisramos0 · 2018-08-15T21:29:45Z

app/assets/stylesheets/darkswarm/modals.css.scss

@@ -28,7 +28,7 @@ dialog, .reveal-modal {
  // Medium and up - when modal IS NOT full screen
  @media only screen and (min-width: 641px) {
    top: 10%;
-    max-height: 120%;


on my almost-merged cookies work I have changed this max-height to 100%. On the cookies policy page modal I want it to go to until the end of the screen: lots of content. Maybe we can set this to 95% to cover both cases?

@luisramos0 Thanks for the heads-up! Seeing that your cookies banner has been merged into by now. I will rebase this branch upon master.

Hmm... If we retain the 10% position at the top but use a max height of 95%, we might end up with some hidden content again on some screen dimensions.

I see that your changes apply a CSS class "cookies-policy-modal" to the modal. I could try just adding custom styles that sets "top" to 2% and "max-height" to 96% (and animation to start at -2%) only for this particular modal. It would render similar to how it does in master now.

Are you okay with this?

yes, cookies policy modals can take it. I was just trying to avoid the !important:
top: 2% !important;
max-height: 96% !important;

Do you know how can we avoid these !importants?
One easy trick I used in the past is to have the generic/common css files (like modals.css in this case) be on the top of the all.css file. I am not sure how the stylesheets are loaded into all.css but it may well be alphabetically and because Modals comes after Cookies the styles on modals are read after and get the priority :-(

Any suggestions?

Ah! Yes, you're right, the files would be loaded in alphabetical order, so cookies_policy_modal.css.scss would be loaded before modals.scss.

I'm seeing two options:

Use more specific selectors for the cookies policy modal, like this:

.cookies-policy-modal.reveal-modal { }

Load the CSS partial for the cookies policy modal last.

I think the second is more appropriate.

In my experience, page-specific CSS (I would categorize cookies policy modal CSS under this) are best to be loaded last, and could be loaded in any order among themselves because they are not reused.

So far, the file structure under darkswarm/ is flat. I've been a fan of the 7-1 Pattern, which guides the file structure of CSS partials and helps make their load order easier to manage. Following this, I would move the CSS specific to the cookies policy modal under pages/.

The following would then make sure that all the other CSS would still load in the way that they already do:

@import '*'; // This will not import subdirectories. @import 'pages/*';

For a start, I would also move CSS for the other modals under pages/. The rest of the files, we could reorganize as we go along, or when we could properly audit. I'm also not sure yet how OFN instances make CSS customizations, so not sure how messy that would be.

What do you think?

perfect! 7.1 is fine. i am a fan of funcional organizations so id put cookiespolicy css next to the js files and htnl of that page. but rails is always organizing things by type(controllers/models/etc), which I dont like.
Anyway, what you described sounds awesome and id support that change!
i dont think instances have css customizations.
thanks!

I split **/* into a couple of lines, to allow us to start specifying the order in which we import the CSS partials. The files in pages/* are loaded last.

About the taller cookies policy modal, it seems that it is not easy to apply custom position and height to modals - this doesn't seem to be supported by the modal library.

To work around this limitation, I went ahead and made a general change for all modals in large screens to start from 5% and have 90% max height, instead of 10% and 80%. Before these changes, most modals were rendering at 10px top position, not the originally planned 10%. Changing this to 5% doesn't seem to be much of a compromise, and it works out nicely.

Let me know what you think.

luisramos0 · 2018-08-18T14:44:24Z

app/assets/stylesheets/darkswarm/modals.css.scss

@@ -20,9 +20,10 @@ dialog, .reveal-modal {
  // Small - when modal IS full screen
  @media only screen and (max-width: 640px) {
    height: 500px;


Hey, now that we are on this one.

I see @HugsDaniel change past Feb2018:
from

min-height: 100%; // This is needed to make the height not the height of whole content page
to

height: 500px;

Is there a reason why the modals (for example the login modal) should not take the full screen if there's stuff to be shown?
I'd go for max-height: 100% here and use the empty space below the 500px point.
What do you think?

I'm not sure, but I'm thinking the min-height: 100% could have been made before the following was added:

body.modal-open { overflow: hidden; }

The comment would have been true - 100% height for the modal would have occupied the full length of the then scrollable body.

@luisramos0 I removed height: 500px for small screens, to allow the modal to go past this height. There doesn't seem to be any issue from doing that.

luisramos0 · 2018-08-18T14:46:09Z

app/assets/javascripts/admin/all.js

@@ -60,7 +60,7 @@
 //= require moment/nb.js
 //= require moment/pt-br.js
 //= require moment/sv.js
-//= require ../shared/mm-foundation-tpls-0.8.0.min.js
+//= require ../shared/mm-foundation-tpls-0.8.1.min.js


Looks good! I think we must copy/move this fork to the OFN org somewhere like http://github.com/openfoodfoundation/angular-foundation

Good idea to fork it to our organisation. There also seems to be another fork that had a couple of useful commits: https://github.com/cwadrupldijjit/angular-foundation

We could also open our pull request against that repository and see if it's maintained. If not, it could be useful to cherry-pick those commits for our fork.

I'm not sure if @kristinalim has organisation permissions to create new repositories though. That may be something we have to do. But are you happy to open another pull request with the other repository first and see if that guy would like to be the new maintainer?

I can reassign the repository to OFN, no problem. I wouldn't need prior permission, but OFN would need to approve the transfer of ownership within one day.

I actually submitted a pull request to the repository already: yalabot/angular-foundation#319 (Oops! There's a merge conflict with 0.9.0-SNAPSHOT there - I will fix that.) But there is hint that the repository will no longer be maintained.

Ah, I didn't realize that you meant I could try opening a pull request at https://github.com/cwadrupldijjit/angular-foundation instead. 🙂 I will do that. Thanks, @mkllnk!

Here are the PRs to the original repository and another fork:

Fix vertical position of modal when ".reveal-modal" has % "top" property yalabot/angular-foundation#319

Fix vertical position of modal when ".reveal-modal" has % "top" property cwadrupldijjit/angular-foundation#1

I created a release 0.9.0-20180826174721 which is based on the "master" branch in https://github.com/cwadrupldijjit/angular-foundation/ and cherry-picks the modal fixes. (The compiled JS is in the gh-pages branch, following the process for the angular-foundation project.) This is the version which this PR now uses.

The instructions about transferring repositories which I read must have been for non-organization Github accounts. I couldn't initiate a transfer to openfoodfoundation. Would a fork of my fork will be sufficient, while we're waiting to see if any of the PRs could get merged?

luisramos0

I have tested the changes and it works. The code looks good!
3 points raised:

move angular-foundation fork to the ofn org
modals max-height: 80% - merge darkswarm/modals.css with the cookies code
modals max-height: 500px

Great work @kristinalim , thanks!

mkllnk

Great detective work.

mkllnk · 2018-08-19T23:48:03Z

app/assets/javascripts/admin/all.js

@@ -60,7 +60,7 @@
 //= require moment/nb.js
 //= require moment/pt-br.js
 //= require moment/sv.js
-//= require ../shared/mm-foundation-tpls-0.8.0.min.js
+//= require ../shared/mm-foundation-tpls-0.8.1.min.js


Good idea to fork it to our organisation. There also seems to be another fork that had a couple of useful commits: https://github.com/cwadrupldijjit/angular-foundation

We could also open our pull request against that repository and see if it's maintained. If not, it could be useful to cherry-pick those commits for our fork.

I'm not sure if @kristinalim has organisation permissions to create new repositories though. That may be something we have to do. But are you happy to open another pull request with the other repository first and see if that guy would like to be the new maintainer?

kristinalim · 2018-08-27T03:24:52Z

This is now ready for another round of reviews.

sauloperez

Impressive! agree with @luisramos0 on

move angular-foundation fork to the ofn org

sigmundpetersen · 2018-08-30T12:19:24Z

@luisramos0 please review the updates and move to Test Ready if good

kristinalim · 2018-08-30T13:13:17Z

Can I get temporary access to create repositories on openfoodfoundation? Right now, I'm getting a permission error when I attempt to transfer the angular-foundation repository.

luisramos0 · 2018-08-30T15:38:11Z

@sigmundpetersen we need to move the repo first.
@kristinalim I don't have that permission.

sauloperez · 2018-08-30T17:03:58Z

try again now @kristinalim

kristinalim · 2018-08-30T18:18:11Z

@sauloperez I still get this: "You don’t have the permission to create repositories on openfoodfoundation"

luisramos0 · 2018-08-30T21:53:47Z

app/assets/stylesheets/darkswarm/modals.css.scss

@@ -20,7 +20,6 @@ dialog

  // Small - when modal IS full screen
  @media only screen and (max-width: 640px) {
-    height: 500px;


luisramos0

Awesome, after the foundation repo is moved to OFN, it's good to go. Needs some good UI testing on different fronts.

kristinalim · 2018-08-31T09:55:47Z

I was already able to transfer the repository to the "openfoodfoundation" Github org. (Thank you, @sauloperez!)

I also added section "Other modals to check" in recommended testing notes above.

Moving this PR to "Test Ready".

mkllnk · 2018-09-03T05:01:06Z

@kristinalim There is a conflict that needs resolving now, sorry.

kristinalim · 2018-09-03T16:44:33Z

Thanks, @mkllnk! I rebased this and did some checks, and moved this back to "Test Ready".

sauloperez · 2018-09-05T14:14:22Z

I'm very sorry @kristinalim. More conflicts 🙈

When there is enough content in the modal, the height of the modal plus its top margin could exceed the height of the viewport. Considering a top position of 10%, a max height of 80% renders a tall modal vertically centered, with 10% remaining space at the bottom.

Occasionally, the page scrolls up while the modal is being opened. This was causing the final position of the modal to be at the wrong location relative to the viewport. This was happening because of a race condition between the animation that slides the modal from above the viewport to the middle, and focus() which the modal does: https://github.com/yalabot/angular-foundation/blob/0.8.0/src/modal/modal.js#L109 The final vertical position of the modal is at 10%, so the animation which translates the modal -25% vertically was starting -15% above the viewport. The focus() was then causing vertical scroll. This lowers the starting point of the animation, so there will no longer be scrolling. Additionally, the animation would only happen on large screens. The CSS property "top" is 0 for smaller screens.

There is a bug in the handling of % values for the "top" CSS property of the modals. See details here: openfoodfoundation/angular-foundation#1 A PR to the original repository has also been submitted, but the project doesn't seem to be active anymore: yalabot/angular-foundation#319 And to another fork of the repository: cwadrupldijjit/angular-foundation#1 The bug was causing the 10% "top" CSS property for the modal to be treated as 10px.

Change modals for large screens from 10% position from top and 80% max height, to 5% position from top and 90% max height. This is to accommodate the taller cookies policy modal. It seems that it is not easy to apply a custom position and height to a specific modal only. This doesn't seem to be supported by the modal library currently being used. Before the recent changes, most modals were rendering at 10px top position (not the originally planned 10%), so changing this to 5% doesn't seem to be much of a compromise.

kristinalim · 2018-09-05T18:41:43Z

Haha, @sauloperez. 😆 I rebased, and also checked that changes of @luisramos0 in #2611 are still okay. Checks are now green, though the build on Semaphore failed a couple of times first.

luisramos0 · 2018-09-06T22:35:11Z

staged here: https://staging.openfoodfrance.org/

sstead · 2018-09-07T02:11:26Z

Testing notes

Regarding this issue #1766 , I saw that on mobile the scrolling and button visiblility was good. But for small windows on a laptop I still needed to scroll (contrary to Lyn's original suggestion that the model becomes scroll free) but the button is clickable. Please just check that we're happy the requirements of /1766 are met.

One thing that could be improved is the display of backend product image modal in a small screen view. It didn't sit at the top of the screen, requiring scrolling. Not crucial.

There's one thing I couldn't test... could @myriamboure or @RachL please turn on cookies for the instance, and then check that the cookies modal displays correctly on big screens, small screens and mobile? Thanks. It's in Configuration > General Settings

Besides these things the rest looks good.

https://docs.google.com/document/d/1V41s7QatTIDCiss6L905tZdH6oyrPiZ8BNupUnO5Ksw/edit#

RachL · 2018-09-07T10:34:07Z

I've tested and added my screenshots to your document @sstead. However I agree that the requirement of issue #1766 is not met. Can we unlick this issue so it stays open and move the rest to ready to go? I've never done this so far. cc @luisramos0 @kristinalim

luisramos0 · 2018-09-07T16:17:29Z

If you move this to ready to go, only the linked issue 2201 will be moved. 1766 will stay as it is I believe.

daniellemoorhead · 2018-09-07T23:06:34Z

If you move this to ready to go, only the linked issue 2201 will be moved. 1766 will stay as it is I believe.

Unfortunately/fortunately this isn't the case @luisramos0 so I've disconnected the PR. @RachL it would be worth putting a note on the original issue to say what is remaining to be done.

luisramos0 · 2018-09-10T11:16:57Z

Are we moving this PR to ready to go and keeping the issue open? (that's what I understood)
Can I stage something else on staging-fr?

kristinalim · 2018-09-10T11:34:59Z

@luisramos0 Yes, as far as I understand, we are in agreement that this PR is okay for the next release. #1766 in mobile is still better off with these changes.

I updated the PR description to specify that only the issue of the button in #1766 being unclickable is resolved by this PR. I also moved #1766 back to "Dev Ready". (I reopened it, so it was in "All the Things".)

luisramos0 · 2018-09-10T11:43:27Z

awesome @kristinalim
@RachL or @sstead can you pls confirm and move it to ready to go?

RachL · 2018-09-11T13:01:46Z

@luisramos0 yes confirming and moving this along

kristinalim self-assigned this Aug 15, 2018

kristinalim force-pushed the fix-scrolling_in_modals branch 2 times, most recently from 2e28ad2 to 1efce9e Compare August 15, 2018 19:20

luisramos0 reviewed Aug 15, 2018

View reviewed changes

luisramos0 reviewed Aug 18, 2018

View reviewed changes

luisramos0 suggested changes Aug 18, 2018

View reviewed changes

This was referenced Aug 19, 2018

New Cookies banner #2411

Merged

Enterprise Registration - Scroll on first step #1766

Closed

mkllnk reviewed Aug 19, 2018

View reviewed changes

kristinalim force-pushed the fix-scrolling_in_modals branch 3 times, most recently from 2074195 to 9c8c7c7 Compare August 27, 2018 03:09

sauloperez approved these changes Aug 30, 2018

View reviewed changes

luisramos0 reviewed Aug 30, 2018

View reviewed changes

luisramos0 approved these changes Aug 30, 2018

View reviewed changes

kristinalim force-pushed the fix-scrolling_in_modals branch from 9c8c7c7 to 9bcca71 Compare September 3, 2018 16:28

kristinalim added 3 commits September 5, 2018 23:58

kristinalim added 4 commits September 5, 2018 23:58

Keep height of modal, dialog within screen height

f03eb02

Move some CSS to pages/ subdirectory.

f472519

Remove height limit for modals in small screens

9258e8c

kristinalim force-pushed the fix-scrolling_in_modals branch 3 times, most recently from 17b34de to 25d5112 Compare September 5, 2018 16:55

Clean up SCSS as suggested by scss-lint

339ea6f

kristinalim force-pushed the fix-scrolling_in_modals branch from 25d5112 to 339ea6f Compare September 5, 2018 18:22

luisramos0 added the pr-staged-fr staging.coopcircuits.fr label Sep 6, 2018

RachL assigned sstead Sep 7, 2018

RachL removed the pr-staged-fr staging.coopcircuits.fr label Sep 11, 2018

openfoodfoundation deleted a comment from luisramos0 Sep 11, 2018

sauloperez merged commit 64e71a1 into openfoodfoundation:master Sep 12, 2018

Fix top position, height, and scrolling with modals #2544

Fix top position, height, and scrolling with modals #2544

Conversation

kristinalim commented Aug 15, 2018 • edited Loading

What? Why?

Fix 10% vertical position from the top of the screen

Make sure the modal does not go past the bottom of the screen

Fix scrolling up when the modal opens

Do not exceed screen height in smaller screens less than 500px tall

What should we test?

Release notes

luisramos0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment • edited Loading

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinalim commented Aug 27, 2018

sauloperez left a comment

Choose a reason for hiding this comment

sigmundpetersen commented Aug 30, 2018

kristinalim commented Aug 30, 2018

luisramos0 commented Aug 30, 2018

sauloperez commented Aug 30, 2018

kristinalim commented Aug 30, 2018

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

kristinalim commented Aug 31, 2018

mkllnk commented Sep 3, 2018

kristinalim commented Sep 3, 2018

sauloperez commented Sep 5, 2018

kristinalim commented Sep 5, 2018

luisramos0 commented Sep 6, 2018

sstead commented Sep 7, 2018 • edited Loading

RachL commented Sep 7, 2018 • edited Loading

luisramos0 commented Sep 7, 2018

daniellemoorhead commented Sep 7, 2018

luisramos0 commented Sep 10, 2018

kristinalim commented Sep 10, 2018

luisramos0 commented Sep 10, 2018

RachL commented Sep 11, 2018

kristinalim commented Aug 15, 2018 •

edited

Loading

luisramos0 Aug 17, 2018 •

edited

Loading

luisramos0 left a comment •

edited

Loading

sstead commented Sep 7, 2018 •

edited

Loading

RachL commented Sep 7, 2018 •

edited

Loading