300453 - 'Disable common annoyances' prevents focusing of designmode iframes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[s.marshall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050710 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050710 Firefox/1.0+

When 'Disable common annoyances' is turned on, JavaScript is prevented from
focusing iframes within the page - even though it is permitted to focus other
controls within a page such as edit boxes. 

Since iframes are often used as a rich-text alternative to edit boxes, this is
inconsistent.

Reproducible: Always

Steps to Reproduce:
1. Visit the above URL
2. Click in the iframe and click 'hello'
3. Click the Superscript button
4. Type 'world'
Actual Results:  
With 'Disable common annoyances' on, focus remains on the Superscript button, so
that 'world' does not appear in the edit box (iframe).

Expected Results:  
Focus should have returned to the edit box after step 3 and 'world' should
appear there (in superscript). This is what happens when 'Disable common
annoyances' is off.

I minimised this code as much as possible for the testcase (and to still
demonstrate why we need the functionality), but the relevant line is:

  setTimeout(function() {myframe.contentWindow.focus() },0);

(don't know why the setTimeout is there, may just be for IE.)

Most likely this was categorised as 'common annoyances' in part of an attempt to
prevent JavaScript from changing the system focus top-level window (i.e.
focusing the current window every second so that the user can't do anything
else). However in this case the change is within a top-level window so it should
be allowed, just as you're allowed to focus ordinary editboxes.

Comment 1

[Bastian Grupe]

I can confirm the described behaviour with the following build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050711
Firefox/1.0+

I'd like to see it fixed aswell, since I will also be doing some design mode
stuff in the future.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Doesn't calling focus() on an iframe also raise the window the iframe is in?  
Certainly looking at nsGlobalWindow::Focus it looks like it might at least in 
some cases (eg for embedding apps).

In any case, isn't a Firefox prefs bug.

Comment 3

[Michael Lefevre]

To expand on what Boris said, if you turn off "allow scripts to raise or lower
windows" in the prefs for Firefox 1.0.x, it has the same effect of setting the
"dom.disable_window_flip" pref to true, which breaks the testcase here. The
question then is whether it's possible to fix the behaviour with iframes while
disallowing "window flipping".

The arrangement of prefs is intentional - the "disable common annoyances" box
breaks other features that are commonly used in pages, in particular I've seen a
couple of pages which are broken by disabling switching images. Users will have
to come to understand that. I believe something more sophisticated and
site-specific is planned for a future (1.5? 2.0?) version of Firefox.

Comment 4

[Bastian Grupe]

(In reply to comment #3)

In my opinion the step from 1.0 to 1.1 was rather a step backwards than forwards
by disabling to set more advanced options. I don't think that it is appropriate
that I have to install an extension just because I want 5 of 6 "common
annoyances" disabled and need the remaining one.

As for site specific, +1 from me there, maybe even as an icon in the statusbar
(like greasemonkey has).

Comment 5

[Asa Dotzler [:asa]]

Johnny, can you look into this?

Comment 6

[Asa Dotzler [:asa]]

We need to understand this before we ship. JST, when you get free, can you look
at this?

Comment 7

[Asa Dotzler [:asa]]

Is this the same as bug 304746?

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the issue is that focusing an iframe _does_ raise the window.  I suggest that
the right fix for this bug is to turn this off (so focusing an iframe just
focuses the iframe) and then to make the "window flip" pref not apply to focus()
calls on iframes.  Does that seem reasonable?

Comment 9

[Jesse Ruderman]

I agree with bz.

Comment 10

[Jesse Ruderman]

I could probably fix this using IsFrame() if I understood what each part of
nsGlobalWindow::Focus did.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I believe the part that uses treeOwnerAsWin is what's raising the toplevel
window while the part that uses presShell and focusController is what's moving
the focus into the iframe.

Comment 12

[Boris Zbarsky [:bzbarsky]]

But it's also possible that the SetFocus() call on the widget is what raises the
window.  Would need testing there...

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #12)
> But it's also possible that the SetFocus() call on the widget is what raises the
> window.  Would need testing there...

Indeed, the SetFocus(PR_TRUE) call on the widget is what raises the window to
the top (if *a* firefox window is focused already), and it's also the call we
need to get focus to actually move into a subframe.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So perhaps we should make it possible to focus a widget without raising the
whole window...

Comment 15

[Brian Ryner (not reading)]

It sounds like what we should be doing is:

1. Check the focus controller to see if the toplevel window has focus (IsActive)
2. If it does, call widget->SetFocus() and set aRaise = PR_FALSE if the
disable_window_flip pref is set to true.
3. If the toplevel window doesn't have focus, then just set the focus
controller's focusedWindow to the iframe being focused.

Comment 16

[Brian Ryner (not reading)]

*** Bug 304746 has been marked as a duplicate of this bug. ***

Comment 17

[Johnny Stenback (:jst)]

*** Bug 305084 has been marked as a duplicate of this bug. ***

Comment 18

[Martijn Wargers (dead)]

*** Bug 219049 has been marked as a duplicate of this bug. ***

Comment 19

[Jesse Ruderman]

*** Bug 118993 has been marked as a duplicate of this bug. ***

Comment 20

[Jesse Ruderman]

*** Bug 196922 has been marked as a duplicate of this bug. ***

Comment 21

[Grayson Mixon (ColdFusion650)]

isnt this kind of pointless since they back out "disable common annoyances"?
unless this also affects a particular part of the old advanced javascript
options menu.

Comment 22

[Boris Zbarsky [:bzbarsky]]

They who?

And yes, this affects the "advanced javascript options" stuff.

Comment 23

[Johnny Stenback (:jst)]

Attachment: Fix (and also eliminate some duplicated code).

This fixes this bug (first half of the diff), and also removes some duplicated
code (the second half of the diff).

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197761 [details] [diff] [review]
Fix (and also eliminate some duplicated code).

r=mrbkap

Comment 25

[Johnny Stenback (:jst)]

Re-nominating now that I've got a fix. This is actually very straight forward
(we'd only need the first half of the patch for the branch). We could undo the
backout of the change of the default value of the dom.disable_window_flip if we
choose to take this.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 197761 [details] [diff] [review]
Fix (and also eliminate some duplicated code).

let's take the patch and leave the pref alone for now.

Comment 27

[Johnny Stenback (:jst)]

Attachment: 1.8 version (same change w/o the extra cleanup).

Comment 28

[Johnny Stenback (:jst)]

Fixed on trunk and branch.