Opened 5 years ago

Closed 4 years ago

#7946 closed Bug (fixed)

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

Reported by: satya Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: General Version: 3.4
Keywords: IBM Cc: damo, jamescun, tmonahan

Description

To reproduce the defect:

  1. Open AutoGrow Plugin sample and enter some content in editor which spans few pages.
  1. 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.

Attachments (10)

7946.patch (2.4 KB) - added by tmonahan 5 years ago.
Potential fix for this issue
7946_2.patch (1.9 KB) - added by garry.yao 5 years ago.
7946_3.patch (1.4 KB) - added by garry.yao 4 years ago.
7946_4.patch (6.3 KB) - added by garry.yao 4 years ago.
7946_5.patch (6.6 KB) - added by garry.yao 4 years ago.
7956_5_Current.png (182.6 KB) - added by fredck 4 years ago.
Current results with 7956_5.patch.
7956_Expected.png (188.0 KB) - added by fredck 4 years ago.
Expected results.
7946_Illustration.png (19.4 KB) - added by fredck 4 years ago.
Illustration showing the basics of the nested scrolling.
7946_6.patch (4.4 KB) - added by garry.yao 4 years ago.
7946_7.patch (5.6 KB) - added by garry.yao 4 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 5 years ago by j.swiderski

  • Status changed from new to confirmed
  • Version changed from 3.6.1 (SVN - trunk) to 3.4

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

comment:2 Changed 5 years ago by j.swiderski

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.

comment:3 Changed 5 years ago by tmonahan

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?

Changed 5 years ago by tmonahan

Potential fix for this issue

Changed 5 years ago by garry.yao

comment:4 follow-up: Changed 5 years ago by 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.

comment:5 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

comment:6 in reply to: ↑ 4 Changed 4 years ago by fredck

  • Status changed from review to review_failed

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 :)).

Changed 4 years ago by garry.yao

comment:7 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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

comment:8 Changed 4 years ago by 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?

comment:9 Changed 4 years ago by fredck

  • Status changed from review to review_failed

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

Changed 4 years ago by garry.yao

comment:10 follow-up: Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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)

comment:11 Changed 4 years ago by fredck

  • Status changed from review to review_failed

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.

comment:12 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

Two new tcs to cover the scrolling behavior on enterkey:

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

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

Changed 4 years ago by garry.yao

Changed 4 years ago by fredck

Current results with 7956_5.patch.

Changed 4 years ago by fredck

Expected results.

comment:13 Changed 4 years ago by fredck

  • Status changed from review to review_failed

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.

Changed 4 years ago by fredck

Illustration showing the basics of the nested scrolling.

comment:14 Changed 4 years ago by fredck

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).

comment:15 in reply to: ↑ 10 Changed 4 years ago by fredck

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.

comment:16 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

Changed 4 years ago by garry.yao

comment:18 Changed 4 years ago by 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

comment:19 Changed 4 years ago by fredck

  • Status changed from review to review_failed

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

Changed 4 years ago by garry.yao

comment:20 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:21 Changed 4 years ago by fredck

  • Milestone set to CKEditor 3.6.3
  • Status changed from review to review_passed

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

comment:22 Changed 4 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7399].

Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy