Closed Bug 310458 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.8beta5

People

(Reporter: frenchfrog, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8, regression)

Attachments

(1 file)

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.
Blocks: splitwindows
Keywords: regression
Summary: [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, paste, select all, arrow keys navigation)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Additionnal detail, I'm talking about Adblock Plus:
http://www.extensionsmirror.nl/index.php?showtopic=774
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?
If this is a regression, when did it regress?
(In reply to comment #5)
> If this is a regression, when did it regress?

1.8b4_2005073013 - 1.8b4_2005073111 (splitwindow indeed).
OK... So can you try removing that focus() call and seeing what happens?
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.
(In reply to comment #7)
> OK... So can you try removing that focus() call and seeing what happens?
Yess! It helps.
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...
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...
Flags: blocking1.8b5?
>What if you replace "settingsHandle.focus();" with "var foo =
settingsHandle.focus"?  Is the bug still around?

No it works fine.
(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.
> 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....
not a blocker. let's look at the trunk for doing anything better here.
Flags: blocking1.8b5? → blocking1.8b5-
Attached patch Could this help?Splinter Review
Blake, can you reproduce this bug?  If so, does this patch help?
Attachment #197904 - Flags: review?(mrbkap)
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 on attachment 197904 [details] [diff] [review]
Could this help?

r=mrbkap
Attachment #197904 - Flags: review?(mrbkap) → review+
Attachment #197904 - Flags: superreview?(jst)
Comment on attachment 197904 [details] [diff] [review]
Could this help?

sr=jst
Attachment #197904 - Flags: superreview?(jst) → superreview+
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.
Attachment #197904 - Flags: approval1.8b5?
Comment on attachment 197904 [details] [diff] [review]
Could this help?

blake, can you land this for BZ tonight?
Attachment #197904 - Flags: approval1.8b5? → approval1.8b5+
Fix checked in. MOZILLA_1_8_BRANCH and trunk.
Assignee: general → bzbarsky
Keywords: fixed1.8
Target Milestone: --- → mozilla1.8beta5
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.