1074971 - [e10s] Switching, opening, and closing tabs doesn't work while a tab is loading

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

If you open a URL that takes a while to load (e.g. some youtube one for me) then while the spinner is active pressing Cmd+T/W doesn't do anything. You can click a tab to switch to it but the browser pane won't actually switch.

It seems like we don't process events or messages at all?

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

When switching tabs the content pane becomes white even.

Comment 2

[Jim Mathies [:jimm]]

Sounds like a dupe of bug 1067523, although that one got resolved because the site changed.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We route keystrokes through the content process before chrome gets a chance to use them. If the content process is busy, then we won't respond to keyboard shortcuts. I think it probably makes sense to avoid sending shortcuts like Ctrl-T/W to content since we probably don't want a web page to be able to intercept those anyway.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

That sounds like a solution to bug 1052569, and also an opening of the bug 380637 can of worms (it will break some web apps in some cases).

Comment 5

[Jonathan Howard]

"You can click a tab to switch to it but the browser pane won't actually switch."
dom.ipc.processCount works well.

Repeating part of comment I just added in bug 1101138. Some keys work, at least Ctrl+Tab and Alt+f.

My hunch [2nd time referring to this] for single processCount fixing bug 666365 may help reduce delay; "the browser pane won't actually switch."

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just for my own memory, the Chromium function that does this is IsReservedCommandOrKey. Useful to see how they do it.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/browser_command_controller.cc&q=isreservedcom&sq=package:chromium&type=cs&l=259

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Felipe, would you mind taking this over? It's very closely related to bug 862519.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Sure

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Allow support for reserved commands

In bug 862519 we added e10s support to commands, by allowing the key events to first be dispatched to content, wait for a response, and then process them in the parent with the correct preventedDefault flag.

This adds support to add an attribute (reserved="true") to commands, which will skip that support, making them non overridable by web content. This is helpful in that, when the content is busy, we don't want to wait to process these commands.

The list of reserved commands will be kept short, and as much as possible I think we should mirror Chrome's list of reserved keys (comment 7 has a link to that list) in order to avoid web compat issues.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Reserve main browser commands

As described in comment 4, this should fix bug 1052569 and open the non-overridable commands can of worms.. This reserves:

New Tab
Close Tab
New Window
Close Window
Next Tab
Prev Tab
Quit
New Private Browsing

Which is a conservative list, and a subset of what Chrome reserves, so hopefully web compat issues would be minimal with these. Note however that their Private Browsing shortcut is different than ours, so that is the one more at risk.

This also makes it easy for a simple add-on to reserve more keys like Ctrl+F, which a lot of people want but it's probably harder to argue for it as a default.

Comment 12

[Marco Mucci [:MarcoM]]

Hi Felipe, can you provide a point value.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8573310 [details] [diff] [review]
Reserve main browser commands

Seems worth trying. Philipp might have feedback on the list of shortcuts to block.

Comment 14

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

Comment on attachment 8573306 [details] [diff] [review]
Allow support for reserved commands

This makes HasHandlerForEvent to behave rather oddly.
It doesn't check anymore whether there are handlers for the event, but whether
there are non-reserved handlers for the event, whatever that means.

So, change the name of HasHandlerForEvent at least
And I think "reserved" doesn't quite describe what is happening here, since it is
more like "dont-let-content-process-to-override-the-behavior-but-stuff-in-chrome-can-still-do-that". Could we call the attribute chromeOnlyHandling="true" ?

If we want to make chrome to always execute these handlers, shouldn't we prevent
content to even get the relevant key events. So I'd expect mNoCrossProcessBoundaryForwarding to be set somewhere.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8573310 [details] [diff] [review]
Reserve main browser commands

If that's a subset of what Chrome reserves, it should be fine. I'm slightly concerned about new private window since the P key might be used for print related stuff. But we can include it for now and then remove it later if lots of apps break.

Out of interest: does IE also block certain key commands?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Attachment: Allow support for reserved commands, v2

Ok, this is a much cleaner version of the patch. I was abusing the aExecute param to mean two things at the same time, but now instead I just added a new outParam to HasHandlerForEvent to optionally query if the handler is reserved. With this I think the HasHandlerForEvent name can remain unchanged.

I also set the mNoCrossProcessBoundaryForwarding as requested to make it so that content doesn't even receive it. I had left it out deliberately ("ah, might as well let the page at least receive the event"), but yeah that would be strange behavior.

I just wanted to push back on the name of the attribute. I believe "reserved" in this context makes sense, since it's declared with the XUL command handlers, it's expected that chrome can still do whatever it wants with the event, it's just reserved from being exposed to content. (And it looks a bit cleaner than "reservedForChrome" in the file). But if you still think it's not a good name I can change it when landing.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/7c05621a38a2
https://hg.mozilla.org/integration/fx-team/rev/a5c70a2fab90

Comment 18

[:Felipe Gomes (needinfo for replies!)]

This added a new attribute for XUL <command> elements, reserved="true", which makes the key event for that command to not be forwarded to content, making it impossible for content to see or preventDefault those commands. We're currently using it for Ctrl+T, +W, +N, etc.  (Full list at comment 11)

This is an e10s-only feature.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c05621a38a2
https://hg.mozilla.org/mozilla-central/rev/a5c70a2fab90

Comment 20

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 36.0a1 (2014-11-18) using Ubuntu 12.04 64bit and Windows 7 64bit.

Verified fixed on Firefox 39.0a1 (2015-03-16) using Ubuntu 12.04 64 bit, Windows 7 64bit and Mac OS X 10.9.5.

Comment 21

[Will Bamberg [:wbamberg]]

I've added a note to Site Compat for Firefox 39:
https://developer.mozilla.org/en-US/Firefox/Releases/39/Site_Compatibility#Reserved_browser_operations

And a page on the new XUL attribute:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/reserved

Please let me know if this is correct and covers things.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Looks good!

Comment 23

[Matt Giuca]

I'm on the Google Chrome team and we are now thinking that sites should be allowed to preventDefault on keyboard shortcuts while in fullscreen mode (though we haven't implemented yet). Edge already does this. Wondering if Firefox team would be willing to change this behaviour, and if the reason for changing to a non-interceptable model was a policy decision or technical limitation (as suggested by this bug).

Am I right in understanding that the key combinations on the reserved list (which aren't delivered to the site) are the only ones that can be used while the site is loading? (So there is an architectural limitation to allowing sites to capture the events, not just a policy decision.)