Opened 7 years ago

Closed 6 years ago

#5446 closed Bug (fixed)

Setting config.filebrowserImageBrowseUrl results in displaying also Browser Server on links

Reported by: MagicDude4Eva Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version:
Keywords: Cc:

Description (last modified by fredck)

I am using the latest nightly build and when setting config.filebrowserImageBrowseUrl the "Browse Server" button is displayed on image-info. The "Browse Server" button is also displayed in the Link-tab.

This should not happen. I also specifically set config.filebrowserImageBrowseLinkUrl='' hoping that this will remove the Browse Server on Link but this is not the case.

Attachments (4)

5446.patch (1.3 KB) - added by tobiasz.cudnik 6 years ago.
5446_2.patch (1.3 KB) - added by tobiasz.cudnik 6 years ago.
5446_3.patch (1.6 KB) - added by tobiasz.cudnik 6 years ago.
5446_4.patch (1.8 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by fredck

  • Milestone set to CKEditor 3.4

comment:2 Changed 6 years ago by tobiasz.cudnik

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

comment:3 Changed 6 years ago by fredck

  • Description modified (diff)
  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

Changed 6 years ago by tobiasz.cudnik

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

comment:5 Changed 6 years ago by tony

Ahem; don't you mean :

{
action : 'Browse', 
target: 'Link:txtUrl',
url: ( undefined !== editor.config.filebrowserImageBrowseLinkUrl ) ? editor.config.filebrowserImageBrowseLinkUrl : editor.config.filebrowserBrowseUrl
},
style : 'float:right', 
hidden : true,

Or even maybe :

{
action : 'Browse', 
target: 'Link:txtUrl',
url: ( undefined !== editor.config.filebrowserImageBrowseLinkUrl ) ? editor.config.filebrowserImageBrowseLinkUrl : editor.config.filebrowserBrowseUrl || editor.config.filebrowserImageBrowseUrl
},
style : 'float:right', 
hidden : true,

tobiasz?

Seeing how you'd would generally want to link an image to a file instead of just an image of itself or an entirely different image. _most_ file browsers will default to showing all files using editor.config.filebrowserBrowseUrl and only images when editor.config.filebrowserImageBrowseUrl is accessed by the browser. At least that's the way my implementation works.

Otherwise the patch seems OK without having tested it.

comment:6 Changed 6 years ago by tony

Yeah. Just tested this. I am confirming myself and asking for someone to test and approve my modification to tobiasz's patch because tobiasz's patch removes function original. tobiasz, the first part of your patch against _source/plugins/image/dialogs/image.js should be :

{
action : 'Browse', 
target: 'Link:txtUrl',
url: ( undefined !== editor.config.filebrowserImageBrowseLinkUrl ) ? editor.config.filebrowserImageBrowseLinkUrl : editor.config.filebrowserBrowseUrl || editor.config.filebrowserImageBrowseUrl
},
style : 'float:right', 
hidden : true,

The second part against _source/plugins/filebrowser/plugin.js :

if ( element.filebrowser.action == 'Browse' ) 
{
var url = ( undefined !== element.filebrowser.url ) 
? element.filebrowser.url : editor.config[ 'filebrowser' + ucFirst( dialogName ) + 'BrowseUrl' ] 
|| editor.config.filebrowserBrowseUrl;  
if ( url ) 

Is fine.

comment:7 Changed 6 years ago by fredck

  • Status changed from review to review_failed

I'm with tony on this. We don't have to change the feature but simply have the undefined check.

Changed 6 years ago by tobiasz.cudnik

comment:8 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Second patch fixes first one's property bug and includes Tony's proposition about filebrowser url inheritance (although in reversed priority order).

comment:9 Changed 6 years ago by tony

Having looked a bit deeper, here's what I think _should_ be happening instead :

If the user says that : editor.config.filebrowserImageBrowseLinkUrl === false No link button should appear. Same for if : editor.config.filebrowserSOME_PLUGIN_BrowseLinkUrl === false

If editor.config.filebrowserImageBrowseLinkUrl or editor.config.filebrowserSOME_PLUGIN_BrowseLinkUrl is undefined and not exactly false then the link button should show and open to editor.config.filebrowserBrowseUrl when clicked.

Also, for the second part of tobiasz's patch, the QuickUpload action isn't being patched like this too. Should it be? Thanks, Tony

comment:10 Changed 6 years ago by Saare

  • Status changed from review to review_failed

QuickUpload should also be patched. I also tend to agree with @tony.

Changed 6 years ago by tobiasz.cudnik

comment:11 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

This is how things work with latest patch, using Image dialog as example:

Image Info tab uses always:

config.filebrowserImageBrowseUrl

Link tab uses following:

config.filebrowserImageBrowseLinkUrl
config.filebrowserImageBrowseUrl

If first one casts to false (we don't need the strict comparison), then Browse Server button is not visible. If it's undefined, used is the second one.

Fix for Quick Upload is now also included.

comment:12 Changed 6 years ago by wwalc

  • Status changed from review to review_failed

Just to remind:

  • filebrowserBrowseUrl enables all "Browse Server" buttons.
  • filebrowserUploadUrl enables all "Quick Upload" tabs.

Let's say that both configuration options are used to control the global filebrowser configuration.

What we really need to do here, is to guarantee that when such a global setting is present, user still must have a way to disable/modify each particular place where filebrowser is enabled (wherever possible - at the level of dialogs and/or where filebrowser.url is specified in the element).

The situation at this moment is that it is possible to modify each particular url assigned to the "Browse Server" button:

// make the url in the flash dialog different than filebrowserBrowseUrl
filebrowserFlashBrowseUrl : 'flashbrowse.php'

however, it is impossible to disable this single button with something like:

// hide the "Browse Server" button in the Flash dialog - doesn't work at this moment
filebrowserFlashBrowseUrl : false

Same applies to QuickUpload tabs.

We can change the target url in the Flash dialog with:

// make it different than filebrowserUploadUrl
filebrowserFlashUploadUrl : 'flashupload.php'

but we cannot hide specific upload tab with:

filebrowserFlashUploadUrl : false

I'm setting it to review_failed mainly because the change included in the patch regarding QuickUpload part doesn't make too much sense to me and I believe we could have a more generic solution here, just like I explained above. Apart from adding support to disable "Browse Server" in the "Link" tab of the "Image" dialog with filebrowserImageBrowseLinkUrl, we should be able to disable the "Browse Server" button and/or the Upload tab in selected dialog.

In other words, what we need to add is:

  • if editor.config[ 'filebrowser' + ucFirst( dialogName ) + 'BrowseUrl' ] is defined and is set to false, buttons in this dialog should be hidden (...but still config.filebrowserImageBrowseLinkUrl could enable such button back in the Image dialog if config.filebrowserImageBrowseUrl was set to false)
  • if editor.config[ 'filebrowser' + ucFirst( dialogName ) + 'UploadUrl' ] is defined and set to false, the tab should be hidden in this dialog.

Changed 6 years ago by tobiasz.cudnik

comment:13 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

You're right Wiktor, the patch was missing second undefined check, so the inheritance chain is now as follows(for the Image dialog):

config.filebrowserImageBrowseLinkUrl
config.filebrowserImageBrowseUrl
config.filebrowserBrowseUrl

Inheritance is executed only for undefined values.

comment:14 Changed 6 years ago by wwalc

  • Status changed from review to review_passed

comment:15 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [5988].

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