'Disable common annoyances' prevents focusing of designmode iframes

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: s.marshall, Assigned: jst)

Tracking

({fixed1.8})

Trunk
x86
Windows XP
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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.

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

12 years ago
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.
Assignee: nobody → general
Component: Preferences → DOM
Keywords: helpwanted, qawanted
Product: Firefox → Core
QA Contact: preferences → ian
Version: unspecified → Trunk

Comment 3

12 years ago
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

12 years ago
(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).

Updated

12 years ago
Flags: blocking1.8b4?

Comment 5

12 years ago
Johnny, can you look into this?
Assignee: general → jst

Comment 6

12 years ago
We need to understand this before we ship. JST, when you get free, can you look
at this?
Flags: blocking1.8b4? → blocking1.8b4+

Comment 7

12 years ago
Is this the same as bug 304746?

Updated

12 years ago
Blocks: 304746

Updated

12 years ago
Blocks: 305084

Updated

12 years ago
Blocks: 299424

Comment 8

12 years ago
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

12 years ago
I agree with bz.

Comment 10

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

Comment 11

12 years ago
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

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

Comment 13

12 years ago
(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

12 years ago
So perhaps we should make it possible to focus a widget without raising the
whole window...
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.
*** Bug 304746 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

12 years ago
*** Bug 305084 has been marked as a duplicate of this bug. ***
No longer blocks: 305084
*** Bug 219049 has been marked as a duplicate of this bug. ***

Comment 19

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

Comment 20

12 years ago
*** Bug 196922 has been marked as a duplicate of this bug. ***
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

12 years ago
They who?

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

Updated

12 years ago
Flags: blocking1.8b5+ → blocking1.8b5-
(Assignee)

Comment 23

12 years ago
Created attachment 197761 [details] [diff] [review]
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).
Attachment #197761 - Flags: superreview?(bryner)
Attachment #197761 - Flags: review?(mrbkap)
Attachment #197761 - Flags: superreview?(bryner) → superreview+
Comment on attachment 197761 [details] [diff] [review]
Fix (and also eliminate some duplicated code).

r=mrbkap
Attachment #197761 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 25

12 years ago
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.
Status: NEW → ASSIGNED
Flags: blocking1.8b5- → blocking1.8b5?

Comment 26

12 years ago
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.
Attachment #197761 - Flags: approval1.8b5+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 27

12 years ago
Created attachment 198083 [details] [diff] [review]
1.8 version (same change w/o the extra cleanup).
(Assignee)

Comment 28

12 years ago
Fixed on trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

12 years ago
Keywords: helpwanted, qawanted
You need to log in before you can comment on or make changes to this bug.