In #2075949: Add Libricons to messages, we came to the conclusion that the dblog module should update icon usage to SVG, following the new practices for implementation (see #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core).

This module uses the file "message-16-error.png" at:

Line 1430 in core/modules/filter/filter.module
Line 87 in core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php

They should be properly replaced with SVG substitutes.

Test Steps

  1. Create a piece of content
  2. In the body field, turn on the html source view and paste in the following: <img src="http://google.com" />
  3. Save the content
CommentFileSizeAuthor
#24 2083949-filter-icons-24.patch2.21 KBLewisNyman
#19 2083949-svg-image-filter-19.patch1.39 KBAnonymous (not verified)
#18 2083949-svg-image-filter-18.patch1.92 KBLewisNyman
#11 icon-before.png14.55 KBrteijeiro
#11 icon-after.png14 KBrteijeiro
#9 2083949-filter-icons-9.patch1.67 KBemma.maria
#6 2083949-filter-icons-6.patch1.68 KBemma.maria
#4 2083949-filter-icons-4.patch842 bytesLewisNyman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented
Title: Update use of icons to new standards » filter.module: Update use of icons to new standards

...less vague title (there are at least 3 issues with the same name: #2075949-10: Add Libricons to messages).

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue tags: +styleguide, +styleguide-add-icons

tagging

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

This one is different to the other icon issues because it's an img tag. This is a standalone theme function, so I think we could use the “onerror” technique here http://dbushell.com/2013/02/04/a-primer-to-front-end-svg-hacking/

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Active » Needs review
FileSize
2083949-filter-icons-4.patch842 bytes

Status: Needs review » Needs work

The last submitted patch, 2083949-filter-icons-4.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
2083949-filter-icons-6.patch1.68 KB

I have applied the patch in #4 and amended the test file so it should pass now.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2083949-filter-icons-6.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
2083949-filter-icons-9.patch1.67 KB

Here is a patch that contains the correct path to the image.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs work » Needs review
rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs review » Reviewed & tested by the community
FileSize
icon-after.png14 KB
icon-before.png14.55 KB

Patch applies cleanly and code seems right. Now the svg icon is shown so it's a RTBC for me ;)

BEFORE

icon-before.png

AFTER

icon-after.png

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +JavaScript

I'd like sign-off on this from nod_ as well, because of the technique being used to fall back to PNG files that's being used (as suggested in #3).

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Reviewed & tested by the community » Needs work

inline event attribute is too much last centuary. Why not use the

and CSS fallback at the end of the article? no JS needed and we don't need to wait for something to fail to have the right image.
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

@nod_ Which one is that? I assumed we can't use a CSS fallback like the other issues because this is inline html?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented 
<object id="logo" type="image/svg+xml" data="logo.svg">
    <div>logo description</div>
</object>

#logo div {
    width: 300px;
    height: 50px;
    background-image: url("logo.png");
}
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue summary: View changes

Added test steps

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Let's move this one forward again? :)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

Ok lets do it! See: #2142655: [policy, no patch] Agree on a technique for loading SVG images in HTML that degrades to PNG

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
2083949-svg-image-filter-18.patch1.92 KB

I had a go at this... and failed. Someone else with better OO chops feel free to have a go.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2083949-svg-image-filter-19.patch1.39 KB

This ought to do the trick – replaces the image node passed by reference with an object.

Status: Needs review » Needs work

The last submitted patch, 19: 2083949-svg-image-filter-19.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Right, I've had a thorough look at the tests for this, and this is my conclusion: I can't make a useful series of tests that rely on the output of the posted form, because all of the 'red x' images (

s) are the same. The test on the objects needs to be something like 
$this->assertEqual((string) $element['data'], $red_x_image);
        $this->assertEqual((string) $element['type'], $type);
        $this->assertEqual((string) $element['title'], $title_text);
but I'm not able to identify them without looping through all of them, which seems both wasteful and not useful. Does anyone with more backend testing experience have any ideas as to how to make these tests work? I'm stumped!
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue tags: +frontend
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Related issues: +#2286601: [policy] Drop support for browsers that don't support SVG

Related: #2286601: [policy] Drop support for browsers that don't support SVG

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs work » Needs review
FileSize
2083949-filter-icons-24.patch2.21 KB

Here's a reroll of #9 as an example of how simple this is if we don't have to support png :)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Closed (fixed)

We now only support SVG and this work was completed in #2358675: Remove messages icons in misc