Ticket #5308 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

File browser window doesn't scroll

Reported by: bensinclair1 Owned by: Saare
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

5308.patch (641 bytes) - added by comp615 4 years ago.
simply added scrollbars
5308_2.patch (596 bytes) - added by comp615 4 years ago.
Updated as per alfonsoml's proposal
5308_3.patch (686 bytes) - added by comp615 4 years ago.
Fixed as per fred's description :-P
5308_4.patch (2.4 KB) - added by Saare 4 years ago.

Change History

comment:1 Changed 4 years ago by jschroeter

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 4 years ago by wwalc

  • Milestone set to 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 4 years ago by jhillacre

  • Cc joel@… added

comment:4 Changed 4 years ago by tobiasz.cudnik

  • Status changed from new to assigned
  • Owner set to tobiasz.cudnik

Changed 4 years ago by comp615

simply added scrollbars

comment:5 Changed 4 years ago by comp615

  • Keywords Review? added

comment:6 Changed 4 years ago by alfonsoml

  • 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 4 years ago by fredck

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 4 years ago by comp615

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 4 years ago by fredck

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 4 years ago by comp615

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 follow-up: ↓ 15 Changed 4 years ago by alfonsoml

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 4 years ago by fredck

  • Keywords Confirmed added

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

Changed 4 years ago by comp615

Updated as per alfonsoml's proposal

comment:13 Changed 4 years ago by comp615

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 4 years ago by comp615

  • Keywords HasPatch added

comment:15 in reply to: ↑ 11 ; follow-up: ↓ 16 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.4 to CKEditor 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 4 years ago by comp615

Fixed as per fred's description :-P

comment:16 in reply to: ↑ 15 Changed 4 years ago by comp615

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

comment:17 Changed 4 years ago by fredck

  • Owner changed from tobiasz.cudnik to fredck
  • Milestone changed from CKEditor 3.5 to CKEditor 3.4

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 4 years ago by fredck

  • Status changed from assigned to review

comment:19 Changed 4 years ago by Saare

  • Keywords Confirmed removed
  • Status changed from review to review_passed

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

comment:20 Changed 4 years ago by fredck

  • Owner fredck deleted
  • Status changed from review_passed to new
  • Milestone changed from CKEditor 3.4 to CKEditor 3.5
  • Summary changed from File browser window doesn't scroll... to 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 4 years ago by Saare

  • Owner set to Saare
  • Status changed from new to assigned

Changed 4 years ago by Saare

comment:22 Changed 4 years ago by Saare

  • Status changed from assigned to review

comment:23 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

comment:24 Changed 4 years ago by Saare

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

Fixed with [5830].

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