jonkemp / inline-css

Inline css into an html file.
MIT License
429 stars 85 forks source link

Width attributes applying with 'px' #46

Open podorozhny opened 8 years ago

podorozhny commented 8 years ago

Im getting <table width="600px" ...> instead of <table width="600" ...> using it with gulp-inline-css 3.1.0.

jamesmacwhite commented 8 years ago

@podorozhny @jonkemp I've noticed this behaviour as well. There is a similar issue with height as well.

Interestingly the width-attr.html test seems to be OK. I'm thinking this is perhaps a bug with certain options enabled together.

Essentially what happens currently is:

table#container-wrapper { 
    width: 600px;
}
<table id="container-wrapper" style="" width="600px">
    <tr>
        <td></td>
    </tr>
</table>

The empty style attribute, seems to be a clue about where this is failing, because there also should be a width: 600px rule present. There is code to strip the px value off table width attributes in lib/setWidthAttrs.js. I did a bit debugging with console.log and it does appear to be working but something is going wrong somewhere else in the process.

[09:08:10] Starting 'inline-css'...
{ '0':
   { type: 'tag',
     name: 'table',
     attribs: { id: 'container-wrapper', style: 'width: 600px;', width: '600' },
     children: [ [Object], [Object], [Object] ],
     next:
      { data: '\r\n\t\t\t\t',
        type: 'text',
        next: null,
        prev: [Circular],
        parent: [Object] },
     prev:
      { data: '\r\n\t\t\t\t\t',
        type: 'text',
        next: [Circular],
        prev: null,
        parent: [Object] },
     parent:
      { type: 'tag',
        name: 'td',
        attribs: [Object],
        children: [Object],
        next: [Object],
        prev: [Object],
        parent: [Object] },
     styleProps: { width: [Object] } },
  options:
   { xmlMode: true,
     decodeEntities: false,
     lowerCaseTags: true,
     lowerCaseAttributeNames: false,
     recognizeCDATA: false,
     recognizeSelfClosing: false,
     withDomLvl1: true,
     normalizeWhitespace: false },
  length: 1,
  _root: [Circular] }

After a bit of messing around it appears to be a conflict between certain options, having both applyWidthAttributes and applyTableAttributes set to true causes this behaviour.

It appears that within lib/setTableAttrs.js and line 38 within this if block, the properties get mangled.

if (styleVal !== undefined) {
    $el.attr(styleToAttrMap[style], styleVal);
    $el.css(style, '');
}

I haven't found a way to fix this without breaking the inline test yet, though part of the test seems to be wrong, as having px in height attributes isn't valid.

The problem is that line is very specific to remove any targeted CSS rules on <table>, <tr>, <td>, <th>, <tbody>, <thead>, <tfoot> that are converted to attributes, but it completely messes up the pixel widths in the process.

Quickest workaround currently is to simply not include px on CSS widths properties that target tables or table cells.

jamesmacwhite commented 8 years ago

@podorozhny If your interested I forked this and made some updates:

https://github.com/centralcollegenottingham/inline-css

The px issue is quite easy to fix on its own, a simple styleVal.replace('px', '') resolves it. Easiest solution for that problem. The table-attr.html test has been wrong from the start, having px on a width/height attribute is never valid.

I went a little bit further because the output of empty style="" attributes and removing certain width and height CSS properties was bugging me. I use this in a project for HTML email and for compatibility with Outlook clients that happen to use a higher DPI scaling setting than 100% via Windows Display settings, means you should always declare width/height attributes with the accompanying CSS width/height property when its not a percentage based value, so I've implemented that into my fork too.

Feel free to try it out.