Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-form-select] Clicking to view dropdown causes window to scroll to the top while embedded in mpage #2682

Closed
po016255 opened this issue Oct 3, 2019 · 19 comments · Fixed by #2773
Assignees
Labels
Milestone

Comments

@po016255
Copy link

po016255 commented Oct 3, 2019

Bug Report

Description

While in an mpage, when you click the terra-form-select(default variant) to display the drop down contents, the page will scroll all the way up making it impossible to see the results. The popup is open and you can choose an option via keyboard(without being able to see it) or use scroll wheel to scroll down, but if you click the scrollbar the dropdown closes. This is behavior that will prevent consumers from performing this workflow.

HC Scrolling - status

Steps to Reproduce

  1. Open patient chart in solgm for a patient with health concerns. (Schreur,Grace)
  2. Open OnePlan Plus Button mpage tab
  3. Click a healthconcern in the list to open detail view.
  4. Click the modify button(top right) to open the modify view.
  5. Scroll down to "Health Concern Status" and click the drop down button.

The outermost mpage scroll bar will go from its current position all the way up. Depending on resolution, this bar could be all the way up already and thus this issue may not bee seen. But when using smaller resolutions, the user will see the drop down scroll out of view as in the gif. This most likely will happen normally in resolutions used on citrix by clients.
On a larger resolution monitor, you can reliably recreate the issue by shrinking down the size of the powerchart window.

Example on 4k monitor shrinking down the patient window slightly to recreate:
terra-form-select_scroll_issue_4k

Additional Context / Screenshots

When the default variant is used, the page will scroll up. On the same health concerns modify view when the "expressed by" drop down is chosen, the page does not scroll. That drop down is using the "combo box" variant.

Teams discussion

Expected Behavior

Do not change the scroll position when clicking drop down or at least keep the drop down control in view if the scroll position reacts to opening the drop down.

Environment

@ Mentions

@StephenEsser

@bjankord
Copy link
Contributor

bjankord commented Oct 3, 2019

We think we'll need a reduced test case to better debug this.

@bjankord bjankord added this to the Backlog milestone Oct 4, 2019
@ShettyAkarsh
Copy link
Contributor

@po016255 any updates on the reduced test case ?

@po016255
Copy link
Author

How do you want this reduced test case setup, this is currently a page accessible in powerchart. How are you guys going to be debugging this? The current example doesn't require manipulating any of the other controls on the page.

@ryanthemanuel
Copy link
Contributor

Is this something you can recreate using gaia for your component? Ideally we will need something that we can look at that is outside of powerchart

@po016255
Copy link
Author

What do you need to load it in gaia? We have a dcos endpoint in solgm and deveng that can be used, it is the same url being loaded in powerchart.

You can also run plan-js locally with mock data by checking it out, doing "npm install" and "npm run start". Working test url is http://localhost:8080/#/raw/tests/plan-js/plan-has-data You can choose any of the HCs and click modify to get to the page. Currently the issue is not happening in standalone IE11 directly. Does gaia recreate the iframe used in powerchart?

@supreethmr
Copy link
Contributor

@po016255
I am using Mpages-InBrowser Development Setup to debug the reported behaviour of form-select. here we need to get to source of page to run application on local browser, In this case it is the window in which form-select is present. so in order to navigate to page shown in image we need more elaborated steps to that will help us to navigate to the page where your seeing the above mentioned behaviour and also if there is some informations like sign-ons with permissions required to access this window please share those informations through mail.

@po016255
Copy link
Author

Is that https://github.cerner.com/MPagesEcosystem/mpage-gaia? We have a dcos endpoint that can be used, plan_engine for live data, and plan-js for mock data. They each have their own url.

What project do you want to use? I can give you specific instructions based on what you want to use. It will result in creating a URL that you should be able to set for the iframe used by the mpages.

@po016255
Copy link
Author

po016255 commented Oct 28, 2019

I setup gaia to see what would be needed. It appears you just need a working tab in powerchart.

Using the 6.13 static content you can use the live dev tab with:
CCL Program: mp_unified_driver
Program parameters: ^MINE^,1742024.0,18746697.0,118617332.0,441.0,1119.0,^powerchart.exe^,^$STATIC_CONTENT$^,^ONEPLAN^,9

Signon is "oneplan" in deveng. I can send you the password.

Within powerchart you can load by searching for patient "SCHREUR, GRACE" and choosing the tab called "One Plan". You can follow the instructions I had up top, click a health concern and then choose the modify button. The "Health Concern Status" box recreates the behavior in both chrome and ie. But you need to shrink the window so there is an outer scrollbar (unless your resolution is low enough to have the scroll bar be there already).

@po016255
Copy link
Author

Running in IE.

terra-form-select_page_scrolls_up_when_clicked

@StephenEsser
Copy link
Contributor

StephenEsser commented Nov 12, 2019

@supreethmr

Can you provide the code for the reproducible test-case mentioned above? Does this only happen when embedded in an iFrame?

Does the page jump when using the arrow keys to navigation the dropdown list of options. Each time a new option becomes active (using keyboard up/down arrows) scrollIntoView is called. We want the list to smoothly scroll during keyboard arrow interactions when the dropdown list has an overflow.

@po016255
Copy link
Author

po016255 commented Nov 12, 2019

I tested arrow keys, it doesn't happen when you are just navigating up and down. It happens when you click the box to show the menu for the first time. Although, it will scroll back down to put the control in view when you hit enter to select something in the menu.

Example:

  1. Control is in view.
  2. Click to show menu.
  3. Page scrolls up, control out of view.
  4. Use arrow key to select a different option while out of view.
  5. Hit enter to select new option while out of view.
  6. Page scrolls down so control is now in view again.

terra-form-select_page_control_scroll_into_view_enter_key

@StephenEsser
Copy link
Contributor

I spent some time looking into this during office hours and I do think that scrollTop might provide a solution to resolve the iframe jumping.

I was able to reproduce this locally by running the terra-core site in IE 10 using virtual box and adding an iframe into the document that referenced back to the select documentation page.

 <iframe src="/components/terra-form-select/form-select/select" />

Opening a select within the iframe and using the arrow keys to navigate the options caused the page to jump.

If we proceed with scrollTop we'll need to make sure we calculate the scroll offsets appropriately for arrow key navigation when the dropdown opens above/below as well as when the dropdown has a custom max height and is scrollable.

@ShettyAkarsh
Copy link
Contributor

@StephenEsser, as @supreethmr mentioned above we were able to fix this issue in our dev-site when form-select is rendered inside the iframe. we are working with the ipoc team here in BLR to render the same version of form-select in powerchart to make sure it works fine there also before logging the PR.

@supreethmr
Copy link
Contributor

supreethmr commented Nov 17, 2019

I was wrong about scrollIntoView() method being reason for scrolling page to top on form-select open. scrollIntoView() was called on keyboard navigation in above mentioned scenario.

As @StephenEsser mentioned scrollIntoView() does jumps page when using keyboard for navigation in form-select but on form select open it is focus() method which is causing page to scroll.

Root Cause : document.querySelector(this.selectMenu).focus();
The issue of page scrolled to top on form-select open is due to focus() method which sets focus to
ul menu element in openDropdown() method.
Focus() method by default scrolls to focused element this causing the whole page to scroll to top on form-select open.

I was able to re-produce this issue in chrome by modifying dev-site DefaultValueZero test example.

Branch with code changes

Video of scroll issue in chrome
chrome_select_scroll_issue1.zip

Video of scroll issue in IE 11/10
IE10_select_scroll_issue1.zip

I was able to fix this in all browser including IE11 by using preventScroll parameter of focus method like below :
document.querySelector(this.selectMenu).focus({ preventScroll: true });

But preventScroll does not seems to be working as expected in IE 10. page will still gets scrolled to top on form-select open.

@StephenEsser
Copy link
Contributor

StephenEsser commented Nov 19, 2019

Okay, I think I have a better understanding of what is happening.

Every focusable element within an iframe can shift the parent window. You can reproduce this by adding the form-input doc-site page into the iframe and using tab to navigate through the focusable items in the iframe. It will shift the parent window. Causing a similar jump effect. This is default behavior of the browser and I don't think it can be avoided.

The terra-form-select example makes this jump look more extreme because it is using hookshot to portal the content. Hookshot, by default, positions content invisible in the top left hand corner of the screen. Because we're using a 10 millisecond timeout before calling focus, I believe that the hookshot content is actually still positioned into the top lefthand corner of the iframe when we call focus(). Causing the page to jump the maximum distance to the top.

For testing purposing, increasing the timeout before calling focus can prevent the page from jumping. This allows the hookshot content to be property positioned on screen before focusing it.

In general, I don't think we should be using timeouts at all. In this case, since we are already using them, we might look into seeing how much of a timeout it takes to fix this issue. But a better solution would be to call focus only after we know the dropdown has been enabled/positioned. setState takes a callback function. We should only focus the menu once we know it has been positioned. ( Conditionally focus it if necessary. )

https://github.com/cerner/terra-core/blob/master/packages/terra-form-select/src/single/Frame.jsx#L239

Specifically the isEnabled prop which is passed to the Dropdown and onto Hookshot.

@supreethmr
Copy link
Contributor

supreethmr commented Nov 19, 2019

Thanks @StephenEsser for routing me to actual root cause. I was able to test this by setting focus after dropdown is positioned, It does resolves page scroll issue and prevents page from jumping to top.
While testing this solution i found that whenever form-select as selected option activeOption.scrollIntoView() is called before focus() method and activeOption.scrollIntoView() creates same page jump effect as focus() method was causing earlier as shown in the video : select_scroll_intoView.False.zip

I tested by replacing scrollIntoView() with scrollTop like below and it works well in IE-10 also on dropdown open active-option gets highlighted without causing page jump.

      document.documentElement.scrollTop = activeOption.offsetTop;

In keyboard navigation it fails to highlight the active option as we traverse through options using keyboard navigation keys could be due to setting scroll-top of document element to offsetTop of activeOption.

@supreethmr
Copy link
Contributor

The terra-form-select example makes this jump look more extreme because it is using hookshot to portal the content. Hookshot, by default, positions content invisible in the top left hand corner of the screen. Because we're using a 10 millisecond timeout before calling focus, I believe that the hookshot content is actually still positioned into the top lefthand corner of the iframe when we call focus(). Causing the page to jump the maximum distance to the top.

with Further investigation on form-select I found that page jump within Iframe is due to hookshot. As @StephenEsser mentioned in his comment hookshot content would not be positioned by the timescrollIntoView()in menu.jsx gets called. ( this was the same thing which was happening with focus() method ). the page scroll on dropdown Open is expected behavior of scrollIntoView() and I feel needs no changes in fixing this issue.

To ensure that page jump happens only when form-select rendered with hookshot, tested form-select with isTouchAccessible prop which removes hookshot and renders menu in normal dom.
form-select with isTouchAccessible = true doesn't causes page to jump on open instead scrolls into the selected item by highlighting at top of form-select.

with isTouchAccessible prop where selected option gets highlighted on Open :
form_select-scroll-2

without isTouchAccessible prop where page jumps to top on Open :
form_select-scroll-1

we can also recreate this issue in other components like terra-dropdown also. which uses hookshot to display menu-list.

I also found reference of other select components like native-select and react-select which renders dropdown in normal dom instead of rendering it in portal and doesn't causes page jump even when used within Iframe.

In below screenshot we can see that div with class name menu renders right after react-select :
Screen Shot 2019-11-22 at 6 47 47 PM

so would like to know your thoughts on using hookshot in form-select : @StephenEsser.

@supreethmr
Copy link
Contributor

supreethmr commented Nov 26, 2019

Adding to my previous comment, I found out few caveats of using form-select with isTouchAccessible prop ( without hookshot ) is that it clips to parent container width when overflow is set to hidden for parent-container. There are few components like terra-content-container, terra-dialog, terra-abstract-modal and terra-tabs which have overflow set to hidden, so using form-select with these components will lead to clipping issue as shown in below screenshots :

Content-Container :
Screen Shot 2019-11-26 at 6 50 57 PM

Terra-Dialog :
Screen Shot 2019-11-26 at 6 45 27 PM

Abstract-Modal:
Screen Shot 2019-11-26 at 6 50 31 PM

I tried to modify few styles in frame and dropdown styles to fix clipping issue. Since we have position: relative for select class in Frame.module.scss. this will make form-select position relative to it's parent which will be having overflow:hidden.

So I tried removing position: relative from select class in Frame.module.scss and modified is-touch-accessible class in _Dropdown.module.scss to :

  .is-touch-accessible {
    display: inline-block;
    left: auto;
    margin-top: 2.31%; // (need to find the value for margin-top since drop-down position will be absolute)
    position: absolute;
    z-index: 9001; // use same z-index as hookshot
  }

well with above css changes, form-select with isTouchAccessible prop looks fine as shown below but would like to know whether it is right way of fixing the clipping issue.
Screen Shot 2019-11-26 at 8 41 02 PM

I have updated branch for reference.

@StephenEsser
Copy link
Contributor

StephenEsser commented Nov 26, 2019

Long term we will need to investigate if it is possible to shift focus from a component into hookshot while using screen reader gestures on iOS mobile. If it is impossible we may need to investigate phasing hookshot out of the Select. This is a large and non-passive change that will need some planning.

Short term we should focus on solutions that resolve the page jumping as much as possible. I'd recommend splitting this issue into multiple parts. The first part would be resolving the page jump on the initial focus using the setState callback solution above. The second part would be solving the issue for page jumping when using keyboard navigation.

For the isTouchAccessible prop I think we should maintain the existing relative/absolute positioning logic. The clipping is always going to be limited by the nearest ancestor component with a relative style. Many of our components implement a relative positioning. We should encapsulate our positioning as much as possible. Solutions implementing the component will need to focus on designing around the component constraints.

@neilpfeiffer neilpfeiffer added this to the In Progress milestone Dec 9, 2019
@nramamurth nramamurth modified the milestones: In Progress, Backlog Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants