Ticket #7991 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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

Reported by: j.swiderski 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

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

Change History

comment:1 Changed 4 years ago by j.swiderski

  • Status changed from new to confirmed

comment:2 Changed 4 years ago by wwalc

  • Milestone set to CKEditor 3.6.1

Changed 4 years ago by garry.yao

comment:3 Changed 4 years ago by garry.yao

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

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

comment:4 Changed 4 years ago by Saare

  • Status changed from review to review_passed

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

comment:5 Changed 4 years ago by wwalc

  • Status changed from review_passed to review_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 4 years ago by garry.yao

comment:6 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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

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

comment:9 in reply to: ↑ 8 Changed 4 years ago by Saare

  • Status changed from review to review_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 follow-up: ↓ 12 Changed 4 years ago by alfonsoml

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

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 4 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 4 years ago by garry.yao

comment:13 Changed 4 years ago by garry.yao

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

comment:14 Changed 4 years ago by wwalc

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 4 years ago by garry.yao

comment:15 Changed 4 years ago by garry.yao

  • Status changed from review_passed to review

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 4 years ago by garry.yao

illustration of the dialog field width definition

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

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 4 years ago by wwalc (previous) (diff)

Changed 4 years ago by wwalc

Changed 4 years ago by garry.yao

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

comment:19 Changed 4 years ago by alfonsoml

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

comment:20 Changed 4 years ago by wwalc

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

  • Status changed from review to review_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 4 years ago by garry.yao

  • Status changed from review_passed to closed
  • Component changed from General to UI : Dialogs
  • Resolution set to fixed

Fixed with [7029].

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