Opened 13 years ago

Closed 13 years ago

#7991 closed Bug (fixed)

Labels don't fit the Table Properties dialog in some languages.

Reported by: Jakub Ś Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: UI : Dialogs Version: 3.6.1
Keywords: Cc:

Description

In [6979] unit select list has been removed. Now ,for some languages, the labels describing cell padding and cell spacing don’t fit the dialog.

These languages are: Vietnamese, Russian, Lithuanian, Georgian, French, Faroese, Dutch, Czech, Catalan, Bulgarian, Mongolian, Polish

In Firefox and Opera labels go out of dialog. In IE9 part of label is hidden (like overflow:hidden) In Webkit and IE8 scroll bars appear.

In IE6 and IE7 dialog is enlarged to fit the label. In other words it looks nice here.

Attachments (9)

7991.patch (2.1 KB) - added by Garry Yao 13 years ago.
7991_2.patch (5.1 KB) - added by Garry Yao 13 years ago.
7991_3.patch (9.8 KB) - added by Garry Yao 13 years ago.
7991_4.patch (10.9 KB) - added by Garry Yao 13 years ago.
7991.png (24.0 KB) - added by Garry Yao 13 years ago.
illustration of the dialog field width definition
7991_styling.png (5.9 KB) - added by Wiktor Walc 13 years ago.
7991_5.patch (11.1 KB) - added by Garry Yao 13 years ago.
test_dialog_fields.js (1.2 KB) - added by Garry Yao 13 years ago.
7991_apidocs.patch (6.1 KB) - added by Wiktor Walc 13 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 13 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 13 years ago by Wiktor Walc

Milestone: CKEditor 3.6.1

Changed 13 years ago by Garry Yao

Attachment: 7991.patch added

comment:3 Changed 13 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

Can't we just resigns all dimension restrictions over the table dialog, which makes l11n just easier and also looks neater.

comment:4 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

Looks good to me, if anyone sees a problem please reassign the status of the ticket.

comment:5 Changed 13 years ago by Wiktor Walc

Status: review_passedreview_failed

In my opinion the proposed solution is not right if we think of user experience.

We should keep input fields short where we don't expect large numbers.

Changed 13 years ago by Garry Yao

Attachment: 7991_2.patch added

comment:6 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:7 Changed 13 years ago by Garry Yao

Right now we had a limitation in the dialog layout that the specified field width is bound with label size, which means short field won't allow a long label fill to stretch the dialog. E.g. having the following definition:

 type : 'text',
 id : 'txtCellSpace',
 style : 'width:3em'

Constraint both the label and input to "3em" width.

Propose a new "inputStyle" definition entry which makes it possible to have fine-grid control over style on both input element and the label.

comment:8 Changed 13 years ago by Alfonso Martínez de Lizarrondo

There's already a "controlStyle" option: http://dev.ckeditor.com/ticket/5927

comment:9 in reply to:  8 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

Replying to alfonsoml:

There's already a "controlStyle" option: http://dev.ckeditor.com/ticket/5927

That's different, as controlStyle is applied to the input itself while inputStyle is applied to the surrounding <div>. Though we really are missing the docs for controlStyle.

comment:10 Changed 13 years ago by Alfonso Martínez de Lizarrondo

It's gonna be fun to have "inputStyle" applied to a div and "controlStyle" applied to an input.

Are we sure that we want to use those names?
Once it's released we'll be again on the old game "yeah, that's bad, but changing it would break external code. We might change it in the next big rewrite 2 years from now"

comment:11 Changed 13 years ago by Wiktor Walc

Indeed it might be quite confusing... The patch is good, but using better names for options right from the beginning also sounds reasonable. Alfonso, do you have any suggestions?

comment:12 in reply to:  10 Changed 13 years ago by Garry Yao

Replying to alfonsoml:

It's gonna be fun to have "inputStyle" applied to a div and "controlStyle" applied to an input.

As you proposed "controlStyle" back in #5927 to introduce an easy way of styling the control element separately from label, exactly of the same purpose with me here (but I didn't know that option and it turns out to have only one usage in the codebase).

I propose to deprecate the "controlStyle" in favor of "inputStyle" for the following reasons:

  1. The style should goes on the input wrapper (e.g. cke_dialog_ui_input_text) instead of the input element itself, as the wrapper carries part of the input style.
  2. IMO "inputStyle" get also better adopted to the current code as we already have something like "getInputElement".

