Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cancelBubble legacy property #211

Closed
cvrebert opened this issue Apr 8, 2016 · 48 comments
Closed

cancelBubble legacy property #211

cvrebert opened this issue Apr 8, 2016 · 48 comments

Comments

@cvrebert
Copy link
Member

cvrebert commented Apr 8, 2016

UIEvent.cancelBubble works in all modern browsers.
Testcase: http://jsbin.com/jufexu/edit?html,css,js,output

Event.cancelBubble works in all modern browsers (Chrome, Safari, Edge, IE11) except Firefox.
Testcase: http://jsbin.com/wirelo/edit?html,js,output

To accurately reflect reality, I propose speccing at least one of these properties.

@annevk
Copy link
Member

annevk commented Apr 9, 2016

I think we should just define Event.cancelBubble then. Seems the shortest path towards interoperability and it's not really a big deal since it just reflects the stop propagation flag as far as I can tell.

@smaug----, objections?

@smaug----
Copy link
Collaborator

In Gecko UIEvent has that indeed and it sets the propagation stopped flag. This is really ancient stuff.
I thought we could have removed it, like to removed couple of similar properties few years ago.
IIRC, this is Netscapeism.

Looks like the property works in a bit different way in different browsers. In Gecko it maps to
stopPropagation() and you can effectively un-stopPropagation() with cancelBubble = false.
In Blink you can also cancel, but only cancelBubble = true, not stopPropagation().

@smaug----
Copy link
Collaborator

but yeah, ok to spec, although it is ancient bad API.

@annevk
Copy link
Member

annevk commented Apr 9, 2016

Hmm, yeah, in Blink's implementation this is its own primitive (and it seems you can set that back to false). Not sure we want it to be that complicated. @foolip, @RByers, thoughts?

@smaug----
Copy link
Collaborator

oh, it is also old IEism. Totally forgot. IE didn't use to have stopPropagation(), only cancelBubble

@RByers
Copy link
Contributor

RByers commented Apr 9, 2016

Our usage stats suggest that it's still pretty popular (though falling). This will be tainted somewhat by event copying code, but 4% of page views is huge compared to what we normally see for event copying (eg. 0.2% for webkitMovementX before we removed it). I Get a ton of hits for "cancelBubble" in httpArchive.

So yeah we're unlikely to be able to remove it. But if Gecko just has it mapped to stopPropagation then perhaps that would be web compatible for us (certainly simpler). What does Edge do? We might need to add UseCounters measuring how often the difference matters (eg. how often it's set back to false, or set from within capturing handlers while there are still other capturing handlers to run).

That said, if IE, Edge, WebKit and blink are all consistent here already, maybe it's best to just define it exactly as it's implemented in those browsers. I guess it's not completely crazy to think that some capturing handler may want to prevent all bubbling without preventing the rest of the capture or target phases.

@tkent-google any thoughts?

@foolip
Copy link
Member

foolip commented Apr 11, 2016

In Blink it's a separate internal slot, and used in two places:
https://chromium.googlesource.com/chromium/src/+/aede1d0909b3f905c74c769e610b903015f50569/third_party/WebKit/Source/core/events/EventDispatcher.cpp#179
https://chromium.googlesource.com/chromium/src/+/4e4da03f08faa602998125895ac4b884c60be001/third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.cpp#36

@inexorabletash, any idea why there's special event dispatch code for IndexedDB?

If we end up having to specify something by the name cancelBubble it would be strange if it affected things outside the bubbling phase, but it might be worth measuring how often cancelBubble is actually set to true. @RByers, could you do a quick httparchive query to see how it's usually used to see if that's even worth investigating?

@annevk
Copy link
Member

annevk commented Apr 11, 2016

@foolip he mentioned in the above comment that there's a ton of hits.

@annevk
Copy link
Member

annevk commented Apr 11, 2016

Also, IndexedDB likely has special dispatch code because Blink's implementation of event paths is not generic I believe.

@foolip
Copy link
Member

foolip commented Apr 11, 2016

Ah, OK, then I change my question to "what do the hits look like"? I guess most of them set it to true because that's the only useful thing one can do with the API?

@smaug----
Copy link
Collaborator

FWIW, looks like there was code in Gecko which tried to allow changing cancelBubble flag only during target/bubble phase[1], but it was apparently never working, if I trust my own comment from 10 years ago https://bugzilla.mozilla.org/show_bug.cgi?id=330494#c3
(Gecko used to have super hard to maintain and understand event dispatch code before Gecko 1.9, so, around that 10 years ago.)

