Problem/Motivation

The 'Alternative text' required error in the EditorImageDialog includes an <em> tag that is not closed. This results in the rest of the form being wrapped in the <em> tag. Italics everwhere.

This bug was introduced in #2307647: [Follow-up] Allow manual override of required image alt text in the Text Editor image dialog when appropriate

To reproduce:

  1. Open the image dialog from the CKEditor
  2. Leave the 'Alternative text' field empty

Proposed resolution

@AlexPott #3
The is odd here. is for emphasis but we're putting the text in brackets () to visually de-emphasise it... how about we just remove the .

Remaining tasks

  • Write a patch.
  • Add screenshots to show the effect of the patch.
  • Review the patch.

User interface changes

The EditorImageDialog form keeps its original font style when the Alternative text is marked as required with a form error.

API changes

None

Screenshot before

/files/issues/editorimagedialog-before.png

Screenshot after

CommentFileSizeAuthor
#4 2398925-4-after.png138.86 KBidebr
#4 2398925-4.patch1021 bytesidebr
#1 editor-imagedialog_required_markup-2398925-1.patch1 KBidebr
#1 editorimagedialog-after.png130.78 KBidebr
#1 editorimagedialog-before.png131.93 KBidebr

Comments

idebr’s picture

idebr commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new editorimagedialog-before.png131.93 KB
new editorimagedialog-after.png130.78 KB
new editor-imagedialog_required_markup-2398925-1.patch1 KB

Screenshot before:
editorimagedialog before

Screenshot after:
editorimagedialog before

aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs usability review

The <em> is odd here. <em> is for emphasis but we're putting the text in brackets () to visually de-emphasise it... how about we just remove the <em>.

idebr’s picture

idebr commented
Issue summary: View changes
StatusFileSize
new 2398925-4.patch1021 bytes
new 2398925-4-after.png138.86 KB

I agree the <em> tag is semantically incorrect, just the parentheses should do. I have updated the patch and issue summary accordingly.

Bojhan’s picture

Bojhan commented
Issue tags: -Needs usability review

Alexpott is correct here. I dont really see much point in demphasising this either. This error message is also written a little weird (rare cases?), but thats probally another issue.

idebr’s picture

idebr commented

@Bojhan Would the error be better on a single line without the parentheses? I'm looking for a way forward, since there is a bug in the markup but it current cannot be resolved because we are unable to decide on the contents of the markup :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

#4 is exactly what Alex wanted, and what Bojhan +1'd, so RTBCing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2398925-4.patch, failed testing.

idebr queued 4: 2398925-4.patch for re-testing.

idebr’s picture

idebr commented
Status: Needs work » Reviewed & tested by the community

Settings back to RTBC per #7

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed d70f9dd and pushed to 8.0.x. Thanks!

  • alexpott committed d70f9dd on 8.0.x 
    Issue #2398925 by idebr: Editor Image dialog: alt text required_error...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.