Image2: dialog is not integrated with ACF

[Piotrek Koszuliński]

Fields like hasCaption and alignment should have requiredContent defined.

[Olek Nowodziński]

​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.

[Piotrek Koszuliński]

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.

[Olek Nowodziński]

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.

[Olek Nowodziński]

Related ticket: #11199.

[Piotrek Koszuliński]

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).

[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.

[Olek Nowodziński]

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.

2. 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.

3. Alignment fields in dialog (left, right, etc.) should be displayed conditionally according to ACF rules.

[Piotrek Koszuliński]

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.

[Olek Nowodziński]

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

[Piotrek Koszuliński]

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.

[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.

[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.

[Olek Nowodziński]

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.

[Piotrek Koszuliński]

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.

[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.

[Piotrek Koszuliński]

Pushed commit to dev.

[Olek Nowodziński]

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