Fix for: focused input in dialog is scrolled out of viewport when soft keyboard appears

[engineering-this]

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

What changes did you make?

This is early PR with proposal of solution.

Current solution

When soft keyboard appears on touch devices it causes 'resize' event on which we were recalculating center position of fixed dialog. After that browser tried to scroll into view of input but it took wrong position, probably before moving it.
Such complex way of centering dialog is unnecessary, as dialog can't be dragged without mouse.

I have reworked how a dialog is styled:

• Dialogs wrapper now has position: fixed with width and height set to 100%.
• When the dialog is shown document.body gets a class with 'overflow:hidden', and style padding-right calculated from width of scrollbar.
• In IE the dialog is centered via absolute position calculated by script.
• In any other browser the dialog is centered by only CSS.
• When the dialog is is dragged it becomes absolute positioned.

After changes we won't adjust dialogs position once opened on the mobile devices, it will be positioned only by CSS and the way it's scrolled into view of focused inputs is only relying on browser implementation.

Unfortunately browsers implementation isn't perfect and sometimes it still scrolls it in wrong way, but that's rare. And it should be mentioned that this can happen even with fully static pages, with no js, and everything relative positioned.

Other solution

Adding once( 'scroll' ... inside focus listener and calling scrollIntoView on currently focused input. This is much simpler, but creates visible flickering, so I dropped this idea for now.

Todo

• Adding styles to other skins, and testing them.
• Improving way how missing scrollbars are covered with padding, or dropping idea to let page expand.
• Unit tests.
• Cleaning code.

Closes #2395. Closes #3359. Closes #453.

[jacekbogdanski]

I can admit that with this changes Androind behaves much better.

There are a couple of issues of this implementation:

Missing unit tests

Although I understand that is early PR so you have in mind to add appropriate tests.

Changing dialog size

When changing the dialog size scrollbars shows up:

It doesn't occur on master.

iOS still bugged

Your changes don't fix iOS at all, it behaves the same before the fix. However, this bug was mostly annoying on Android and dialog works correctly on iOS around 80% of the time.

It would be nice to spend some time trying to fix iOS also, although I believe fixing it may consume a lot of time.

[engineering-this]

@jacekbogdanski this fix is meant for Android devices, but also affects all modern browsers (in positive way I think). Although it doesn't fix issues with iOS, it doesn't look worse in any way. Mobile browsers in general has some problems with focusing inputs, especially when page is zoomed and I think it works pretty good on iOS.

[engineering-this]

I've added unit tests, changelog entry, styles for other skins and manuals for each skin.

[jacekbogdanski]

@engineering-this referencing to #2413 (comment) - I see your point. However, I'm not sure if we have any real profit from additional condition thus you still have to calculate dialog position for Internet Explorer. After our F2F talk I thought that this code is called every time when the browser window is resized. Although now I see that this point is valid only for IE (correct me if I'm wrong). So we are introducing additional, misleading condition, however, the rest of the code smoothly handles this case.

Could you elaborate if you see any additional benefit (like performance etc. ) which I don't see? Note that layout have to still be recalculated if dialog has been moved.

[engineering-this]

@engineering-this referencing to #2413 (comment) - I see your point. However, I'm not sure if we have any real profit from additional condition thus you still have to calculate dialog position for Internet Explorer. After our F2F talk I thought that this code is called every time when the browser window is resized. Although now I see that this point is valid only for IE (correct me if I'm wrong). So we are introducing additional, misleading condition, however, the rest of the code smoothly handles this case.

Could you elaborate if you see any additional benefit (like performance etc. ) which I don't see? Note that layout have to still be recalculated if dialog has been moved.

The fix seems to work pretty good on mobiles, but since we don't have any way to detect mobile I don't see other way. On the other hand CSS role is to position elements. And centering an element in a view with absolute position calculated with JS and adjusting it on every resize event is an overkill. I don't know if there is benchmark that could compare rendering speed in our cases, but I bet just dropping resize listener for non IE browsers is good for performance.

Theoretically we could completely drop listening for resize, as it is possible to center element in IE8+ with CSS only too, but that's a bit hacky, and requires additional wrapper. We could either keep 'flex' for modern browsers and use hax for IE or use haxy IE code for everything.

<div style="display:table;width:100%;height:100%;">First container.
  <div style="display:table-cell;vertical-align:middle;">Second container.
    <div style="margin-left:auto;margin-right:auto;">This is centered element.</div>
  </div>
</div>

I don't like one thing tho: we are changing dom structure which leads risk of breaking things. So I'm for keeping old logic for IE as it works there perfectly fine.

[engineering-this]

Changes:

• Removed more parts of code, mostly code that didn't do anything.
• Updated test due to plugin changes.
• Fixed test failing for small browser window.
• Removed functions that became one liners.

@jacekbogdanski I have one concern. How do you think we should deal with following case:

1. User drags dialog, dialog it becomes absolutely positioned.
2. User resizes window, so dialog is outside of viewport.

Should we leave it as it is now (scrollbars appears once dialog is outside of viewport). Or move dialog into view on resize?

[jacekbogdanski]

I think it would be nice to be consistent and update dialog position when resizing browser window. Currently it works fine with native 'flex` css option, but if you move dialog it will become static.

Also, I'm wondering if we shouldn't recalculate dialog position for newly opened dialog after window resize:

1. Open dialog in a small browser window.
2. Move it a little bit to mark it as moved.
3. Close dialog.
4. Maximaze browser window.
5. Open dialog again.

Expected

Dialog should be positioned at the center of the editor.

Actual

Dialog is placed at the same position before window resize.

Also, there is an issue on IE < 11:

1. Open manual test.
2. Click the link button.
3. Click the close button.
4. Repeat.

Expected

Dialog is showed up again.

Actual

Viewport is grey out. Dialog doesn't appear on second button click.

[engineering-this]

• Changed base to major.
• Changed dialog positioning from pixel values to % of parent.
• Fixed issues with IE.
• Removed resize listener on window.

Dialog still keeps its position when closing and opening again or resizing, but values are now in percentages, so resizing moves a bit dialog, keeping proportions.

[jacekbogdanski]

I'm not sure if dropping resize event completely is a good way. Currently, the dialog is not fully responsive which is mostly visible on IE where it's not initially handled by flex. If you start shrinking window you will see that at some point left margin starts to grow bigger than left.

[f1ames]

Rebased onto latest major.

[f1ames]

I see this PR also fixes #453 🎉

[f1ames]

LGTM 👍 Good job 👏