[1] I think this would be to mimic old IE. IIRC, it has effectively just target and bubble phases.

@RByers
Copy link
Contributor

RByers commented Apr 11, 2016

Ah, OK, then I change my question to "what do the hits look like"? I guess most of them set it to true because that's the only useful thing one can do with the API?

I'm swamped right now and can't dig in deeply, but here's the first 5000 results. Looks like jQuery, mootools some twitter libraries are major users. Want to spot check some of these and let us know what you find?

@foolip
Copy link
Member

foolip commented Apr 12, 2016

I opened 10 of them "at random" and took a quick look. Mostly it's used as a fallback when stopPropagation doesn't exist, but those cases wouldn't show up in use counter data.

In https://maps.googleapis.com/maps/api/js and http://w.sharethis.com/share5x/js/stcommon.2b05accc08f1ee42fe1983647ab923ea.js and multiple other scripts it's used together with stopPropagation which would show up in use counters but removing it probably wouldn't be observable.

In http://www.speedup.ir/angular/app/scripts/vendor/jquery.nicescroll.js there are many uses, most similar to the above but one that's gated on document.fireEvent:

      else if (document.fireEvent) {
        this.notifyScrollEvent = function(el) {
          var e = document.createEventObject();
          el.fireEvent("onscroll");
          e.cancelBubble = true; 
        };
      }

As long as Edge has removed document.fireEvent, that should be dead code everywhere.

In http://kolelnasmag.com/wp-content/themes/Fikra%20Mag3/js/uisearch.js there's an addEventListener polyfill which should also be dead code now.

In https://static.xx.fbcdn.net/rsrc.php/v2iaFb4/yH/l/en_US/0bcWBIJHikk.js @domenic makes an appearance :)

/**
 * @generated SignedSource<<4c94898b765e9ddb7bac02513e528723>>
 *
 * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 * !! This file is a check-in of a static_upstream project!      !!
 * !!                                                            !!
 * !! You should not modify this file directly. Instead:         !!
 * !! 1) Use `fjs use-upstream` to temporarily replace this with !!
 * !!    the latest version from upstream.                       !!
 * !! 2) Make your changes, test them, etc.                      !!
 * !! 3) Use `fjs push-upstream` to copy your changes back to    !!
 * !!    static_upstream.                                        !!
 * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 *
 * Copyright (c) 2012 Barnesandnoble.com, llc, Donavon West, and Domenic
 * Denicola

Anyway, in that file there's a bit of code that I saw in a few places, seems like this is the unminified code:
https://github.com/davidxi/js-code-study/blob/9b990abbf038e0195567b30c8da216498c23691b/src/Event.js

It looks like this isn't actually an Event but a custom EventObject so it shouldn't be a problem.

That was a very quick check, but I didn't see anyone setting cancelBubble to false and it seems plausible that mapping cancelBubble=true to stopPropagation() is web compatible, at least worth investigating.

@RByers
Copy link
Contributor

RByers commented Apr 12, 2016

From this description it sounds like we don't even know for sure that removing cancelBubble completely would be incompatible. Every browser has it, right? Is it work preparing a patch that uses a use counter to track whether cancelBubble actually as an impact (i.e. at bubbling time log if cancelBubble was used without stopPropagation being used redundantly), and using the telemetry cluster to look for examples in the top 10k?

Or is the cost of having it as an alias of stopPropagation low enough, and the compat risk sufficiently non-trivial that we should just do it?

@annevk
Copy link
Member

annevk commented Apr 13, 2016

If we could get away with making it an alias rather than its own primitive I think that'd be fantastic. Removing it altogether given the usage numbers seems rather risky.

@foolip
Copy link
Member

foolip commented Apr 13, 2016

So a few things would be worth measuring, then:

  1. How often cancelBubble is changed from false to true (likely common) and from true to false (hopefully rare).
  2. How often the cancelBubble flag actually changes which callbacks are invoked (hopefully rare).

I could add this to Blink but need to focus on Media Session for a bit right now. @RByers?

@RByers
Copy link
Contributor

RByers commented Apr 13, 2016

Sorry I'm really swamped right now, but this is really DOM team, not input team. @tkent-google WDYT, worth exploring? Might someone on the DOM team be interested in doing the measurements @foolip suggests?

@tkent-google
Copy link
Collaborator

Yeah, we should collect such data definitely. I filed crbug.com/603377.

@smaug----
Copy link
Collaborator

Do we already know how different browser engines handle cancelBubble? It feels like we should write some extensive tests covering using cancelBubble during different event phases and with and without stopPropagation() etc. Would anyone have time to write such tests?

@foolip
Copy link
Member

foolip commented Apr 15, 2016

In Blink, for each target where event.eventPhase==event.BUBBLING_PHASE, if cancelBubble is true, the callbacks aren't invoked. It's initialized to false and can be set to true (or false) by anything that has access to the event object, including in the middle of the bubbling phase. It's not reset at the end of dispatch or at any other point.

@annevk
Copy link
Member

annevk commented Apr 15, 2016

Okay, so the difference with stop propagation is that stop propagation affects all phases. Seems reasonably safe to change cancelBubble then given that when cancelBubble was popular there was no capture phase and if you set it during the target phase it would only take effect while bubbling.

@smaug----
Copy link
Collaborator

for each target where event.eventPhase==event.BUBBLING_PHASE

it does not work AT_TARGET?

@annevk
Copy link
Member

annevk commented Apr 15, 2016

@smaug---- why would something named "cancel bubble", cancel target? (You can set it there, of course.)

@smaug----
Copy link
Collaborator

no idea. I wasn't asking that. I was just asking what the implementation actually does.
Like, what happens if one does
var e = new Event("foo"); e.cancelBubble = true; target.dispatchEvent(e);

I was asking mainly because AT_TARGET is special. Even though an listener is added to capture phase, listener AT_TARGET is called.

@annevk
Copy link
Member

annevk commented Apr 16, 2016

That will call listeners on the target in Chrome.

@foolip
Copy link
Member

foolip commented Apr 18, 2016

The cancelBubble setter always just sets an internal flag, but that flag isn't used until one gets to the bubbling phase. For full disclosure there is something strange in https://chromium.googlesource.com/chromium/src/+/aede1d0909b3f905c74c769e610b903015f50569/third_party/WebKit/Source/core/events/EventDispatcher.cpp#179 that I don't understand, namely how it looks like multiple targets might end up in the target phase, but whatever the reason is, it's still true that only callbacks invoked while the phase is bubbling are affected.

@cvrebert
Copy link
Member Author

The UseCounters got committed: https://bugs.chromium.org/p/chromium/issues/detail?id=603377#c3

@cvrebert
Copy link
Member Author

Chrome 52 (which includes the UseCounters) was promoted to Stable yesterday.

@foolip
Copy link
Member

foolip commented Jul 21, 2016

Usage timelines here:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1348
https://www.chromestatus.com/metrics/feature/timeline/popularity/1349
https://www.chromestatus.com/metrics/feature/timeline/popularity/1350

But they're not recent enough to show the beginning of the S-curve, it'll probably be a week or so before one can see something useful.

@cvrebert
Copy link
Member Author

With ~25 days of data, and the metrics looking somewhat stable:

How often cancelBubble is changed from false to true (likely common)

1.306% as of yesterday. So it's indeed common (relatively speaking).

and from true to false (hopefully rare).

0% for the entire timeline. So it's indeed extremely rare.

How often the cancelBubble flag actually changes which callbacks are invoked (hopefully rare).

Peaked at 0.027% on the chart.
0.025% as of yesterday.


So it seems like cancelBubble could definitely be simplified to a stopPropagation() alias,
and could potentially be removed entirely (0.027% ≤ 0.03% threshold).

@annevk
Copy link
Member

annevk commented Aug 23, 2016

@foolip are you willing to make this simplification in Blink? @cdumez how about WebKit? If we can make this a stopPropagation() alias it would be much easier to support in DOM, although there's still the annoyance of confusing new folks with legacy features.

@foolip
Copy link
Member

foolip commented Aug 23, 2016

Given the usage I'd certainly LGTM a simplification of cancelBubble to a stopPropagation() alias. But what should it do when going from true to false? Just ignore and leave the value unchanged I guess?

Since Gecko has this on UIEvent I guess at best we could hope to have it there in all engines, but since the amount of code would be the same in any case that doesn't seem very worthwhile to pursue. And it's probably easier to spec this in DOM than in UI Events too.

@annevk
Copy link
Member

annevk commented Aug 23, 2016

Yeah, putting it directly on Event seems fine with me since that's what most browsers ship. I'm mostly concerned about not requiring additional internal state.

@RByers
Copy link
Contributor

RByers commented Aug 23, 2016

I agree that making this a stopPropagation alias is the best choice - that's very low risk and gets us most of the benefit. The additional value of trying to remove it completely doesn't seem work the cost in compat pain given the low but non-trivial usage to me.

@bzbarsky
Copy link

it sounds like we don't even know for sure that removing cancelBubble completely would be incompatible

We do know that supporting "selectstart" events as Event but not having cancelBubble on Event breaks the ability to select text in emails in Outlook Web Access. See https://bugzilla.mozilla.org/show_bug.cgi?id=1280534#c33

Note that either I messed up debugging the Firefox side there or OWA is actually attaching different listeners or creating a different DOM depending on whether cancelBubble is on Event or something... It's not entirely clear what exactly is happening there (e.g. Chrome running against a version of the page saved in Firefox never reaches the cancelBubble codepath). But certainly moving cancelBubble to Event makes whatever insanity OWA is involved in work.

@cvrebert
Copy link
Member Author

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=657651 to try to keep things moving forward.

@inexorabletash
Copy link
Member

Missed the ping above, sorry.

IIRC, the Indexed DB-specific code is because it's the only place other than DOM where events bubble; it's effectively an implementation of the "get the parent" mechanism for EventTarget.

@Ms2ger
Copy link
Member

Ms2ger commented Dec 1, 2016

Gecko moved cancelBubble to Event in https://bugzilla.mozilla.org/show_bug.cgi?id=1298970

annevk added a commit that referenced this issue Dec 7, 2016
More delightful garbage from the past. Fixes #211.
@annevk
Copy link
Member

annevk commented Dec 8, 2016

In Gecko it maps to stopPropagation() and you can effectively un-stopPropagation() with cancelBubble = false.

This came up in review: do we want to prevent un-stopPropagation() going forward or do we want to keep that behavior? Basically, do we want to make setting it to false be a no-op rather than have that unset the internal flag?

@cvrebert
Copy link
Member Author

cvrebert commented Dec 8, 2016

FWIW, the metrics say there's virtually no code on the Web that does cancelBubble=false.

cvrebert added a commit to web-platform-tests/wpt that referenced this issue Dec 9, 2016
cvrebert added a commit to web-platform-tests/wpt that referenced this issue Dec 9, 2016
@zcorpan
Copy link
Member

zcorpan commented Dec 12, 2016

Tests by @cvrebert to help inform a spec change here
web-platform-tests/wpt#4304

@annevk
Copy link
Member

annevk commented Dec 16, 2016

@smaug---- and @foolip can you comment on whether you are okay with making setting cancelBubble to false a no-op? As mentioned above the metrics indicate this to be safe.

@foolip
Copy link
Member

foolip commented Dec 16, 2016

Yes, I think that's quite reasonable. @tkent-google, WDYT?

@smaug----
Copy link
Collaborator

I think that sounds good.

@cdumez
Copy link

cdumez commented Dec 16, 2016

Sorry for being late but on WebKit side, I'd be OK making cancelBubble=true an alias for stopPropagation().

Regarding cancelBubble=false, my personal preference would be to un-stopPropagation() rather than making it a no-op. This is simply because having cancelBubble=true do something and cancelBubble=false be a no-op seems like a weird API. Also, I think there is very little cost (in terms of code complexity) to make cancelBubble=false clear the stopPropagation flag. That said, I am not opposed to making it a no-op if this is what others prefer.

@annevk
Copy link
Member

annevk commented Dec 16, 2016

The reason for the no-op is that otherwise we have a legacy API with a new feature, which is rather odd.

@tkent-google
Copy link
Collaborator

| Yes, I think that's quite reasonable. @tkent-google, WDYT?

+1

annevk pushed a commit to web-platform-tests/wpt that referenced this issue Dec 19, 2016
annevk added a commit that referenced this issue Dec 19, 2016
More delightful garbage from the past.

Tests: web-platform-tests/wpt#4304.

Fixes #211.
dougt pushed a commit to dougt/chromium that referenced this issue Jan 26, 2017
According to spec https://dom.spec.whatwg.org/#dom-event-cancelbubble, setting
cancelBubble to true is considered as an alias to stopPropagation(), setting
cancelBubble to false is a no-op. Detail discussion is on
whatwg/dom#211.

BUG=675549

Review-Url: https://codereview.chromium.org/2641953006
Cr-Commit-Position: refs/heads/master@{#446118}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests