Closed
Bug 330710
Opened 19 years ago
Closed 19 years ago
Replace obsolete preventBubble/preventCapture with stopPropagation
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
()
Details
(Keywords: fixed1.8.1)
Attachments
(4 files)
137.27 KB,
patch
|
neil
:
review+
mconnor
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
137.22 KB,
patch
|
Details | Diff | Splinter Review | |
142.25 KB,
patch
|
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
3.38 KB,
patch
|
Details | Diff | Splinter Review |
Replace obsolete preventBubble/preventCapture with stopPropagation. stopPropagation is the DOM way to stop event flow and preventBubble/preventCapture will be probably removed soon.
Assignee | ||
Comment 2•19 years ago
|
||
No, I think. This is about replacing preventBubble / preventCapture calls in moz tree with stopPropagation calls. This is not about removing nsDOMEvent::PreventBubble etc.
Blocks: 105280
Comment 3•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
but I'll change all of them to stopPropagation. Boring but easy. That .preventBubble doesn't take account C++ callers.
Assignee | ||
Comment 6•19 years ago
|
||
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.)
Attachment #215416 -
Flags: review?(mconnor)
Assignee | ||
Comment 7•19 years ago
|
||
(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•19 years ago
|
||
Comment on attachment 215416 [details] [diff] [review] proposed patch r=me for the browser/toolkit parts
Attachment #215416 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 215416 [details] [diff] [review] proposed patch Perhaps jst could then review the rest.
Attachment #215416 -
Flags: superreview?(jst)
Assignee | ||
Comment 10•19 years ago
|
||
(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•19 years ago
|
||
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.
Attachment #215416 -
Flags: review?(neil) → review+
Comment 12•19 years ago
|
||
Comment on attachment 215416 [details] [diff] [review] proposed patch sr=jst
Attachment #215416 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 13•19 years ago
|
||
Assignee | ||
Comment 14•19 years ago
|
||
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.
Attachment #216254 -
Flags: approval-branch-1.8.1?
Assignee | ||
Updated•19 years ago
|
Attachment #216254 -
Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(mconnor)
Comment 15•19 years ago
|
||
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.
Attachment #216254 -
Flags: approval-branch-1.8.1?(mconnor)
Assignee | ||
Comment 16•19 years ago
|
||
Comment on attachment 216252 [details] [diff] [review] to be checked in (trunk) This was checked in
Assignee | ||
Comment 17•19 years ago
|
||
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.
Assignee | ||
Comment 18•19 years ago
|
||
(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).
Assignee | ||
Updated•19 years ago
|
Attachment #216254 -
Flags: approval-branch-1.8.1?(mconnor)
Comment 19•19 years ago
|
||
Comment on attachment 216254 [details] [diff] [review] for 1.8 woo.
Attachment #216254 -
Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Assignee | ||
Comment 20•19 years ago
|
||
Comment on attachment 216254 [details] [diff] [review] for 1.8 Checked in to branch
Assignee | ||
Updated•19 years ago
|
Comment 21•19 years ago
|
||
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
Assignee | ||
Comment 22•19 years ago
|
||
(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•19 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•