Incorrect false for generated content on Android 4.0 #738

Closed
thany opened this Issue Oct 31, 2012 · 24 comments

Comments

Projects
None yet
4 participants
@thany

thany commented Oct 31, 2012

Generated content is falsely undetected (meaning it is available) on the standard browser in Android 4.0.4 (in my case, CyanogenMod version "9-20120716-UNOFFICAL-ville" on an HTC One S).

The circumstances by which the bug can be reproduced are exactly the same as mentioned in this issue:
#665

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Nov 1, 2012

Owner

Wow bizarro, Android has some interesting behaviour. Any font-size less than 5px will not affect the height of the test element and return 0 on testing the offsetHeight 5px will return 1px and 6px will return 2px. Once I hit 7px it starts returning the actual height that corresponds to the font-size.

Will get a fix in real soon. Thanks.

Owner

ryanseddon commented Nov 1, 2012

Wow bizarro, Android has some interesting behaviour. Any font-size less than 5px will not affect the height of the test element and return 0 on testing the offsetHeight 5px will return 1px and 6px will return 2px. Once I hit 7px it starts returning the actual height that corresponds to the font-size.

Will get a fix in real soon. Thanks.

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Nov 1, 2012

Owner

@thany can you run this testcase and let me know if this alerts true http://jsfiddle.net/kSrqy/28/

Owner

ryanseddon commented Nov 1, 2012

@thany can you run this testcase and let me know if this alerts true http://jsfiddle.net/kSrqy/28/

@thany

This comment has been minimized.

Show comment Hide comment
@thany

thany Nov 1, 2012

Very cool :)
I'm seeing the same thing on Safari 3.2 on Windows XP... Could it be the same bug?

/edit
Oh, and my phone says "true" on the fiddle. For reference, my Transformer TF201, running Android 4.1.1 stock, also says "true". Unfortunately Safari 3.2 on XP says "false".

thany commented Nov 1, 2012

Very cool :)
I'm seeing the same thing on Safari 3.2 on Windows XP... Could it be the same bug?

/edit
Oh, and my phone says "true" on the fiddle. For reference, my Transformer TF201, running Android 4.1.1 stock, also says "true". Unfortunately Safari 3.2 on XP says "false".

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Nov 2, 2012

Owner

I'm not too worried about Safari 3.2 since it's not on our radar for testing, plus the fact that browserstack doesn't even go that far back for Safari versions I have no way to test it. If you can find a fix yourself I'll add it in otherwise I won't worry.

Owner

ryanseddon commented Nov 2, 2012

I'm not too worried about Safari 3.2 since it's not on our radar for testing, plus the fact that browserstack doesn't even go that far back for Safari versions I have no way to test it. If you can find a fix yourself I'll add it in otherwise I won't worry.

@thany

This comment has been minimized.

Show comment Hide comment
@thany

thany Nov 2, 2012

I get my old browsers from here:
http://www.oldapps.com/
But requires a lot of manual labor to test them all like that. I understand you can't support any versions that are really old ;)

But thanks again for the fix for Android. I'll look forward to the next release.

thany commented Nov 2, 2012

I get my old browsers from here:
http://www.oldapps.com/
But requires a lot of manual labor to test them all like that. I understand you can't support any versions that are really old ;)

But thanks again for the fix for Android. I'll look forward to the next release.

@ryanseddon ryanseddon closed this in 871caa1 Nov 3, 2012

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Nov 3, 2012

Owner

@thany feel free to do a pull request if you find a fix for Safari 3.2 otherwise I'll leave it as a known issue.

Owner

ryanseddon commented Nov 3, 2012

@thany feel free to do a pull request if you find a fix for Safari 3.2 otherwise I'll leave it as a known issue.

SlexAxton added a commit to SlexAxton/Modernizr that referenced this issue Nov 5, 2012

Merge branch 'master' of github.com:Modernizr/Modernizr
* 'master' of github.com:Modernizr/Modernizr:
  Fixed false positive of 3d transforms if test element inherits margin, padding or border, fixes #740
  Fix false negative in Android 4, fixes #738
  Update feature-detects/exif-orientation.js
  Trim trailing whitespace and insert final newlines
@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 5, 2013

Did this make it into the build as I'm running on Android 4.0.3 (Stock browser version 4.0.2215272349.382087) and Modernizr 2.7.1 and it's still flagging incorrectly?

