Opened 8 years ago

Closed 7 years ago

#5308 closed Bug (fixed)

File browser window doesn't scroll

Reported by: Ben Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4.1
Component: General Version: 3.2
Keywords: HasPatch Cc: joel@…

Description

I am using my own custom file browser with CKEditor 3.2. When I view the file browser in a normal browser window it scrolls fine.

However, when I click the Browse Server button and it opens the file browser in a popup, the scroll bars don't work...

Attachments (4)

5308.patch (641 bytes) - added by Charlie 7 years ago.
simply added scrollbars
5308_2.patch (596 bytes) - added by Charlie 7 years ago.
Updated as per alfonsoml's proposal
5308_3.patch (686 bytes) - added by Charlie 7 years ago.
Fixed as per fred's description :-P
5308_4.patch (2.4 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by Jakob

We're also running into problems with this, so I added a

scrollbars=yes,

to the params in /_source/plugins/popup/plugin.js in line 36. I hope this will get fixed with the next release. Of course we could add a scrollable div around our custom filebrowser like suggested in the board but since our filebrowser is not only used via CKEditor I actually don't like to put CKEditor-specific changes into our filebrowser code.

comment:2 Changed 8 years ago by Wiktor Walc

Milestone: CKEditor 3.4

It looks like we need to add scrollbars=yes or make it configurable.

Another post where this feature was requested: http://cksource.com/forums/viewtopic.php?f=11&t=15966

comment:3 Changed 7 years ago by Joel Hillacre

Cc: joel@… added

comment:4 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

Changed 7 years ago by Charlie

Attachment: 5308.patch added

simply added scrollbars

comment:5 Changed 7 years ago by Charlie

Keywords: Review? added

comment:6 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? removed

The features for the window.open call should be configurable.

Also, please note that the bug is currently assigned to Tobiasz.

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Not a problem to have others contributing patches, really.

In any case, couldn't we have it in a way that the opened page itself defines whether to have scrollbars or not?

comment:8 Changed 7 years ago by Charlie

Sorry, do you mean the caller defines whether or not to have them? Or the page in the opened window?

I believe that as is, adding scrollbars=yes only adds the bars if they are needed (I.E. the page's content is larger than the viewable area). So in a sense, the page which is opened does determine if they are needed using this method.

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

Well, the caller is the editor itself, and it could be cumbersome to have a configuration just for that.

Instead, we could leave it flexible, so the opened page (the page in the opened window) defines whether or not scrollbars are necessary (body overflow set to auto/scroll).

comment:10 Changed 7 years ago by Charlie

I mean while this is true. I think that borders on unhelpful. For people who don't want to modify their pages (As per original request), it does nothing.

In addition, people (such as myself) migrating from FCKEditor will find this behavior odd, since FCK opened with the scrollbars.

IMHO, it's better to do the opposite, since this will maximise compatibility. Always show the toolbar and if people don't want it, they can make their website smaller (I.E. fit to the window size they open).

comment:11 Changed 7 years ago by Alfonso Martínez de Lizarrondo

My proposal would be as follows:

First, remove all the "feature=no" entries. As defined by window.open, if a feature isn't present in the request it defaults to "no".

Second, remove the dependent, modal and alwaysRaised requests because AFAIK, they don't do anything in a normal web page. Some of them used to work in old Firefox versions, but now they don't seem to do anything.

Lastly provide the option to specify the requested features:

options = (editor.config.fileBrowserWindowFeatures || 'resizable=yes') +

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added

That's a good proposal Alfonso. Let's go ahead with it.

Changed 7 years ago by Charlie

Attachment: 5308_2.patch added

Updated as per alfonsoml's proposal

comment:13 Changed 7 years ago by Charlie

I did it again. I figured while I'm putting this in my code I might as well put the patch here. This is just alfonsoml's proposal. Except I kept scrollbars=yes because I feel very strongly for the reasons I listed above that this is the best default setting.

It should not affect anyone who is not currently complaining about it. In addition, for those who are complaining, it will fix their problem without any setting changes. The extreme minority are those who display websites bigger than the popup size and do NOT want scrollbars. That is the only group who should have to utilize this setting.

Hope that all makes sense!

comment:14 Changed 7 years ago by Charlie

Keywords: HasPatch added

comment:15 in reply to:  11 ; Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

Replying to alfonsoml:

First, remove all the "feature=no" entries. As defined by window.open, if a feature isn't present in the request it defaults to "no".

Looking at the [MSDN docs http://msdn.microsoft.com/en-us/library/ms536651%28VS.85%29.aspx], that's not true. Some things default to "yes".

For backwards compatibility, I would not touch the current options, eventually removing the old FF stuff, adding the new configuration option.

Changed 7 years ago by Charlie

Attachment: 5308_3.patch added

Fixed as per fred's description :-P

comment:16 in reply to:  15 Changed 7 years ago by Charlie

Is there any reason a change this small can't make it into the 3.4 release?

comment:17 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.5CKEditor 3.4
Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben

We're working to close the 3.4 right now, so we should not continue adding things to it. Even small tickets take time, because they involve discussion, coding, review, commit, etc.

Also, we still didn't have the final patch up to 40 minutes ago ;) Now things change :)

comment:18 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

comment:19 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Confirmed removed
Status: reviewreview_passed

Just fix the coding style and add documentation for the new configuration before commiting.

comment:20 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5
Owner: Frederico Caldeira Knabben deleted
Status: review_passednew
Summary: File browser window doesn't scroll...File browser window doesn't scroll

It looks like no one tested that thing... neither comp615, nor me, nor Saare... where is the "editor" variable being coming from? What a shame guys :P

We're really closing the 3.4 this time.

comment:21 Changed 7 years ago by Sa'ar Zac Elias

Owner: set to Sa'ar Zac Elias
Status: newassigned

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5308_4.patch added

comment:22 Changed 7 years ago by Sa'ar Zac Elias

Status: assignedreview

comment:23 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_passed

comment:24 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [5830].

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