Last Comment Bug 310458 - [splitwindow] edit commands not available to extensions window (cut, copy, paste, select all, arrow keys navigation)
: [splitwindow] edit commands not available to extensions window (cut, copy, p...
Status: RESOLVED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major with 3 votes (vote)
: mozilla1.8beta5
Assigned To: Boris Zbarsky [:bz] (TPAC)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: splitwindows
  Show dependency treegraph
 
Reported: 2005-09-29 08:35 PDT by François Gagné
Modified: 2006-03-12 18:57 PST (History)
7 users (show)
asa: blocking1.8b5-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Could this help? (2.00 KB, patch)
2005-09-29 14:01 PDT, Boris Zbarsky [:bz] (TPAC)
mrbkap: review+
jst: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review

Description François Gagné 2005-09-29 08:35:17 PDT
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 2005-09-29 09:01:27 PDT
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é 2005-09-29 09:03:30 PDT
Additionnal detail, I'm talking about Adblock Plus:
http://www.extensionsmirror.nl/index.php?showtopic=774
Comment 3 Ria Klaassen (not reading all bugmail) 2005-09-29 09:11:10 PDT
I posted that already in Bug 220900 comment 134:
https://bugzilla.mozilla.org/show_bug.cgi?id=220900#c134
Comment 4 Boris Zbarsky [:bz] (TPAC) 2005-09-29 09:18:36 PDT
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 [:bz] (TPAC) 2005-09-29 09:22:02 PDT
If this is a regression, when did it regress?
Comment 6 Ria Klaassen (not reading all bugmail) 2005-09-29 09:32:02 PDT
(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 [:bz] (TPAC) 2005-09-29 09:36:02 PDT
OK... So can you try removing that focus() call and seeing what happens?
Comment 8 François Gagné 2005-09-29 09:51:45 PDT
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) 2005-09-29 10:00:15 PDT
(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 [:bz] (TPAC) 2005-09-29 10:35:15 PDT
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 [:bz] (TPAC) 2005-09-29 10:53:50 PDT
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é 2005-09-29 10:56:24 PDT
>What if you replace "settingsHandle.focus();" with "var foo =
settingsHandle.focus"?  Is the bug still around?

No it works fine.
Comment 13 Aaron Leventhal 2005-09-29 11:59:00 PDT
(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 [:bz] (TPAC) 2005-09-29 12:09:56 PDT
> 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] 2005-09-29 12:15:18 PDT
not a blocker. let's look at the trunk for doing anything better here.
Comment 16 Boris Zbarsky [:bz] (TPAC) 2005-09-29 14:01:26 PDT
Created attachment 197904 [details] [diff] [review]
Could this help?

Blake, can you reproduce this bug?  If so, does this patch help?
Comment 17 Blake Kaplan (:mrbkap) 2005-09-30 19:14:08 PDT
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) 2005-10-03 00:33:15 PDT
Comment on attachment 197904 [details] [diff] [review]
Could this help?

r=mrbkap
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-03 15:54:08 PDT
Comment on attachment 197904 [details] [diff] [review]
Could this help?

sr=jst
Comment 20 Boris Zbarsky [:bz] (TPAC) 2005-10-03 16:41:40 PDT
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] 2005-10-03 18:19:46 PDT
Comment on attachment 197904 [details] [diff] [review]
Could this help?

blake, can you land this for BZ tonight?
Comment 22 Blake Kaplan (:mrbkap) 2005-10-03 22:45:18 PDT
Fix checked in. MOZILLA_1_8_BRANCH and trunk.

Note You need to log in before you can comment on or make changes to this bug.