swapFrameLoaders should not reset chrome event handler for chrome subframes

RESOLVED FIXED in mozilla20

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: vporof, Assigned: paul)

Tracking

unspecified
mozilla20
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: 788977
Blocks: 816946
Keyboard shorctuts have changed. Ctrl+Shift+P is now Accel+P.
We have the same problem with the inspector (arrow keys in the markup panel). bug 818411
No longer blocks: 788977
Victor, can you tell me if you see any other missing shortcuts?
(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.
Component: Developer Tools: Debugger → Developer Tools
Summary: [toolbox] Keyboard shortcuts don't work after docking or undocking the window → [toolbox] Keyboard shortcuts (<keys>) don't work after using swapFrameLoaders
webconsole.xul is probably affected too.
About comment 2: no, it's not related.
Actually, the shortcuts work if the focus is on the debugger toolbar buttons.
Summary: [toolbox] Keyboard shortcuts (<keys>) don't work after using swapFrameLoaders → [toolbox] Keyboard shortcuts (<keys>) don't bubble up after using swapFrameLoaders
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.
I need some platform help here.
Whiteboard: [See comment 9]
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>?
Hmm, does something odd happen with message managers.
Priority: -- → P1
(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.
(In reply to neil@parkwaycc.co.uk 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.
(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?
Posted file (restartless addon) test case (obsolete) —
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.
(In reply to neil@parkwaycc.co.uk from comment #15)
> So the keys in toolbox.xul itself still work?

Yes. As long as the focus is not in an inner iframe.
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.
Component: Developer Tools → Document Navigation
Product: Firefox → Core
Summary: [toolbox] Keyboard shortcuts (<keys>) don't bubble up after using swapFrameLoaders → swapFrameLoaders should not reset chrome event handler for chrome subframes
(In reply to neil@parkwaycc.co.uk 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?
> 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?
Posted patch Draft patch (obsolete) — Splinter Review
This might work, but I don't have time to (write a) test.
(In reply to neil@parkwaycc.co.uk 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.
Posted patch patch v1 (obsolete) — Splinter Review
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).
Attachment #691801 - Attachment is obsolete: true
Attachment #691942 - Attachment is obsolete: true
Attachment #693068 - Flags: feedback?(neil)
Whiteboard: [See comment 9] → [has-patch]
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
Attachment #693068 - Flags: feedback?(neil)
Posted patch patch v1.1 (obsolete) — Splinter Review
Who can review this patch?
Attachment #693068 - Attachment is obsolete: true
Attachment #693326 - Flags: review?
Posted patch patch v1.2Splinter Review
Attachment #693326 - Attachment is obsolete: true
Attachment #693326 - Flags: review?
Attachment #693327 - Flags: review?
Whiteboard: [has-patch] → [needs-review]
Attachment #693327 - Flags: review? → review?(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.
Attachment #693327 - Flags: review?(bzbarsky) → review+
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
s/accel/control: https://tbpl.mozilla.org/?tree=Try&rev=af9fbf04835c
Whiteboard: [needs-review]
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a0948a41a5d5
Assignee: nobody → paul
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.