204777 - Handling of VK_BACK to go back needs to move out of platformHTMLBindings.xml

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Simon Fraser [no longer active]]

Because we handle VK_BACK in platformHTMLBindings.xml, mapping it to
cmd_browserBack, embedders who just use gecko for mail display are finding that
hitting the backspace key results in showing another page.

We need to rip the VK_BACK handling out of platformHTMLBindings.xml, and move it
up to the xpfe level.

Comment 1

[Aaron Leventhal]

Attachment: Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

Comment 2

[Kathleen :Brade]

Comment on attachment 123389 [details] [diff] [review]
Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

I'm not sure we really want to get rid of the commands in
nsGlobalWindowCommands.cpp

Comment 3

[Aaron Leventhal]

We need to get rid of those. I originally added those to nsGlobalWindow.cpp so
that I could handle them in platformHTMLBindings.xml. Since we're getting rid of
that, we don't need the extra bloat anymore.

This lxr search shows that the only place we use it is in platformHTMLBindings,
and we're removing it there:
http://lxr.mozilla.org/seamonkey/search?string=cmd_browserback

Comment 4

[Simon Fraser [no longer active]]

Looks like the correct approach, but please leave the C++ commands in
nsGlobalWindowCommands.cpp, since they could be useful to someone.

Comment 5

[Aaron Leventhal]

Why would they be useful when the documented approach for embeddors is
nsIWebNavigation? Aren't we just keeping around bloat we don't need?

Comment 6

[Simon Fraser [no longer active]]

It's part of my long-term evil plan to eliminate a bunch of embedding APIs in
favor of commands. But, OK, nuke them for now.

Comment 7

[Kathleen :Brade]

Comment on attachment 123389 [details] [diff] [review]
Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

Please #ifdef the commands in nsGlobalWindowCommands.cpp/.h

Please reformat the comments in idl.

Part of me feels like we're missing a "consumeEvent" call somewhere.

Comment 8

[Aaron Leventhal]

> Looks like the correct approach, but please leave the C++ commands in
> nsGlobalWindowCommands.cpp, since they could be useful to someone.

Simon, does this mean you're ready to plunk sr= on it?

Comment 9

[Simon Fraser [no longer active]]

If you respond to kathy's comments I will.

Comment 10

[Aaron Leventhal]

Kathy and Simon, I have reformatted the idl to use JavaDoc style.
I will #ifdef the commands in nsGlobalWindow.cpp.
What do you mean we're missing a ConsumeEvent call?

Comment 11

[Aaron Leventhal]

I spoke with Kathy on IRC, we agreed that the consumeEvent happens via the XUL
command stuff, so that should take care of her comments.

Also note that if embeddors want both backspace behaviors, they will have to use
the typeaheadfind service BackOneChar() method to see if it wants to use the
backspace first, and if not, go back in history.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 123389 [details] [diff] [review]
Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

This patch needs to also remove the builtin bindings here:

http://lxr.mozilla.org/seamonkey/source/content/xbl/builtin/win/platformHTMLBin
dings.xml#111

Also, is TAF a frozen embedding API? I'd expect most embedders not to use it,
but if they do, and have to call into it to get proper back behaviour, we need
to freeze it, or make a better way (a command?).

Comment 13

[Aaron Leventhal]

Comment on attachment 123389 [details] [diff] [review]
Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

> This patch needs to also remove the builtin bindings here:
> http://lxr.mozilla.org/seamonkey/source/content/xbl/builtin/win/platformHTMLBin
> dings.xml#111
This patch does that, please see the last file in it.

> Also, is TAF a frozen embedding API? I'd expect most embedders not to use it,
> but if they do, and have to call into it to get proper back behaviour, we need
> to freeze it, or make a better way (a command?).
We can do that in another bug

Comment 14

[Aaron Leventhal]

Filed b ug 205953 for freezing nsITypeAheadFind.

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 123389 [details] [diff] [review]
Handle backspace in xpfe, use typeaheadfind service to check if it needs the key before using it to go back in history

Gotcha; sorry i missed that last part of the patch.

Comment 16

[saari (gone)]

topembed+

Comment 17

[jag (Peter Annema)]

Simon: the way I read this, iff you use TAF, you should call into TAF first to
see if it wants to handle the backspace key, and if not, you call whatever
default action you have (in this case going back a page).

Aaron:

