310458 - [splitwindow] edit commands not available to extensions window (cut, copy, paste, select all, arrow keys navigation)

Status: RESOLVED FIXED

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

Description

[François Gagné]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050928 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050928 Firefox/1.6a1

The preference window of Adblock don't have working edit and navigation commands

Reproducible: Always

Steps to Reproduce:
1. Install Adblock
2. Do a Ctrl+Shift+P to get the preferences

Actual Results:  
Unable to Paste, navigate with the arrow keys.


If you change to another application and go back to Firefox the Paste and
navigation with arrow keys works.

Comment 1

[Jason Ellis]

I can confirm this on the latest branch nightly.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20050929 Firefox/1.4

Comment 2

[François Gagné]

Additionnal detail, I'm talking about Adblock Plus:
http://www.extensionsmirror.nl/index.php?showtopic=774

Comment 3

[Ria Klaassen (not reading all bugmail)]

I posted that already in Bug 220900 comment 134:
https://bugzilla.mozilla.org/show_bug.cgi?id=220900#c134

Comment 4

[Boris Zbarsky [:bzbarsky]]

The adblock code does:

  function adblockSettings() {
      var settingsHandle = 
        window.open("chrome://adblock/content/settings.xul", 
                    "adblockPreferences",
                    "chrome,resizable,centerscreen,close=no");
      settingsHandle.focus();
  }

I bet that focus() call is the problem.  Can someone who can reproduce this bug
try removing that (the file is adblock.js, in the adblock.jar JAR) and seeing
whether that helps?

If that helps, I suspect this is not a splitwindow issue.  For that matter, is
there any evidence that this is caused by splitwindow?

Comment 5

[Boris Zbarsky [:bzbarsky]]

If this is a regression, when did it regress?

Comment 6

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #5)
> If this is a regression, when did it regress?

1.8b4_2005073013 - 1.8b4_2005073111 (splitwindow indeed).

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK... So can you try removing that focus() call and seeing what happens?

Comment 8

[François Gagné]

definitively a regression of splitwindow because it followed (to my memory) the
timeline of Comment #54 of bug #305032, also comment #1 and comment #2 in bug
#307355.

Comment 9

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #7)
> OK... So can you try removing that focus() call and seeing what happens?
Yess! It helps.

Comment 10

[Boris Zbarsky [:bzbarsky]]

What if you replace "settingsHandle.focus();" with "var foo =
settingsHandle.focus"?  Is the bug still around?

That is, is the problem getting the "focus" property, or calling the "focus"
method?  My bets are on the former...

Comment 11

[Boris Zbarsky [:bzbarsky]]

So for what it's worth, I do see the lookup of the focus property causing a
problem here.  So while the patch in bug 305032 helps for the case when
arguments attachment confusing things, it seems that we still have a problem. 
Aaron, Brian, what window, if any, _should_ have focus when this is all done? 
It looks like we're focusing the toplevel XUL window here, which seems
reasonable all things considered...

Comment 12

[François Gagné]

>What if you replace "settingsHandle.focus();" with "var foo =
settingsHandle.focus"?  Is the bug still around?

No it works fine.

Comment 13

[Aaron Leventhal]

(In reply to comment #11)
> So for what it's worth, I do see the lookup of the focus property causing a
> problem here.  So while the patch in bug 305032 helps for the case when
> arguments attachment confusing things, it seems that we still have a problem. 
> Aaron, Brian, what window, if any, _should_ have focus when this is all done? 
> It looks like we're focusing the toplevel XUL window here, which seems
> reasonable all things considered...

It's a mistake to focus the top level XUL window itself. You want to focus the
first focusable control within it, which happens automatically at least for a
<dialog>.

I suppose we should fix it so that if you do focus a XUL window it makes sure
that one of its child controls is focused, and if not, focuses one.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> It's a mistake to focus the top level XUL window itself.

On our part, or adblock's?

> I suppose we should fix it so that if you do focus a XUL window it makes sure
> that one of its child controls is focused, and if not, focuses one.

Adblock is calling focus() before the XUL has loaded, so that's not an option in
this case....

Comment 15

[Asa Dotzler [:asa]]

not a blocker. let's look at the trunk for doing anything better here.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Could this help?

Blake, can you reproduce this bug?  If so, does this patch help?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197904 [details] [diff] [review]
Could this help?

This seems to work for me (I think I can reproduce and this patch seemed to fix
it). I'll review it either tonight or tomorrow.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197904 [details] [diff] [review]
Could this help?

r=mrbkap

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 197904 [details] [diff] [review]
Could this help?

sr=jst

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197904 [details] [diff] [review]
Could this help?

Requesting 1.8b5 approval.  This changes the behavior of window.focus() only
for toplevel chrome windows that have not yet loaded their XUL.  It makes the
behavior match what we had before splitwindow.	I think this should be pretty
safe, but I wouldn't bet my life on it... ;)

If this gets approved, could someone please check it in?  I won't be able to
until late tomorrow afternoon.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 197904 [details] [diff] [review]
Could this help?

blake, can you land this for BZ tonight?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in. MOZILLA_1_8_BRANCH and trunk.