814638 - swapFrameLoaders should not reset chrome event handler for chrome subframes

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Open debugger
2. Notice how Ctr+Shift+P brings up the operators panel
3. Undock the window
4. Press Ctrl+Shift+P, nothing happens anymore

Same goes for all the other keyboard shortcuts.

Comment 1

[Victor Porof [:vporof][:vp]]

Keyboard shorctuts have changed. Ctrl+Shift+P is now Accel+P.

Comment 2

[Paul Rouget [:paul]]

We have the same problem with the inspector (arrow keys in the markup panel). bug 818411

Comment 4

[Paul Rouget [:paul]]

Victor, can you tell me if you see any other missing shortcuts?

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Paul Rouget [:paul] from comment #4)
> Victor, can you tell me if you see any other missing shortcuts?

All shortcuts don't work after docking/undocking.

Comment 6

[Paul Rouget [:paul]]

webconsole.xul is probably affected too.

Comment 7

[Paul Rouget [:paul]]

About comment 2: no, it's not related.

Comment 8

[Paul Rouget [:paul]]

Actually, the shortcuts work if the focus is on the debugger toolbar buttons.

Comment 9

[Paul Rouget [:paul]]

Summary of the situation:

Each developer tool (WebConsole, Debugger, Inspector and StyleEditor) live in iframes. These 4 iframes are part of a same document: toolbox.xul. toolbox.xul lives in an iframe. This iframe is either located below the web page (so in browser.xul), or in a standalone window (toolbox-window.xul).

To "undock" the tools, we move the toolbox.xul document from the browser.xul iframe into the iframe in toolbox-window.xul. To do so, we use nsIFrameLoaderOwner.swapFrameLoaders.

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/Toolbox.jsm#397

Problem:

debugger.xul is composed of:
- a XUL toolbar
- an iframe (that hosts the code viewer, named SourceEditor)
  This bring us to:
  (browser.xul or toolbox-window.xul) > toolbox.xul > debugger.xul > dataURI
- a set of <xul:keys>
  For example "Accel + P" brings a popup

The bug is: after swapFrameLoaders is called (undocking the toolbox) if the focus is inside the dataURI iframe (code is focused) the <keys> don't work anymore

If the toolbar (in debugger.xul) is focused (click on one of the button for example), the keys work.

Comment 10

[Paul Rouget [:paul]]

I need some platform help here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hrm.  Swapping the toolbox.xul frameloader between elements shouldn't affect the interaction of debugger.xul and its subframe, I wouldn't think...

Neil, any idea what might be going on here with the <xul:keys>?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Hmm, does something odd happen with message managers.

Comment 13

[[e-mail]]