I'm slightly confused by your code:

   // ----------- Back space -------------------------
   if (keyCode == nsIDOMKeyEvent::DOM_VK_BACK_SPACE) {
+    PRBool backspaceUsed;
+    BackOneChar(&backspaceUsed);
+    if (backspaceUsed) {
       aEvent->PreventDefault(); // Prevent normal processing of this keystroke
     }

If we have that, then why do we need this:

+function BrowserMultiPurposeBackspace()
+{
+  // When backspace is pressed, it might mean back
+  // in typeaheadfind if that's active, or it might mean back in history
+
+  var typeAhead =
Components.classes["@mozilla.org/typeaheadfind;1"].getService(Components.interfaces.nsITypeAheadFind);
+  
+  if (!typeAhead || !typeAhead.backOneChar()) {
+    BrowserBack();
+  }
+}

Are these triggered in two separate cases? How come it wasn't needed before?

If they are needed, could you call these BrowserHandleBackspace and
cmd_handleBackspace?

Also, if TAF isn't installed, the getService line will throw an exception.
Either try/catch it, or do something like

const TYPE_AHEAD_FIND_CONTRACTID = "@mozilla.org/typeaheadfind;1";
if (TYPE_AHEAD_FIND_CONTRACTID in Components.classes) {
  var typeAhead = Components.classes[TYPE_AHEAD_FIND_CONTRACTID]
                            .getService(Components.interfaces.nsITypeAheadFind);
  ...
}

Comment 18

[Aaron Leventhal]

Jag, I can fix the exception before checking in.

I can also add a couple of comments as to why there are two different handlings
of backspace for typeaheadfind. One is for when there is XUL, in that case there
is a XUL command for backspace. The other is for when there is an embedded
situation with typeaheadfind, and the backspace makes it to the typeaheadfind
handler.

Comment 19

[jag (Peter Annema)]

Aaron: so if embedding already has a way of intercepting backspace (if TAF is
installed) and dealing with it correctly, why do we need to special case XUL?
Based on the old code I'm under the impression that we don't need to special
case XUL.

Comment 20

[Aaron Leventhal]

Jag, the problem is that the order of seeing the keystrokes is this:

1) XUL
2) Typeahead
3) platformHTMLBindings.xml

In the past, if typeahead didn't want the backspace it could let it fall through
to platformHTMLBindings.xml, and let it use it for going back in history.

Because of of the problem in this bug, I had to move the back in history command
out of platformHTMLBindings. This means that typeaheadfind could no longer just
allow the backspace to pass through if it didn't need it.

Now for XUL clients we have to check with typeahead first to see if it needs it,
and handle it if typeahead doesn't need it. Or, is there another way to have a
backspace command in XUL but give typeaheadfind the first opportunity to use it?

Comment 21

[jag (Peter Annema)]

Aaron: okay, that makes sense. Yeah, a comment would be good, change the
multipurpose to handle, and add the try/catch and attach the new patch.

Comment 22

[Aaron Leventhal]

Attachment: New patch for Jag's comments

Comment 23

[Aaron Leventhal]

Comment on attachment 123913 [details] [diff] [review]
New patch for Jag's comments

Carrying Brade's r=

Comment 24

[jag (Peter Annema)]

Comment on attachment 123913 [details] [diff] [review]
New patch for Jag's comments

sr=jag

Comment 25

[Brian Ryner (not reading)]

Uh, this seems pretty hacky to me.  How about guaranteeing that typeahead find
sees the event first by attaching it earlier in the event flow?

Comment 26

[Aaron Leventhal]

Bryner, how can that be done in a way that's consistent and not fragile? The
current setup guarantees that no matter who gets the event first it works
correctly. Relying on the order of event handlers seems fragile to me, unless
there is a way to guarantee that we're before someone else.

Comment 27

[Brian Ryner (not reading)]

But what you checked in is already making assumptions that it shouldn't about
event firing order.  In particular, typeahead find and the XUL <key> handlers
are both attached as bubbling keypress listeners on the toplevel window in the
system event group.

So, to guarantee that typeahead find sees the event first, you would need to
either move TAF earlier or move the XUL key handlers later.  In the case of a
XUL window, it seems like TAF doesn't need to be on the toplevel window, it
could attach to the <browser> element or something.  Of course for embedding,
you'd want it on the toplevel DOM window.  If you could move TAF down to be a
bubbling handler on the <browser> in the system event group, that would
definitely solve this.

Another possibility involves something that I've thought about a bit - creating
a third event group for "application" event listeners, to separate them from
gecko's internal event listeners.  So events would be dispatched to groups in
this order:

1. default group   (event listeners attached from content)

2. system or "gecko" group   (event listeners used by gecko itself - this would
include editor and typeahead find)

3. chrome or "application" group  (event listeners doing something specific to a
certain XUL or embedding application - this would include the backspace handler)

Comment 28

[Aaron Leventhal]

What I checked in works no matter who gets the event first, that's why I think
it can stay for the moment. If you think I should back it out, then I'm not sure
exactly what to do.

Many times I've had to change how typeahead gets events, to fix all kinds of
annoying problems. The current system is the only one that worked. I'm sorry
that I don't remember exactly what all the problems were, but some of them were
related to text entry, and some were related to not being able to have a web
page catch the event before we do. I'd have to go back through the fixed bugs to
see what all the potential regressions are.

I like the last possibility that you outline, and I think we should do it. Then
we can remove the unneccesary use of nsITypeAheadFind. You understand the event
system much better than I do, so if there is to be some cleaner fix, I trust you
more than myself to do it.

See also bug 207175 which is this same fix for Phoenix.

Comment 29

[Aaron Leventhal]

Checked into 1.4 branch as well

Comment 30

[sairuh (rarely reading bugmail)]

for the past couple of weeks since this has been checked in, i haven't noticed
any obvious regression on the 1.4 branch (all platforms), so marking verified1.4.