748803 - Frame with elements blocked through hosts file prevents from scrolling down the page with spacebar

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Loic]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120425 Firefox/15.0a1
Build ID: 20120425030647

Steps to reproduce:

I think it's a regression. If an element contained in a frame (like ads) is blocked via the hosts file in Windows, it's not possible to use the spacebar to scroll down the page because the focus is stolen by the frame.

STR:
1. Be sure you test with a profile without ad/script blockers like Adblock Plus or NoScript.
2. In Windows, open (as admin) the hosts file (C:\WINDOWS\system32\drivers\etc\hosts)
3. Add the line "127.0.0.1    ad.doubleclick.net" (without " " and with a tab between IP and domain)
4. Open the page http://www.washingtonpost.com/world/africa/sudanese-warplanes-bomb-town-in-south-sudan/2012/04/23/gIQArKp0bT_story.html
NB: the page needs 5-10 sec to be fully loaded (3 loads)
5. Press spacebar to scroll down the page 


Actual results:

Spacebar scrolls down the upper right frame containing the element (an ad) blocked by the hosts file.


Expected results:

Spacebar should scroll down the entire page but not the frame with blocked ad.

Comment 1

[Loic]

Regression range:

m-c:
good=2012-01-25
bad=2012-01-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=005488525c43&tochange=402b394b6623

m-i:
good=2012-01-24
bad=2012-01-25
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=388edf50e323&tochange=badb5fd30031

Comment 2

[Alice0775 White]

Regression window:
Cannot reproduce
http://hg.mozilla.org/integration/mozilla-inbound/rev/49c9b4e6325b
Mozilla/5.0 (Windows NT 5.1; rv:12.0a1) Gecko/20120124 Firefox/12.0a1 ID:20120124161851
Can reproduce:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9b12afb9724
Mozilla/5.0 (Windows NT 5.1; rv:12.0a1) Gecko/20120124 Firefox/12.0a1 ID:20120124163248
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49c9b4e6325b&tochange=f9b12afb9724

Suspected:
b9fe3e1419c6	Wes Johnston — Bug 301471 - Autofocus the Try Again button in the net error dialog. r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  I wonder whether it would make sense to not autofocus in subframes.  uxwanted!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Given bug 749218 I think we should track this and strongly consider backing out bug 301471 if we can't avoid autofocusing in subframes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Mounir, is there a sane way to make autofocus not take effect in a subframe?  Would script that runs in that document before onload and removes the attribute when framed work?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Or should autofocus perhaps not do anything in subframes in general?

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Or should autofocus perhaps not do anything in subframes in general?

I do not think that would be a good idea. A lot of webpages are doing heavy use of iframe's AFAIK and making autofocus not working in that case would be a very strong limitation. FWIW, Webkit and Presto have that same behavior.

I think a cleaner solution would be to change that error page autofocus behavior and not autofocus if the error page is loaded inside an iframe.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't autofocus the Try Again button in error pages if the error page is not a toplevel page.

Comment 9

[(no longer active)]

Comment on attachment 619131 [details] [diff] [review]
Don't autofocus the Try Again button in error pages if the error page is not a toplevel page.

The overflow properties in the test looks like a mistake!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, indeed.  I'll remove those.

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 619131 [details] [diff] [review]
Don't autofocus the Try Again button in error pages if the error page is not a toplevel page.

Review of attachment 619131 [details] [diff] [review]:
-----------------------------------------------------------------

I think using @autofocus instead of .focus() here is really a detail. I would be okay with .focus() FWIW.

This said, I"m not sure I understand what you are trying to do with the test. I must be missing something but wouldn't have been easier to have a mochitest checking that the focus wasn't on the iframe but on the top-level document after the page is loaded?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I would be okay with .focus() FWIW.

I would rather not, because that doesn't play as well with explicit focus changes by the user.

> This said, I"m not sure I understand what you are trying to do with the test.

What I'm trying to do with this test is basically write a reftest for bug 749218.  Seems to work.  ;)

> wouldn't have been easier to have a mochitest checking that the focus wasn't on the
> iframe

Maybe, if I knew offhand how to check where the focus is.  ;)

Comment 14

[Alex Keybl [:akeybl]]

Do we expect this bug to rear it's head in any other way? Loss of spacebar scrolling when the hosts file has been edited to block certain content seems very edge-casey.

Comment 15

[Loic]

(In reply to Alex Keybl [:akeybl] from comment #14)
> Do we expect this bug to rear it's head in any other way? Loss of spacebar
> scrolling when the hosts file has been edited to block certain content seems
> very edge-casey.

For the record, another bugs similar to this bug have been reported, but with various behaviours. See:
- Bug 748787
- Bug 749218
- Bug 749969

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Loic from comment #15)
> For the record, another bugs similar to this bug have been reported, but
> with various behaviours. See:
> - Bug 748787
> - Bug 749218
> - Bug 749969

Yeah the number of unique instances of this after FF12's release makes this a higher priority for the next release. Let's try to fix this for FF13 if deemed low risk.

Comment 17

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/80ece0284245

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 619131 [details] [diff] [review]
Don't autofocus the Try Again button in error pages if the error page is not a toplevel page.

[Approval Request Comment]
Regression caused by (bug #): bug 301471
User impact if declined: Some websites randomly scroll to weird places,
   especially when ad-blockers are in use.  Some websites have odd focus
   behavior, especially when ad-blockers are in use.
Testing completed (on m-c, etc.): Manual verification that both the bad behaviors
   are gone and that full-page error page still fous the "Try Again" button.
   Plus the reftest in the patch.
Risk to taking this patch (and alternatives if risky):  I believe this is
   low-risk.  The error page already depends on script running in it, as far as
   I can tell, so there should be no new issues with users who have script
   disabled: either it already works and will continue to work, or it's already
   completely broken.
String changes made by this patch:  None.

Comment 19

[dreapadoir]

--- Comment #5 from MozillaUser233 <[e-mail]> 2012-05-01 14:36:21 PDT ---
(In reply to dreapadoir from comment #4)
> FF12 auto-scrolls down to a section where an add has been blocked.
> This behavior did not occur in previous versions.
> Very annoying and unproductive.
> Is there an estimated time for a fix release?

They say this is the same bug
https://bugzilla.mozilla.org/show_bug.cgi?id=748803

You might get a quicker response from there.

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/80ece0284245

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Maybe, if I knew offhand how to check where the focus is.  ;)

document.activeElement

Given that the focus for autofocus is made asynchronously, I'm not sure that the test couldn't produce random-green because the snapshot would have been taken before the focus change.
I would have preferred to check for the focused element in the load event after a setTimeout(, 0).

(sorry for writing that comment after the landing, I forgot to reply yesterday)

Comment 22

[Loic]

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> https://hg.mozilla.org/mozilla-central/rev/80ece0284245

This patch in Nightly doesn't seem to fix all the regressions, see my comment https://bugzilla.mozilla.org/show_bug.cgi?id=748787#c13

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 619131 [details] [diff] [review]
Don't autofocus the Try Again button in error pages if the error page is not a toplevel page.

[Triage Comment]
Approved for Beta 13 and Aurora 14 given the low risk evaluation and the increasing reports of regression after FF12's release.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Given that the focus for autofocus is made asynchronously

So is the snapshot made onload, no?  I don't think this can accidentally go green.

I would be happy to add an additional mochitest though, if you prefer.

Comment 25

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/445db7a929dc
http://hg.mozilla.org/releases/mozilla-beta/rev/2799dfa4f22a

Comment 27

[Ioana (away)]

Verified as fixed with the STR from comment 0 on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120509070325

Comment 30

[Ioana (away)]

Verified as fixed with the STR from comment 0 on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0 (20120605113340)

Comment 31

[Ioana (away)]

Also verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0