search

InteractiveAdvertisingBureau / openvv

Other
81 stars 50 forks source link

Focus detection bug fix (case #29) #34

Closed alonashkenazi closed 10 years ago

alonashkenazi commented 10 years ago

Fix wrong focus detection in case another window overlaps the browser window. Link to the expected focus detection: https://docs.google.com/a/doubleverify.com/spreadsheets/d/1542E5-3MompVig-kQPuF2xtXSuVKGYmGB5HrN8_qa8U/edit#gid=0

bdementatbrightroll commented 10 years ago

verified tab and window cases on Mac in Chrome, FF, and Safari (non-iframe)

bdementatbrightroll commented 10 years ago

iframe cases: Tab focus is reported correctly in Safari, Firefox and Chrome on Mac, but when the window is NOT focused (but the tab is up), focus is always reported as true.

alonashkenazi commented 10 years ago

As described in the spreadsheet. This is the expected behavior

On Jul 10, 2014, at 5:56 PM, bdementatbrightroll notifications@github.com wrote:

iframe cases: Tab focus is reported correctly in Safari, Firefox and Chrome on Mac, but when the window is focused (but the tab is up), focus is always reported as true.

— Reply to this email directly or view it on GitHub https://github.com/openvv/openvv/pull/34#issuecomment-48669967.

bdementatbrightroll commented 10 years ago

(non-iframe cases) On Windows in Chrome and Firefox, tab focus is reported correctly, but window focus is always reported as true unless the window is minimized.

On Windows in IE, tab and window focus is always reported as false.

alonashkenazi commented 10 years ago

Which IE version did you use?

bdementatbrightroll commented 10 years ago

IE8, here's the user agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0

alonashkenazi commented 10 years ago

You can see in this link https://docs.google.com/a/doubleverify.com/spreadsheets/d/1542E5-3MompVig-kQPuF2xtXSuVKGYmGB5HrN8_qa8U/edit#gid=0that I haven't tested IE8.

On Fri, Jul 11, 2014 at 2:01 PM, bdementatbrightroll < notifications@github.com> wrote:

IE8, here's the user agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0

— Reply to this email directly or view it on GitHub https://github.com/openvv/openvv/pull/34#issuecomment-48762863.

alonashkenazi commented 10 years ago

I'm assuming that IE8 fail in the focus check because the way IN_IFRAME is detected: this.IN_IFRAME = (parent !== window); You can read on it here: http://stackoverflow.com/questions/4850978/ie-bug-window-top-false I suggest to change the check to: this.IN_IFRAME = (parent != window); WDYT?

bdementatbrightroll commented 10 years ago

Another commenter suggested: window.top === window.self I agree with you that it should be changed and either solution looks good to me. I have a slight preference for window.top === window.self

bdementatbrightroll commented 10 years ago

I went ahead and made that modification.

alonashkenazi commented 10 years ago

If there is no additional comments. I would like to merge by tomorrow EOF and close the issue. (different issue was opened for Chrome/Mac focus detection in iframe)

bdementatbrightroll commented 10 years ago

The additional comments satisfy the concerns I had

alonashkenazi commented 10 years ago

Also added unit tests for browser detection