Changed 13 years ago by Garry Yao

Attachment: 7991_3.patch added

comment:13 Changed 13 years ago by Garry Yao

@Saar, can you review the changes to the docProps dialog if it breaks anything?

comment:14 Changed 13 years ago by Wiktor Walc

Using the latest patch, page margins in the "Design" tab in the docProps dialog are no longer centered (to reproduce just type something in the "Top" field).

Changed 13 years ago by Garry Yao

Attachment: 7991_4.patch added

comment:15 Changed 13 years ago by Garry Yao

Status: review_passedreview

I didn't realize the input field content should also be center aligned...but that would be quite a rare case to be considered.

Changed 13 years ago by Garry Yao

Attachment: 7991.png added

illustration of the dialog field width definition

comment:16 Changed 13 years ago by Garry Yao

Just to better illustrate the actual effects of those definitions: illustration of the dialog field width definition

comment:17 Changed 13 years ago by Wiktor Walc

DocProps dialog: actually I do not see any reason why

this.getElement().getParent().setStyle( 'text-align', 'center' ); 

was used there. Am I wrong or controlStyle : 'text-align: center' was enough to keep the text aligned to the center?

Regarding controlStyle / labelStyle / inputStyle (I'll use current names in this post just to avoid confusion). In my opinion they are all useful. Thanks to the controlStyle I believe we can get rid of

 function textCenterAlign() { this.getInputElement().setStyle( 'text-align', 'center' ); } 

used in the last patch. Being able to style the element perfectly using just the definition I believe is something that Alfonso tried to achieve with #5927.

Alfonso suggested using better names, although I agree that we should not use confusing names, I have no idea what names would be better :|.

One more thing regarding the functionality, I've just made a quick test and noticed some inconsistency (using the 2# patch if I'm correct).

{
	type: 'text',
	id : 'foo1',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	default : 'initial value',
	labelStyle : 'color:red'
},
{
	type: 'textarea',
	id : 'foo2',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	default : 'initial value',
	labelStyle : 'color:red'
},
{
	type: 'select',
	id : 'foo3',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	labelStyle : 'color:red',
	items : [ ['foo'] ]
},
{
	type: 'checkbox',
	id : 'foo4',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	labelStyle : 'color:red'
},
{
	type: 'radio',
	id : 'foo5',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	labelStyle : 'color:red',
	items : [ ['foo'] ]
},
{
	type: 'html',
	id : 'foo6',
	style : 'width:200px',
	html : 'foo html',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue'
},
{
	id : 'foo7',
	type: 'file',
	style : 'width:200px',
	label : 'label',
	inputStyle : 'border:5px solid red',
	controlStyle : 'color:blue',
	labelStyle : 'color:red'
}
Last edited 13 years ago by Wiktor Walc (previous) (diff)

Changed 13 years ago by Wiktor Walc

Attachment: 7991_styling.png added

Changed 13 years ago by Garry Yao

Attachment: 7991_5.patch added

comment:18 Changed 13 years ago by Garry Yao

Following Wiktor's review, switch the names between 'inputStyle' and 'controlStyle' for better semantics, fixing the above mentioned bugs.

Changed 13 years ago by Garry Yao

Attachment: test_dialog_fields.js added

comment:19 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Another option (just in case you want another idea, but I think that the latest patch is quite better to keep everyone's sanity than the original proposal) would be to call the style for the wrapper div inputWrapperStyle instead of controlStyle.

When the dialog system is reviewed, it should be checked if such div wrapper is really needed or it can be removed, less elements, less things to think about.

Changed 13 years ago by Wiktor Walc

Attachment: 7991_apidocs.patch added

comment:20 Changed 13 years ago by Wiktor Walc

I've noticed that in fact we're missing the documentation for CKEDITOR.dialog.definition.labeledElement. The attached patch contains changes from Garry's patch to dialogDefinition.js along with some further improvements.

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

Status: reviewreview_passed

The code pieces in the DocProps plugin are there to align the fields in Quirks mode, thus it must remain untoucned.
It's otherwise R+, so I'd opt to commit the patch without these changes (let's also add a comment there to remind what's the purpose of this code piece).

comment:22 Changed 13 years ago by Garry Yao

Component: GeneralUI : Dialogs
Resolution: fixed
Status: review_passedclosed

Fixed with [7029].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy