Opened 6 years ago

Closed 6 years ago

#16699 closed defect (fixed)

Menu: Font-Awesome not working as iconClass

Reported by: Martin Minka Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.3
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

I am using http://fortawesome.github.com/Font-Awesome/ in Dijit buttons and toolbars. It works perfectly.

But there is problem in MenuItem and CheckedMenuItem widgets because it uses IMG tag.

I am not sure what impact it has on the whole Menu suite to replace IMG tag, but I created dirty hack to replace IMG with DIV and seams to work: http://jsfiddle.net/bigm/x52ZF/

Please, could somebody suggest solution which would work directly in MenuItem.js/MenuItem.html ?

Attachments (3)

menuitem.htm (2.9 KB) - added by Martin Minka 6 years ago.
test case
menuitem-fix.html (3.1 KB) - added by Martin Minka 6 years ago.
fix onclick event
spanIcons.patch (10.2 KB) - added by bill 6 years ago.
use <span> rather than <img> for icons

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by bill

Owner: changed from bill to Martin Minka
Status: newpending

Why would we want to replace the IMG tag? I'm not sure what problem you are having.

Please attach a test case using the "attach file" button. It should be as small as possible to still reproduce the problem, almost always a single HTML file that we can load in the browser (i.e. not PHP, JSP, etc.)

Then, give exact instructions on how to reproduce the problem using your attached test file.

The test case is necessary both to confirm that there's a bug, and for us to be able to debug the problem.

Thanks!

Changed 6 years ago by Martin Minka

Attachment: menuitem.htm added

test case

comment:2 Changed 6 years ago by Martin Minka

Status: pendingnew

Attachment (menuitem.htm) added by ticket reporter.

comment:3 Changed 6 years ago by bill

Status: newpending

OK thanks. But what is the fixIcon() function for? If you are just trying to set a MenuItem's icon, you should do it the standard way.

comment:4 Changed 6 years ago by Martin Minka

Status: pendingnew

(sorry, for delay of this post)

Maybe you missed the test example in http://jsfiddle.net/bigm/x52ZF/ ?

I am attaching the HTML I produced from jsfiddle example.

  • open the file
  • you should see icon-camera-retro rendered in top of the page (that is test that the font is loaded correctly)
  • the Copy item in menu has no visible icon
  • click the button on the bottom (it is dirty hack to replace IMG with DIV)
  • you will now see sitemap icon in the Copy item

I am not able to tell why the font icon is not showing if IMG is used. I was trying to prove that it would work if DIV is used. I am not proposing solution, because I am not sure if changing IMG to DIV would not influence visual appearance of menu items in other situations.

I believe that the font icons should work in MenuItem? in same way as they do in buttons and hope that somebody more clever will come with a good fix.

Thank you.

comment:5 Changed 6 years ago by bill

Description: modified (diff)
Milestone: tbd1.9
Owner: changed from Martin Minka to bill
Status: newassigned
Summary: [dijit/MenuItem][cla] Font-Awesome not working as iconClass in MenuItemMenu: Font-Awesome not working as iconClass

Ah I did miss the fiddle, and didn't realize you were trying to demonstrate a fix to the problem.

OK, so the issue is that FontAwesome is using fonts to display the icon, whereas Menu (and the rest of dijit) is expecting a CSS class that sets background-image. I haven't seen this font technique before. It's interesting, although presumably not portable to older browsers?

Anyway, in general some dijit widgets are using <img> for icons rather than <div> or <span> because the <img> element has a natural inline-block styling. But probably (as you thought) Menu can be changed to use a <div> or <span>.

I don't have any suggested workaround besides overriding MenuItem's template.

comment:6 Changed 6 years ago by Martin Minka

I am also excited about this new technique and there are more fonts usable this way: http://css-tricks.com/flat-icons-icon-fonts/

I tried to compile list of files where IMG is used to display icons:

dijit/layout/templates/_TabButton.html
dijit/layout/templates/AccordionButton.html
dijit/templates/MenuItem.html
dijit/templates/TreeNode.html
dijit/templates/CheckedMenuItem.html

