Closed Bug 1024488 Opened 10 years ago Closed 7 years ago

defaultPrevented returns a different result than getPreventDefault()

Categories

(Core :: DOM: Events, defect)

29 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: eli, Unassigned)

References

()

Details

(Keywords: addon-compat, regression, site-compat)

I found a situation where the new .defaultPrevented event attribute returns an incorrect value, while the deprecated .getPreventDefault() returns the correct value. STEPS TO REPRODUCE: Enter the following code in the Firefox scratchpad while running in the "Browser" context: window.addEventListener('fooEvent', function (event) { console.log('fooEvent has fired'); event.preventDefault(); }, true, true); and then in the Javascript console which Firefox provides with Shift+Ctrl+k enter the following code: e = document.createEvent('Event'); e.initEvent('fooEvent', true, true); document.dispatchEvent(e); console.log('after dispatch', e.defaultPrevented, e.getPreventDefault()); ACTUAL RESULT: I see the "fooEvent has fired" message which indicates that the event handler ran properly. But the other log message displays "after dispatch false true" which indicates that .defaultPrevented is wrong and .getPreventDefault() is correct. EXPECTED RESULT: I would expect everything to happen the same except that the second log message should read "after dispatch true true" which would indicate that .defaultPrevented has the correct file. I see this in Firefox 29 and nmaier on StackOverflow has reproduced it in Firefox 30 and in the Nightly (http://stackoverflow.com/a/24180301/1694).
Blocks: 691151
Component: Developer Tools → DOM: Events
Product: Firefox → Core
Related to Bug 985988?
For whatever it's worth, I ran the test code listed in Bug 985988 and confirmed that I see the correct behavior. So while it might be related somehow, it seems that this bug occurs even after that fix.
Should be related to bug 930374. In some situations, the difference is by design. We changed the .defaultPrevented value for standard spec. However, the legacy method, getPreventedDefault() keeps the legacy behavior for compatibility with older Gecko. Looks like that the event listener for "fooEvent" is in chrome context, but the Javascript console is in content context. Then, content's script cannot know if chrome context called .preventDefault() with .defaultPrevent. This is important for hiding a call of preventDefault() in Firefox's UI or add-ons from normal web applications. For DOM Events module, this is INVA or WONTFIX. Although, I don't know if the Scrachpad works in chrome context and if it's intentionally.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3) > Should be related to bug 930374. > > In some situations, the difference is by design. > > We changed the .defaultPrevented value for standard spec. However, the > legacy method, getPreventedDefault() keeps the legacy behavior for > compatibility with older Gecko. Yeah, this seem to have regressed due to Bug 930374. Frankly that "fix" over there is incorrect, as the assumption that all |preventDefault()| calls from chrome are somehow originating from default handlers. The STR here show that this is not necessarily the case. The default handler would be implemented by the content script dispatching the event in the first place not the chrome/scratchpad bit that just tries to prevent the default action from happening. The correct fix would be to not make actual default handlers call |preventDefault()| in the first place. Also the "fix" explicitly does not correct the default handler behavior for chrome/chrome code. > Looks like that the event listener for "fooEvent" is in chrome context, but > the Javascript console is in content context. Then, content's script cannot > know if chrome context called .preventDefault() with .defaultPrevent. Correct. > This is important for hiding a call of preventDefault() in Firefox's UI or > add-ons from normal web applications. "Hiding" calls is unexpected to say the least... > For DOM Events module, this is INVA or WONTFIX. Although, I don't know if > the Scrachpad works in chrome context and if it's intentionally. I don't see how this is INVALID. This also violates the spec, which states: > Used to indicate whether Event.preventDefault() has been called for this event. Adding addon-compat as this regression might have silently broken some add-ons (since mozilla28) and/or might break stuff if fixed and there are add-on relying on the mozilla28 behavior.
Status: UNCONFIRMED → NEW
Depends on: 930374
Ever confirmed: true
Keywords: addon-compat
(In reply to Nils Maier [:nmaier] from comment #4) > The correct fix would be to not make actual default handlers call > |preventDefault()| in the first place. How so? > Also the "fix" explicitly does not correct the default handler behavior for > chrome/chrome code. Yes, that is the wanted behavior. > > I don't see how this is INVALID. This also violates the spec, which states: There is no spec for this stuff. No spec defines how event propagation/handling works cross content/chrome boundaries.
(In reply to Olli Pettay [:smaug] from comment #5) > (In reply to Nils Maier [:nmaier] from comment #4) > > The correct fix would be to not make actual default handlers call > > |preventDefault()| in the first place. > How so? Because default handlers do not prevent the default, instead they are the default. > > Also the "fix" explicitly does not correct the default handler behavior for > > chrome/chrome code. > Yes, that is the wanted behavior. And this spec violation (because here, there is no undefined cross-boundary stuff at play) is wanted by whom? I as an add-on developer will not expect this, in particular as it violates the spec and contradicts the MDN docs. I doubt other developers, add-on devs and Firefox frontend people, would expect that events behave differently in their HTML (or XUL or whatever) DOM depending on whether their code runs with content or chrome privileges. Why should e.g. about:memory (privileged) see `defaultPrevented == true` when for the same event type and default handler about:home (unprivileged) does not? > > I don't see how this is INVALID. This also violates the spec, which states: > There is no spec for this stuff. No spec defines how event > propagation/handling works cross > content/chrome boundaries. Granted. But then again the cross-privilege-boundary event APIs Firefox provides intensionally look the same than W3 "content" event APIs. IMHO, they should behave the same unless there is a specific reason (security, whatever) this cannot happen and/or unless the difference isn't just merely an addition (like XHR accepting nsIInputStream). But then at the very least the difference should be documented, and even better, there should be a distinct API that screams "I'm not a real web API according to spec". |mozPreventDefaultButDoNotTellContent| or whatever. Anyway, a question that remains either way: What should chrome-privileged code (add-ons, Firefox) do in order for content code to see |.defaultPrevent == true|. Right now there doesn't seem to be a way to make this happen at all...
(In reply to Nils Maier [:nmaier] from comment #6) > (In reply to Olli Pettay [:smaug] from comment #5) > > (In reply to Nils Maier [:nmaier] from comment #4) > > > The correct fix would be to not make actual default handlers call > > > |preventDefault()| in the first place. > > How so? > > Because default handlers do not prevent the default, instead they are the > default. Sorry, I don't understand what you meant here. It's odd we call the method name "prevent default" in chrome because they must be handlers for default action. The purpose of a call of preventDefault() is preventing double (default) action. So, we should think that a call of preventDefault() in chrome context is really different from a call of it in content context. > > > Also the "fix" explicitly does not correct the default handler behavior for > > > chrome/chrome code. > > Yes, that is the wanted behavior. > > And this spec violation (because here, there is no undefined cross-boundary > stuff at play) is wanted by whom? I as an add-on developer will not expect > this, in particular as it violates the spec and contradicts the MDN docs. I > doubt other developers, add-on devs and Firefox frontend people, would > expect that events behave differently in their HTML (or XUL or whatever) DOM > depending on whether their code runs with content or chrome privileges. Why > should e.g. about:memory (privileged) see `defaultPrevented == true` when > for the same event type and default handler about:home (unprivileged) does > not? If chrome-like contents is preformed in content context, they should just behave as web apps. I.e., they should consume events before chrome's handlers. So, the difference should be matter. > > > I don't see how this is INVALID. This also violates the spec, which states: > > There is no spec for this stuff. No spec defines how event > > propagation/handling works cross > > content/chrome boundaries. > > Granted. But then again the cross-privilege-boundary event APIs Firefox > provides intensionally look the same than W3 "content" event APIs. IMHO, > they should behave the same unless there is a specific reason (security, > whatever) this cannot happen and/or unless the difference isn't just merely > an addition (like XHR accepting nsIInputStream). But then at the very least > the difference should be documented, and even better, there should be a > distinct API that screams "I'm not a real web API according to spec". > |mozPreventDefaultButDoNotTellContent| or whatever. I'm not sure in which case, addons need to check .defaultPrevented in content context after all chrome handlers listened the events? > Anyway, a question that remains either way: What should chrome-privileged > code (add-ons, Firefox) do in order for content code to see |.defaultPrevent > == true|. Right now there doesn't seem to be a way to make this happen at > all... In chrome context, when defaultPrevented is true, the event has been consumed by web applications or chrome handler run before the handler. In content context, it means, as spec, whether preventDefault() has been called for preventing default action or not. So, if addons need to distinguish whether defaultPrevented is set true by another chrome handler or not, we need to add new API which is available only in chrome context.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > (In reply to Nils Maier [:nmaier] from comment #6) > > (In reply to Olli Pettay [:smaug] from comment #5) > > > (In reply to Nils Maier [:nmaier] from comment #4) > > > > The correct fix would be to not make actual default handlers call > > > > |preventDefault()| in the first place. > > > How so? > > > > Because default handlers do not prevent the default, instead they are the > > default. > > Sorry, I don't understand what you meant here. It's odd we call the method > name "prevent default" in chrome because they must be handlers for default > action. The purpose of a call of preventDefault() is preventing double > (default) action. So, we should think that a call of preventDefault() in > chrome context is really different from a call of it in content context. Handlers in chrome are NOT necessarily default handlers. After thinking some more about it, I guess the fundamental issue here is what actually constitutes a default handler (a collection of default actions). This, to my understanding, depends on who owns the event itself, i.e. what piece of code creates and dispatches it. Only the owning code may implement the default handler. That is a necessity because you cannot know for sure if the default handler should be called until `dispatchEvent` actually returns (or must execute default actions before even dispatching the event). Any other code reacting to an event is not a default handler, and must therefore prevent the default if it wants to handle the event itself instead of the default handler. E.g. if the engine generated a click event e.g. on a link, it should carry out the default action if not prevented from doing so e.g. navigate according the the link href. If another piece of code installed a click handler and reacts to click events the engine generates, it must |.preventDefault()| if it wants to handle the event instead of the default handler aka. the engine. Or, as DOM Level 3 Events puts it: > Default actions SHOULD be performed after the event dispatch has been completed, but in exceptional cases MAY also be performed immediately before the event is dispatched. Now what we have here in the STR is some content script generating and dispatching an event. Therefore the content script owns that event and is therefore the only piece of code allowed to default-handle it. Now the chrome-privileged ScratchPad code has a listener for that event (it does not own, of course), and handles the event, in this instance by doing nothing and |preventDefault()|ing. Now the owner, the content script, must know that the it should not be executing the default handler code. This information is relayed e.g. through |.defaultPrevented|. Except that it is not in this case! And while the specs do not explicitly address security boundaries per se, it addresses trusted vs. untrusted events, so IMO the remarks in comment #5 about boundaries are moot. If you got a problem with "double-defaulted" events, then the code doing the defaulting is wrong. Only the piece of code that generated the event and therefore owns it can implement/call a default handler or delegate that task to a different piece of code, so there cannot be multiple default handlers double-defaulting stuff, at least in theory. If there is some other handler that does not own the event but still considers itself the default handler, or considers itself to be the reincarnation of Napoleon for that matter, it should be fixed. It's a band-aid you put on the code to mask these kinds of misbehavior in bug 930374, which would have been OK, if it didn't break stuff... Unfortunately however, it does break some stuff. Well, some of the *built-in* |preventDefault()| calls that the band-aid "hides" from content might be actually incorrectly hidden now, just like the call in the STR is incorrectly hidden. Maybe you consider the stuff that got broken and/or is still broken to be minor enough to WONTFIX or too hard to correctly fix; that's not my call, although I would disagree on WONTFIX. The assumption that |preventDefault()| in chrome is different is outright wrong as it contradicts the specs, contradicts existing documentation and also might contradict existing code that was silently broken when the engine started hiding defaultPrevented from content in certain cases. If you want to prevent double defaulting by having some flag in the event instance itself to make it easier to deal with the existing code base, then please don't piggyback on |preventDefault()|/|defaultPrevented| which have a previously defined, different behavior and instead actually add a new flag (and make is accessible from chrome-only, if you like). > If chrome-like contents is preformed in content context, they should just > behave as web apps. I.e., they should consume events before chrome's > handlers. So, the difference should be matter. Sorry, I didn't get this at all. > I'm not sure in which case, addons need to check .defaultPrevented in > content context after all chrome handlers listened the events? Maybe, or a website wants to dispatch some events that it knows *might* be handled by some add-on and display something if the user has none of the supported add-ons installed (default handler). Or whatever. Apparently there is a use case for this, or the reporter wouldn't have stumbled across this. E.g. I run a website that dispatches such a custom event from a content (https://about.downthemall.net/3.0/). That event instructs one of my add-ons to open a certain dialog (click on "Add toolbar buttons", which calls https://about.downthemall.net/yui/toolbarinstall.js). Now, I might decide that the default action should be to display a message to the user to install my add-on if the message comes back un-prevented. I cannot use |.defaultPrevented| for that because of this bug. (Right now, the website does nothing, i.e. I didn't implement a default handler). > > Anyway, a question that remains either way: What should chrome-privileged > > code (add-ons, Firefox) do in order for content code to see |.defaultPrevent > > == true|. Right now there doesn't seem to be a way to make this happen at > > all... > > In chrome context, when defaultPrevented is true, the event has been > consumed by web applications or chrome handler run before the handler. OK, but this is not the issue here. That is actually the correct behavior. > In > content context, it means, as spec, whether preventDefault() has been called > for preventing default action or not. Again, this isn't correctly communicated (cross security-boundaries, not that it really matters). The STR clearly called |preventDefault()|, yet in the content context it is incorrectly indicated that |preventDefault()| was NOT called, so the owning content script would execute the default handler (collection of default actions) anyway. > So, if addons need to distinguish whether defaultPrevented is set true by > another chrome handler or not, we need to add new API which is available > only in chrome context. No, *content* (regular websites or whatever) needs to be informed that the default should be prevented and the add-on and/or frontend code running as chrome cannot do so at the moment.
Since addons no longer have chrome privileges, the point is moot. Now it is an implementation detail that chrome happens to be implemented by a DOM-like infrastructure. Web pages can tell if a WebExtensions content script prevented the default.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.