Editor doesn't show all the matched text during find with Autogrow plugin loaded

[Satya Minnekanti]

To reproduce the defect:

1. Open AutoGrow Plugin sample and enter some content in editor which spans few pages.

2. Click on Find icon and enter a word(that was present few times in the content)in Find What field and click on Find button.

Expected Result: Editor will highlight & show all the words that matched the word entered in Find What field.

Actual Result: Editor will highlight & show only the words that are present in paragraphs which are displayed in max height set for editor body.

[Jakub Ś]

This issue may be a duplicate of #7429. If so please close this issue.

If max height is not set then only the words in current viewport are shown. All the words are found, but browser is not scrolled up or down to show them.

If max height is set then behavior is the same as described in #7429

[Jakub Ś]

The second part of my comment shows that this bug is a DUP of #7429

The first part (with max height not set) touches different problem with autogrow plug-in but it is described in more detail here #7955.

Propose to close this one.

[Teresa Monahan]

This problem is dependent on the height of the editor.

When the editor's height is smaller than the browser window and the scroll bar is present (as in the case of #7429 and this ticket when max-height is set to a small value) then the problem is due to the offset value being reset to zero in the scrollIntoView function in core/dom/element.js - see my comment on #7429 for a proposed fix for this.

However when the editor's height is greater than the browser window, I believe this is an entirely different issue:
scrollIntoView relies solely on the scroll bar of the editor to determine whether the matched text is outside the current viewport of the editor and to scroll the editor to display the found results if required. When the editor's height is larger than the browser window, this is not sufficient since scrolling the text into view in the editor does not necessarily mean the content will be displayed in the current browser viewport. Therefore scrollIntoView also needs to take the browser scroll bar and viewport into account.

While this ticket was originally logged against the autogrow feature, it is also reproducible without autogrow.

Steps to Reproduce:

1. Set config.height to a large value e.g. config.height = '1500px';
2. Enter many lines of text but make sure that the editor scroll bar does not appear.
3. Search for some content in the text.

Problem: The editor does not scroll the matched results into view.

If you add some further lines of text to the editor so that the editor scroll bar does appear and do the search again, you will see that the editor scroll bar does scroll as you cycle through the search results. However the matched content is not displayed in the browser viewport since the editor is larger than the browser window.

This is a high priority issue for us. Is there any chance this could be fixed in the 3.6.3 release?

[Teresa Monahan]

Potential fix for this issue

[Garry Yao]

The bug with the current scrollIntoView implementation is the missing of applying the scrolling recursively, up to the top window where tmonahan tried to patch.

I dont understand the rational behind self-made scrollIntoView which is already a cross-browser and mature enough function plus the fact the amount of code required for such a custom implementation.

[Frederico Caldeira Knabben]

Replying to garry.yao:

I dont understand the rational behind self-made scrollIntoView which is already a cross-browser and mature enough function plus the fact the amount of code required for such a custom implementation.

One of the strong points in CKEditor is that it is almost 10 years old with thousand of tickets fixed. Because of this, we should never assume that code is there just by case. There is always a reason why (Am I really saying thing again? :)).

The last patch regresses #1462 and #2279 (and probably others :)).

[Garry Yao]

Ok, I go the point now, it's always nice to have you explained the history, so we'd instead patch the current impl.

[Garry Yao]

Ticket test added with: ​http://ckeditor.t/tt/7946/1.html

Now we need to understand whether we need to address the potentially various layout of the host page which might have other scrollable ancestor?

[Frederico Caldeira Knabben]

The "Editor inside of a scrolled container" ​test is failing for me with both FF and IE9.

[Garry Yao]

Brand new impl which takes care of ancestor overflow as well as framed content, it also correct the "align to top" behavior in certain scenarios.

The scrollIntoParent is now used for wysiwyg only scroll (e.g. enter key) while scrollIntoView is for host page scroll (e.g. Find dialog)

[Frederico Caldeira Knabben]

With FF:

1. Load replacebyclass.
2. Put the caret at the end of the text.
3. Hit ENTER many times, so the selection will move out of the editing area.

The document will not scroll to show the caret blinking.

[Garry Yao]

Two new tcs to cover the scrolling behavior on enterkey:

​http://ckeditor.t/tt/7946/2.html

​http://ckeditor.t/tt/7946/3.html

[Frederico Caldeira Knabben]

Current results with 7956_5.patch.

[Frederico Caldeira Knabben]

Expected results.

[Frederico Caldeira Knabben]

Unfortunately the results with have with the last patch still don't match the expectation. This could be confirmed with ​http://ckeditor.t/tt/7946/1.html, by making a small change to the element to be visible position, moving it in the middle of the text.

I've attached two screenshots that show the after patch and the expected results in that case.

[Frederico Caldeira Knabben]

Illustration showing the basics of the nested scrolling.

[Frederico Caldeira Knabben]

I've attached a tentative illustration that shows the theoretical steps we should pass through when scrolling. I'm not sure it matches all the possible cases, but it should be ok.

The idea is that all the frames hierarchy must scroll as little as possible to make the element visible at the very bottom of the top most frame (or at the top, if the element is at the top).

[Frederico Caldeira Knabben]

Replying to garry.yao:

The scrollIntoParent is now used for wysiwyg only scroll (e.g. enter key) while scrollIntoView is for host page scroll (e.g. Find dialog)

Why making this separation instead of always enforcing the scrollIntoView behavior? Is its performance so bad? It looks like just a series of calculations with no DOM changes, so the impact should be little. Only if you have confirmed an important performance penalty.

[Garry Yao]

I've attached a tentative illustration that shows the theoretical steps we should pass

I agree with your points, the illustrated result is more optimized than current, the current impl is to match the ​spec while indeed when the alignment param is missing we may have this smartness you described here, and I believe it's the way how world's most find dialogs perform.

Why making this separation instead of always enforcing the scrollIntoView behavior?

Not a matter of performance, just because we have this alignTop by default behavior for now, we'll be centralized to a single method after then.

[Garry Yao]

Now the impl will not depend on getDocumentPosition, which makes it super fast ;)

Performance test for ff can be found at:

​http://ckeditor.t/tt/7946/4.html

[Frederico Caldeira Knabben]

The new patch appends a new implementation for scrollIntoView, while still keeping the old code.

[Frederico Caldeira Knabben]

Please have all tests passing (or ignored) in IE as well.

[Garry Yao]

Fixed with [7399].