Thank you for your time. Please let me know if I could help you with some testing or some other way.

comment:7 Changed 6 years ago by bill

Thanks. I looked over those files and found that <img> was necessary (or at least helpful) for use in Tree. The other ones probably don't matter, but would need to test.

  • dijit/layout/templates/_TabButton.html, dijit/layout/templates/AccordionButton.html - from [17429], originally used <img> node
  • dijit/templates/MenuItem.html, dijit/templates/CheckedMenuItem.html - switched from <div> to <img> in [16251] but apparently wasn't necessary
  • dijit/templates/TreeNode.html - switched from <div> to <img> in [14104] to fix vertical alignment problems

comment:8 Changed 6 years ago by thesociable

I did something similar for #14518 - see the approach I used at https://github.com/thesociable/dbootstrap/blob/master/source/dbootstrap/icon_support.js

Essentially it patches the templated mixin to swap an incompatible node with a span. It works for the tree in my simple demo, but might be interesting to see where this falls down.

comment:9 Changed 6 years ago by bill

k2s - Is this working for you on IE, and if so, which version?

BTW I'm especially interested in this technique for icons built in to dijit like the Tree expando and folder icons, because it would solve problems with high-contrast mode where icons (implemented using CSS background-image) aren't displayed, necessitating the complications of characters embedded in the templates like

<span data-dojo-attach-point="expandoNodeText" class="dijitExpandoText" role="presentation"
		>

comment:10 Changed 6 years ago by Martin Minka

my application is IE8 compatible and the attached test file works there without issues, icons in buttons work also ok

http://fortawesome.github.com/Font-Awesome/ states that it is IE7 compatible by including additional font-awesome-ie7.min.css

this page http://icomoon.io/#docs has chapter "Supporting IE 7 and IE 6" (also including additional CSS)

i dont have IE6 but I will try IE7 from http://www.modern.ie/en-us/virtualization-tools

comment:11 Changed 6 years ago by bill

Do you happen to know *how* it works on IE8?

comment:12 Changed 6 years ago by bill

OK, I figured it out. Although neither http://jsfiddle.net/bigm/x52ZF/ nor http://thesociable.github.com/dbootstrap/ run on IE8, http://fortawesome.github.com/Font-Awesome/ does, and it works via a CSS rule:

.icon-edit:before {
   content: '\f044'
}

So, presumably the same as it works on modern browsers. I'm surprised; I didn't think IE8 would support such CSS.

Note that it also has some background-image CSS but it isn't being used because the rule is overridden. Probably it's there for IE7.

comment:13 Changed 6 years ago by bill

PS: @k2s, I tried your fiddle but the button to replace <img> with <span> isn't doing anything, at least not on chrome/mac. The <img> is still there.

comment:14 Changed 6 years ago by Martin Minka

pls. try http://jsfiddle.net/bigm/x52ZF/1/

I changed onclick in data-dojo-props (worked in FF) to data-dojo-event="click" (works for me now with ff, chromium)

I am also attaching fixed test case file, which works for me also with IE8.

Changed 6 years ago by Martin Minka

Attachment: menuitem-fix.html added

fix onclick event

Changed 6 years ago by bill

Attachment: spanIcons.patch added

use <span> rather than <img> for icons

comment:15 Changed 6 years ago by bill

OK thanks. I'm working on a patch for this; everything looks pretty much as before except for the arrow icon on PopupMenuItems (to the right of the label). I notice in your jsfiddle that (although you are using a <span> for the icon to the left of the label), you are still using an <img> node for the arrow icon to the right of the label. The problem I see when switching that node to a <span> is that the arrow is too low.

Last edited 6 years ago by bill (previous) (diff)

comment:16 Changed 6 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30903]:

Use <span> rather than <img> so that font-icons can be used, fixes #16699. In general the layout is like before, except I had to hack a margin-bottom setting for the arrows on PopupMenuItems, to get their height (relative to the text) correct.

Note: See TracTickets for help on using tickets.