flexboxIE and flexboxOld fail to check in case of value is null

[divinetouch]

I'm not really sure whether this is a bug or not as I encountered this issue when working with another project (Material-UI) that is using this library.

Basically the error occurs when the property is _display_ but the value is _null_ (_undefined_ is also a possibility?)

flexboxIE.js

if ((properties[property] || property === 'display' && value.indexOf('flex') > -1) && (browser === 'ie_mob' || browser === 'ie') && version == 10) {

flexboxOld.js

if ((properties[property] || property === 'display' && value.indexOf('flex') > -1) && (browser === 'firefox' && version < 22 || browser === 'chrome' && version < 21 || (browser === 'safari' || browser === 'ios_saf') && version <= 6.1 || browser === 'android' && version < 4.4 || browser === 'and_uc')) {

To get rid of the error temporary I added the check for value : typeof value === 'string'.

[tintin1343]

I have faced a similar issue while working on Material-UI. If I am not wrong, I should not be getting this issue when I am working on chrome right?

And I am pretty sure I am always passing a value of display: 'flex' wherever needed.

[robinweser]

@divinetouch You're right. I already added the typecheck to some plugins, but seems I forgot for both flexbox cases. I was not aware that someone could actually pass an unsupported value, but sure ternary operators sometimes result in undefined or null. I don't think it's wise to assign unsupported values, but since this happened to many ppl, we should add this feature.

@tintin1343 Actually no. At least not with the new 1.0+ versions (Material still uses 0.6.x I guess). With <1.0 there's been an issue with display values other than flex and inline-flex, but this was fixed by now. Still if you assign undefined or null you will face the same issue.

@divinetouch Finally, feel free to submit a pull request fixing this one :)

[tintin1343]

@rofrischmann : We upgraded it to v1 yesterday and it started failing after that. Let me check again.

[robinweser]

@tintin1343 Would be interessting to see a repro of that special case. Actually if your value is at least set (and type of string, which every display value should be after all) there should not be any issue with Chrome.

[tintin1343]

Issue on Chrome:

The version info from the Changelog.md file

I checked all the display properties in the project they all have string values associated with them.

[tintin1343]

@rofrischmann : I had to edit the file and remove value.indexOf('flex') > -1 from both flexboxIE.js and flexboxOld.js to make it work.

[tintin1343]

@rofrischmann : I can fix it. Do you want me to make a PR?

The change is basically a && condition. It fails for pages which does not have a display attribute. You are right. But if i'm using your library and it a particular page does not has a display property the library throws an error because of the OR condition in this line.

flexboxOld.js

if ((properties[property] **||** property === 'display' && value.indexOf('flex') > -1) && (browser === 'firefox' && version < 22 || browser === 'chrome' && version < 21 || (browser === 'safari' || browser === 'ios_saf') && version <= 6.1 || browser === 'android' && version < 4.4 || browser === 'and_uc')) {

the || property === 'display' should be changed to && property === 'display'

[robinweser]

@tintin1343 Replacing the || with && will break all other properties as obviously the property cannot be both e.g. justifyContent and display. Try to test with npm test, you will see 3 tests failing actually.
Your Chrome screenshot actually shows the problem after all. It is not able to call indexOf of null, which means there's at least one position where you're display value is null.
While removing the value.indexOf('flex') > -1 would bring back an old issue with other display values, the right way to fix this was already posted by @divinetouch as I have just tested shorty. You would add a typecheck before value.indexOf. e.g.

properties[property] ||  property === 'display' && typeof value === 'string' && value.indexOf('flex') > -1

Thanks for investigating and helping out though, feel free to edit your pull request and we will merge it asap.

[tintin1343]

@rofrischmann : You are right. I'm making those changes. Thanks!