330710 - Replace obsolete preventBubble/preventCapture with stopPropagation

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Replace obsolete preventBubble/preventCapture with stopPropagation.
stopPropagation is the DOM way to stop event flow and 
preventBubble/preventCapture will be probably removed soon.

Comment 1

[Alex Vincent [:WeirdAl]]

Isn't this just a dupe, basically, of bug 105280?

Comment 2

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

No, I think. This is about replacing preventBubble / preventCapture calls in moz 
tree with stopPropagation calls. This is not about removing 
nsDOMEvent::PreventBubble etc.

Comment 3

[[e-mail]]

Fortunately we only have three .preventCapture()s but we have 184 LXR hits for .preventBubble()s, although skimming them I notice some that are unnecessary.

Comment 4

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

but I'll change all of them to stopPropagation.
Boring but easy.

That .preventBubble doesn't take account C++ callers.

Comment 5

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

Attachment: proposed patch

Neil, want to review? at least the xpfe part?

Comment 6

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

Comment on attachment 215416 [details] [diff] [review]
proposed patch

And perhaps mconnor could review toolkit part and rest.
How should I handle 1.8 branch? I think everything should be checked in to 1.8 too.

(Btw, changes to calendar/ have been done in separate bug.)

Comment 7

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

(In reply to comment #6)
> How should I handle 1.8 branch? I think everything should be checked in to 1.8
> too.
> 
Because otherwise adding a (js console) warning to nsDOMEvent::PreventBubble in bug 330494 doesn't make sense.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 215416 [details] [diff] [review]
proposed patch

r=me for the browser/toolkit parts

Comment 9

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

Comment on attachment 215416 [details] [diff] [review]
proposed patch

Perhaps jst could then review the rest.

Comment 10

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

(In reply to comment #9)
> (From update of attachment 215416 [details] [diff] [review] [edit])
> Perhaps jst could then review the rest.
> 
 I mean the content/ and layout/ changes

Comment 11

[[e-mail]]

Comment on attachment 215416 [details] [diff] [review]
proposed patch

>@@ -1024,24 +1021,21 @@ nsTextEditorFocusListener::Focus(nsIDOME
>   nsCOMPtr<nsIDOMEventTarget> target;
>   aEvent->GetTarget(getter_AddRefs(target));
>   if (!IsTargetFocused(target))
>     return NS_OK;
> 
>   // turn on selection and caret
>   if (mEditor)
>   {
>-    nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(aEvent));
>-    if (nsevent) {
>-      nsevent->PreventBubble();
>-    }
>+    aEvent->StopPropagation();
> 
>     PRUint32 flags;
>     mEditor->GetFlags(&flags);
>-    if (! (flags & nsIPlaintextEditor::eEditorDisabledMask))
>+    if (!(flags & nsIPlaintextEditor::eEditorDisabledMask))
Nit: gratuitous whitespaces changes confuse CVS blame

> function handleKeyPress(element, event)
> {
>   // allow dialog to close on enter if focused textbox has no value
>   if (element.value != "" && event.keyCode == 13)
>-    event.preventBubble();
>+    event.stopPropagation();
> }
This needs to be updated to event.preventDefault() because the dialog now listens to the second dispatch in the system event group.

> function OnReturnHit(event)
> {  
>   if (event.keyCode == 13) {
>     var focusedElement = document.commandDispatcher.focusedElement;
>     if (focusedElement && (focusedElement.id == "addressBucket"))
>       return;
>-    event.preventBubble();
>+    event.stopPropagation();
Again this should probably use event.preventDefault(); (note that I didn't test this one).

>     <command id="cmd_getMsgsForAuthAccounts" 
>-             oncommand="goDoCommand('cmd_getMsgsForAuthAccounts'); event.preventBubble()" 
>+             oncommand="goDoCommand('cmd_getMsgsForAuthAccounts'); event.stopPropagation()" 
This is one of many that are unnecessary, as there is no other listener.

>       if (event.originalTarget.localName == "treecol") { 
>         // clicking on the name column in the filter list should not sort
>-        event.preventBubble();
>+        event.stopPropagation();
>       }
This pattern happens a lot, I wonder why we don't have a central system.

>     <command id="Browser:AddGroupmarkAs" label="&addCurTabsAsCmd.label;"
>              accesskey="&addCurTabsAsCmd.accesskey;"
>-             oncommand="addGroupmarkAs(); event.preventBubble();"/>
>+             oncommand="addGroupmarkAs(); event.stopPropagation();"/>
Same as above again.

>           <menuitem id="viewCommandToolbar" type="checkbox" class="menuitem-iconic"
>                     label="&menuitem.view.command.toolbar.label;"
>                     accesskey="&menuitem.view.command.toolbar.accesskey;"
>-                    oncommand="goToggleToolbar('command-toolbar', 'viewCommandToolbar'); event.preventBubble();"
>+                    oncommand="goToggleToolbar('command-toolbar', 'viewCommandToolbar'); event.stopPropagation();"
>                     checked="true"/>
>           <menuitem id="viewCommandSearchbar" type="checkbox" class="menuitem-iconic"
>                     label="&menuitem.view.command.searchbar.label;"
>                     accesskey="&menuitem.view.command.searchbar.accesskey;"
>-                    oncommand="goToggleToolbar('bookmarks-search', 'viewCommandSearchbar'); event.preventBubble();"
>+                    oncommand="goToggleToolbar('bookmarks-search', 'viewCommandSearchbar'); event.stopPropagation();"
>                     checked="true"/>
These ones look unnecessary too.

>       <handler event="keypress" keycode="VK_UP" phase="target">
>         this.checkAdjacentElement(false);
>-        event.preventBubble();
>+        event.stopPropagation();
>       </handler>
>       <handler event="keypress" keycode="VK_LEFT" phase="target">
>         this.checkAdjacentElement(false);
>-        event.preventBubble();
>+        event.stopPropagation();
>       </handler>
>       <handler event="keypress" keycode="VK_DOWN" phase="target">
>         this.checkAdjacentElement(true);
>-        event.preventBubble();
>+        event.stopPropagation();
>       </handler>
>       <handler event="keypress" keycode="VK_RIGHT" phase="target">
>         this.checkAdjacentElement(true);
>-        event.preventBubble();
>+        event.stopPropagation();
>       </handler>
I wonder why these exist, given the phase="target".

r=me but feel free to implement the suggestions before checking in.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 215416 [details] [diff] [review]
proposed patch

sr=jst

Comment 13

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

Attachment: to be checked in (trunk)

Comment 14

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

Attachment: for 1.8

I think we want the same fixes for 1.8, so that the preventBubble warnings can be 
turned on in Bug 330494.
Does this patch need reviews? It is basically the same as for trunk.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 216254 [details] [diff] [review]
for 1.8

Let's wait a few days before we look to land this on branch after the trunk landing.  Also, I'd like to hear from the Gecko people about branch-safety.  I'm fine with taking the changes if we're adding the "we're going to deprecate and remove preventBubble" js console warning to the branch.

Comment 16

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

Comment on attachment 216252 [details] [diff] [review]
to be checked in (trunk)

This was checked in

Comment 17

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

Attachment: this was missing from the trunk patch

This was missing from the trunk patch. Checked in.
Now trunk shouldn't contain any preventBubble or preventCapture calls
and Bug 330494 can be fixed soon.

Comment 18

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

(In reply to comment #15)
> (From update of attachment 216254 [details] [diff] [review] [edit])
> Let's wait a few days before we look to land this on branch after the trunk
> landing.  Also, I'd like to hear from the Gecko people about branch-safety. 
> I'm fine with taking the changes if we're adding the "we're going to deprecate
> and remove preventBubble" js console warning to the branch.
> 

Haven't heard about any regressions. (which is not a surprise since
stopPropagation does the same thing as preventBubble)
This should be safe and I'll add the JS Console message about deprecation of preventBubble in bug 330494 (for branch preventBubble will still work, no-op
only for trunk).

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 216254 [details] [diff] [review]
for 1.8

woo.

Comment 20

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

Comment on attachment 216254 [details] [diff] [review]
for 1.8

Checked in to branch

Comment 21

[Bryan]

Could this checkin break menu items?  Before downloading a zip that only contains this patch every in FF is fine.  After downloading a zip that contains this patch I no longer have menu items such as File->, Edit->, Edit->.  Meaning when I click on those menu items I get no submenu.  I get no right-click menu item either.

BUILD: 2006040702

Bryan

Comment 22

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

(In reply to comment #21)
> Could this checkin break menu items?

Where did you get 2006040702? And you're really testing 1.8 branch, right?
And on Windows, I suppose. (Linux is ok)

Comment 23

[Bryan]

(In reply to comment #22)
> (In reply to comment #21)
> > Could this checkin break menu items?
> 
> Where did you get 2006040702? And you're really testing 1.8 branch, right?
> And on Windows, I suppose. (Linux is ok)

I grabbed it this AM and yes I'm testing branch!  I posted to the wrong bug though, it's this bug that caused the issue: https://bugzilla.mozilla.org/show_bug.cgi?id=332640

Sorry for the accidental spam...  :-(

Bryan