(In reply to Boris Zbarsky from comment #11)
> Hrm.  Swapping the toolbox.xul frameloader between elements shouldn't affect
> the interaction of debugger.xul and its subframe, I wouldn't think...
> 
> Neil, any idea what might be going on here with the <xul:keys>?

Not offhand; all that happens is that the document notices the keyset being added/removed from the document and asks the XBL service to add/remove the event handler appropriately.

If there is a bug in the handling of keysets in swapped frames it could be verified by opening about:config in a tab, dragging it into another window and seeing whether Alt+Shift+R still works.

Comment 14

[Paul Rouget [:paul]]

(In reply to [e-mail] from comment #13)
> If there is a bug in the handling of keysets in swapped frames it could be
> verified by opening about:config in a tab, dragging it into another window
> and seeing whether Alt+Shift+R still works.

about:config doesn't include any iframe afaik.

I'm working on a test case to, at least, confirm the problem.

Comment 15

[[e-mail]]

(In reply to Paul Rouget from comment #14)
> (In reply to comment #13)
> > If there is a bug in the handling of keysets in swapped frames it could be
> > verified by opening about:config in a tab, dragging it into another window
> > and seeing whether Alt+Shift+R still works.
> 
> about:config doesn't include any iframe afaik.

So the keys in toolbox.xul itself still work?

Comment 16

[Paul Rouget [:paul]]

Attachment: (restartless addon) test case

I've built a test case addon.

Browser source code on github: https://github.com/paulrouget/bug814638-test-case

STR:
- installing the addon brings 2 windows (the second one is below the first one)
- See windows "host1.xul" and "host2.xul"
- host1.xul includes about:config
- focus about:config (click on a pref), press "Accel + T", see the "42" alert.
- the <key> is not in about:config, but in the parent window (debugger.xul), that also includes a textbox)
- focus the textbox, press "Accel + T", see "42" again
- click "swap" to swap the iframes
- focus about:config, press "Accel + T", nothing happen
- focus the textbox, press "Accel + T", see "42"

So it appears that the key event is not crossing the inner iframe anymore.

If we re-swap (back to the original configuration), the problem is still present.

I hope that helps.

Comment 17

[Paul Rouget [:paul]]

(In reply to [e-mail] from comment #15)
> So the keys in toolbox.xul itself still work?

Yes. As long as the focus is not in an inner iframe.

Comment 18

[[e-mail]]

I don't know the history of swapFrameLoaders but I can imagine it was originally implemented for content frames and then tweaked to work with chrome frames. But chrome event handlers for chrome subframes don't need to be reset. Because swapFrameLoaders is resettting them, this means that keys in intermediate frames don't fire because the key event is bubbling up from the subframe to the parent frame without propagating through the swapped frame.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to [e-mail] from comment #18)
> I don't know the history of swapFrameLoaders but I can imagine it was
> originally implemented for content frames
IIRC, yes

> and then tweaked to work with
> chrome frames.
Was it ever really tested for chrome frames?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I can imagine it was originally implemented for content frames and then tweaked to work
> with chrome frames.

Yes.  It was originally done for tab D&D between windows, then we just loosened the restriction to allow it on chrome frames.  Testing for chrome frames was pretty minimal, yeah.

Neil, want to fix?

Comment 21

[[e-mail]]

Attachment: Draft patch

This might work, but I don't have time to (write a) test.

Comment 22

[Paul Rouget [:paul]]

(In reply to [e-mail] from comment #21)
> Created attachment 691942 [details] [diff] [review]
> Draft patch
> 
> This might work, but I don't have time to (write a) test.

I can test your patch and write a test if needed.

Comment 23

[Paul Rouget [:paul]]

Attachment: patch v1

Thank you Neil, your patch works.

Can you take a look at the test? It works here if run alone:
TEST_PATH=content/base/test/chrome/test_bug814638.xul make mochitest-chrome

But, I have to add 3 files to the Makefile. Only one of them is a test. The 2 other files need to be in the makefile to be available via a chrome:// url. But then, they are run as test (and the mochitests get stuck).

Comment 24

[[e-mail]]

Comment on attachment 693068 [details] [diff] [review]
patch v1

>+    test_bug814638_host.xul \
>+    test_bug814638_frame.xul \
I think you have to call the extra files file_bug814638_whatever.xul

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v1.1

Who can review this patch?

Comment 26

[Paul Rouget [:paul]]

Attachment: patch v1.2

Comment 27

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=0d0449318f00

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693327 [details] [diff] [review]
patch v1.2

r=me if:

1) A comment is added there saying that if ourType is chrome then the chrome
   event handler for subframes is already correct (and set to something in our
   document).

2) A comment is added in the ourType != otherType test that says that if we ever
   stop returning there we need to adjust this code accordingly.

Comment 29

[[e-mail]]

Sigh. My intention was to push the patch and test with different metadata, but Mercurial defeated me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6edf9f6b87
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9546d42054c

Comment 30

[Ed Morley [:emorley]]

Backed out for test_bug814638.xul timeouts:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9546d42054c

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a10ed19a32

Comment 31

[Paul Rouget [:paul]]

s/accel/control: https://tbpl.mozilla.org/?tree=Try&rev=af9fbf04835c

Comment 32

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/a0948a41a5d5

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/a0948a41a5d5