Opened 6 years ago

Closed 5 years ago

#11004 closed Bug (fixed)

Image2: dialog is not integrated with ACF

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version: 4.3 Beta
Keywords: Cc:

Description

Fields like hasCaption and alignment should have requiredContent defined.

Change History (21)

comment:1 Changed 6 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 6 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:3 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

t/11004 (+tests) contains:

  • RC definition for Alt. Text, Alignment and Has Caption fields.
  • RC definition for the widget itself.
  • Fixed: Don't set empty alt="" of <img> when no Alt. Text specified.
    • Aligned tests for that change.
  • Minor updates to requiredContent docs.

comment:4 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_failed

Fixed: Don't set empty alt="" of <img> when no Alt. Text specified.

Alt is required - HTML is invalid without it. So we have to add it, even if it's empty.

comment:5 in reply to:  4 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

Replying to Reinmar:

Fixed: Don't set empty alt="" of <img> when no Alt. Text specified.

Alt is required - HTML is invalid without it. So we have to add it, even if it's empty.

Discarded that commit and test branch.

comment:6 Changed 6 years ago by Olek Nowodziński

Related ticket: #11199.

comment:7 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  1. Set allowedContent to p; img[src] and open image2 sample.
  2. Double-click 1st image.
  3. Change src to image2.jpg.
  4. Save.
  5. Switch to source mode.
  • Expected: <p><img alt="" src="assets/image2.jpg" /></p>
  • Actual: <p><img alt="" height="480" src="assets/image2.jpg" width="534" /></p>

Please write tests for most important cases (at least two, three tcs: open dialog, check visible fields, save and check data).

comment:8 Changed 6 years ago by Piotrek Koszuliński

PS.

  1. By important I meant probable.
  2. Remember that only one of width, height may be allowed. It will be the easiest to handle that in #data event.
  3. Remember about resize handler - it should be disabled when width and height are not allowed.

comment:9 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

Created branch t/11004 in tests:

  1. Testing the visibility of dialog fields according to allowedContent of the editor.
  2. Testing data function to check if produced HTML is consistent with ACF rules of the editor.
  3. Testing the presence of widget resizer according to ACF rules of the editor.
  4. Testing justify plugin integration: justify command states according to ACF rules when widget is selected.

Pushed several commits to dev making all tests green.

Notes and remarks:

  1. We really need rich requiredContent, defined as a logical AND/OR of two or more elements with different set of attributes/classes/styles.

At the moment, requiredContent of hasCaption in Image2 dialog is figcaption. It's a very poor assertion since figure is also required to create a right captioned image.

Another example: requiredContent of align field is img[float]. But we need also figure[float] and div p{text-align}, don't we? There are so many cases where dialog layout would fail due to specific ACF rules.

  1. Regarding 1.: This more than a dialog-related thing. To produce consistent HTML, widgetDef#data should also follow requiredContent defined in dialog (and vice-versa). At the moment, even if figcaption is allowed but figure isn't, the hasCaption field will be displayed in the dialog and widgetDef#data will inject <figure> into editable. The same <figure> will be also in editor#getData output. It's bad, really.
  1. Alignment fields in dialog (left, right, etc.) should be displayed conditionally according to ACF rules.

comment:10 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.1CKEditor 4.3.2
Status: reviewreview_failed

I realised that by hardcoding allowed and required content strings we're making impossible to use other data formats by overwriting upcast and downcast methods. Let's rethink this ticket then.

My idea is that we can extract those rules to overwritable object. For example widget.definition could have a features property with:

features: {
    alignedImage: {
        requiredContent: '...',
        allowedContent: '...'
    },
    captionedImage: {
        requiredContent: '...',
        allowedContent: '...'
    }
}

All rules needed for dialog and internal logic could be stored this way what would make the extension possible.

comment:11 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.2CKEditor 4.3.3

comment:12 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

Pushed updated solution to t/11004b (+tests).

comment:13 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

About dev changes:

  1. allowedContent in input definitions does nothing. What's funny - you didn't even defined allowedContent in image2.features :D.
  2. requiredContent of entire widget should contain img[!src,alt].
  3. Since alt is a required attribute in HTML, there's no need to define rules for it - its input should be always displayed and this attr should be always set.
  4. Result of check( features.align.requiredContent ) should be cached in alignCommandIntegrator because commands may be refreshed pretty often. Although, this function is executed before ACF rules are ready, so the result has to be cached after first check.

comment:14 Changed 5 years ago by Piotrek Koszuliński

Tests:

  1. Automated tests look well. You'll need of course to update them after changes related to alt attribute.
  2. Manual tests went gr8 too. I only noticed that it's not possible to allow only width, without allowing height, but I understand that it would require a lot of work to separate them.

comment:15 Changed 5 years ago by Piotrek Koszuliński

Maybe one more thing - would it be possible to separate centering image2 from other alignments? It's rather easy to define acf for right/left/none, but some developers may not like centering.

comment:16 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

Just updated dev and test branches.

I only noticed that it's not possible to allow only width, without allowing height, but I understand that it would require a lot of work to separate them.

That's true. It's a mess.

Maybe one more thing - would it be possible to separate centering image2 from other alignments? It's rather easy to define acf for right/left/none, but some developers may not like centering.

I started coding this feature and, though pretty and easy it is for a dialog, it automatically affects integration with Justify and dozens of other things. Once I had realized that it's a lot of work (and even more tests!) I basically abandoned the idea.

Manual tests required.

comment:17 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

As I thought, buttons are registered to ACF after alignCommandIntegrator call. However, because of #11567, which we'll fix in 4.4, the image2 widget is registered before that. Status of alignment commands won't be correctly recognized after we #11567 is closed, so better to have the check fixed now.

Although, this function is executed before ACF rules are ready, so the result has to be cached after first check.

comment:18 Changed 5 years ago by Piotrek Koszuliński

Except comment:17 and:

/www/cksource/ckeditor-dev/plugins/image2/plugin.js(145): warning: trailing comma is not legal in ECMA-262 object initializers
				},
................................^

everything looks ok.

comment:19 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

comment:20 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Pushed commit to dev.

comment:21 Changed 5 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

git:24f7406 landed in master (ba12e9f tests).

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