Did this make it into the build as I'm running on Android 4.0.3 (Stock browser version 4.0.2215272349.382087) and Modernizr 2.7.1 and it's still flagging incorrectly?

@thany

This comment has been minimized.

Show comment Hide comment
@thany

thany Dec 5, 2013

If you look at the changes you'll see the size of the element to check on has been increased to 7px... Perhaps you encountered a situation where the minimum size that makes M report correctly on Android is larger?

thany commented Dec 5, 2013

If you look at the changes you'll see the size of the element to check on has been increased to 7px... Perhaps you encountered a situation where the minimum size that makes M report correctly on Android is larger?

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 5, 2013

Is there are reason it was set to 7px? What's the logic behind it?

Is there are reason it was set to 7px? What's the logic behind it?

@thany

This comment has been minimized.

Show comment Hide comment
@thany

thany Dec 5, 2013

Or, if you are using a font on the entire site that does not include glyphs for ":" and ")" characters, it also wouldn't work. One hell of an edge case, but it could theoretically happen ;)

Anyway, I think @ryanseddon would know the answer to your question ;)

thany commented Dec 5, 2013

Or, if you are using a font on the entire site that does not include glyphs for ":" and ")" characters, it also wouldn't work. One hell of an edge case, but it could theoretically happen ;)

Anyway, I think @ryanseddon would know the answer to your question ;)

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 5, 2013

Hmmm that might be the cause. I'm using an Icomoon font set for icons. I'm using the icomoon font on my menu items and falling back to a png for browsers that don't support it. I'm seeing both in Android. Do I take it that if I added ':' or ')' to my set it might start working?

Hmmm that might be the cause. I'm using an Icomoon font set for icons. I'm using the icomoon font on my menu items and falling back to a png for browsers that don't support it. I'm seeing both in Android. Do I take it that if I added ':' or ')' to my set it might start working?

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Dec 5, 2013

Owner

@jeepstone hard to say what exactly is happening. Are you able to create a small testcase that reproduces this issue?

Owner

ryanseddon commented Dec 5, 2013

@jeepstone hard to say what exactly is happening. Are you able to create a small testcase that reproduces this issue?

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 6, 2013

@ryanseddon The only thing I have is a fully built site (currently in test). Is that any use? I'd prefer not to post the link on here. Where should I send it (if it's any use)?

@ryanseddon The only thing I have is a fully built site (currently in test). Is that any use? I'd prefer not to post the link on here. Where should I send it (if it's any use)?

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 10, 2013

I've used BrowserStack to identify that it only affects Android browsers running on Android 4.0. I have checked the following:

Working correctly: Samsung Galaxy S (2.2), Samsung Galaxy S2 (2.3), Samsung Galaxy S3 (4.1), LG Nexus 4 (4.2)

Failing: Samsung Galaxy Nexus (4.0), HTC Evo 3D (4.0), HTC Sensation XE (4.0)

I've also added both the : and ) to the Icomoon font with no joy

I've used BrowserStack to identify that it only affects Android browsers running on Android 4.0. I have checked the following:

Working correctly: Samsung Galaxy S (2.2), Samsung Galaxy S2 (2.3), Samsung Galaxy S3 (4.1), LG Nexus 4 (4.2)

Failing: Samsung Galaxy Nexus (4.0), HTC Evo 3D (4.0), HTC Sensation XE (4.0)

I've also added both the : and ) to the Icomoon font with no joy

@stucox

This comment has been minimized.

Show comment Hide comment
@stucox

stucox Dec 10, 2013

Owner

We’ve seen rounding issues with this kind of test when browser windows are zoomed. The rules for mobile browser zooming are a bit funny, so even if you don’t think it’s zoomed this might be worth trying anyway.

As a quick hack test, could you find offsetHeight>=7 in your Modernizr build (should only be in there once) and change it to offsetHeight>=5 and see if it fixes it? (keep the styles before it the same – the idea is to give a bit of a margin between the two).

Owner

stucox commented Dec 10, 2013

We’ve seen rounding issues with this kind of test when browser windows are zoomed. The rules for mobile browser zooming are a bit funny, so even if you don’t think it’s zoomed this might be worth trying anyway.

As a quick hack test, could you find offsetHeight>=7 in your Modernizr build (should only be in there once) and change it to offsetHeight>=5 and see if it fixes it? (keep the styles before it the same – the idea is to give a bit of a margin between the two).

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 10, 2013

Hi @stucox I could only find s.generatedcontent=function(){var a;return y(["#",h,"{font:0/0 a}#",h,':after{content:"',l,'";visibility:hidden;font:3px/1 a}'].join(""),function(b){a=b.offsetHeight>=3}) in custom build 2.7.1.

Changing the offsetHeight doesn't have an effect. Does the font:3px also need changing?

Hi @stucox I could only find s.generatedcontent=function(){var a;return y(["#",h,"{font:0/0 a}#",h,':after{content:"',l,'";visibility:hidden;font:3px/1 a}'].join(""),function(b){a=b.offsetHeight>=3}) in custom build 2.7.1.

Changing the offsetHeight doesn't have an effect. Does the font:3px also need changing?

@stucox

This comment has been minimized.

Show comment Hide comment
@stucox

stucox Dec 10, 2013

Owner

Ah, I’m guessing the fix above was merged after we released v2.6.2. In which case, we probably don’t have a stable build which includes this yet (sorry…). Releases since v2.6.2 have just been emergency backport fixes. If we ask @patrickkettner nicely he might do a v2.7.2 with this fix in it…

But for now, you could try changing the code to font:5px/1 a and b.offsetHeight>=5 respectively, which will match @ryanseddon’s bug fix above.

Owner

stucox commented Dec 10, 2013

Ah, I’m guessing the fix above was merged after we released v2.6.2. In which case, we probably don’t have a stable build which includes this yet (sorry…). Releases since v2.6.2 have just been emergency backport fixes. If we ask @patrickkettner nicely he might do a v2.7.2 with this fix in it…

But for now, you could try changing the code to font:5px/1 a and b.offsetHeight>=5 respectively, which will match @ryanseddon’s bug fix above.

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 10, 2013

Sadly no joy :( with the bug fix. I've heard that @patrickkettner is a lovely human though......

Sadly no joy :( with the bug fix. I've heard that @patrickkettner is a lovely human though......

@stucox

This comment has been minimized.

Show comment Hide comment
@stucox

stucox Dec 10, 2013

Owner

What does it report if you change a=b.offsetHeight>=3 to alert(b.offsetHeight)?

Owner

stucox commented Dec 10, 2013

What does it report if you change a=b.offsetHeight>=3 to alert(b.offsetHeight)?

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 10, 2013

0 in Stock android
3 in Chrome

0 in Stock android
3 in Chrome

@stucox

This comment has been minimized.

Show comment Hide comment
@stucox

stucox Dec 10, 2013

Owner

Sorry, just noticed above I should have said the code should be font:7px/1 a and b.offsetHeight>=7 to match Ryan’s fix.

But if that doesn’t work, what’s b.offsetHeight when the font’s set to 7px?

Owner

stucox commented Dec 10, 2013

Sorry, just noticed above I should have said the code should be font:7px/1 a and b.offsetHeight>=7 to match Ryan’s fix.

But if that doesn’t work, what’s b.offsetHeight when the font’s set to 7px?

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 10, 2013

Bingo! 7px returns 7 for both Chrome and stock Android and now works and returns the correct value for generated content! Many thanks!

Bingo! 7px returns 7 for both Chrome and stock Android and now works and returns the correct value for generated content! Many thanks!

@ryanseddon

This comment has been minimized.

Show comment Hide comment
@ryanseddon

ryanseddon Dec 10, 2013

Owner

Yeah from my testing 7px was the magic number which I mention above. #738 (comment)

Owner

ryanseddon commented Dec 10, 2013

Yeah from my testing 7px was the magic number which I mention above. #738 (comment)

@jeepstone

This comment has been minimized.

Show comment Hide comment
@jeepstone

jeepstone Dec 11, 2013

Thanks @ryanseddon makes sense now.

Thanks @ryanseddon makes sense now.

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this issue Feb 22, 2015

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this issue Feb 22, 2015

Merge branch 'master' of github.com:Modernizr/Modernizr
* 'master' of github.com:Modernizr/Modernizr:
  Fixed false positive of 3d transforms if test element inherits margin, padding or border, fixes #740
  Fix false negative in Android 4, fixes #738
  Update feature-detects/exif-orientation.js
  Trim trailing whitespace and insert final newlines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment