930374 - Event.defaultPrevented shouldn't become true if preventDefault() was called by our internal handler for default action

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, our internal event handler for implementing default action calls preventDefault() for preventing other internal event handlers not handling the event anymore.

I think that this is okay. However, this causes defaultPrevented attribute becomes true for web apps (content) too.

I'm thinking that we can quick hack for this.

1. Before dispatching an event in system group, store the defaultPrevented value. If it becomes true after dispatched, we should set new flag.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventDispatcher.cpp#342

2. Before calling nsIFrame::HandleEvent(), store the defaultPrevented value. If it becomes true after the call, we should set new same flag.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#477

3. If the new flag and defaultPrevented flag is true while the event phase is NONE, nsIDOMWindow::DefaultPrevented() should return false only when the caller is content.

Even with this hack, if some default actions are implemented in normal event group, this hack doesn't work. However, such handlers should be moved to system group because such handlers may be suppressed by stopPropagation() of web apps.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This patch adds mDefaultPreventedByDefaultActionHandler to EventFlagsBase. This stores if mDefaultPrevented is set in system group (see nsDOMEvent::PreventDefault()).

In nsDOMEvent::DefaultPrevented(), the new member is used for deciding the result. See the comments in the method for the detail.

The risky point of this bug is, nsIFrame::HandleEvent() is called in system group instead of normal group. I have no idea about regressions which are caused by this change, though.

Comment 2

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

Comment on attachment 824060 [details] [diff] [review]
Patch


>+  // While this event is being dispatched, returns true if PreventDefault() has
>+  // been called by anybody since any other event handler shouldn't handle such
>+  // event anymore.
>+  // XXX This may return unexpected value if two or more events are nested and
>+  //     preceding event is stored and the nested event handler referes it.
refers

>+  //     For example, when a keypress event causes modifying the text, an input
>+  //     event may be dispatched from keypress event handler of editor.
>+  //     I.e., when an input event handler runs, the keypress event is
>+  //     still being dispatched. However, preventDefault attribute of the
>+  //     keypress event (if it was stored a variable at preceding keypress event
>+  //     handler) returns true even though the content has never called
>+  //     preventDefault() of the keypress event.
>+  //     However, this is really rare case, we don't need to mind it.
Nested events are not a rare case and we need to figure out some way to deal with this situation.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #2)
> >+  //     For example, when a keypress event causes modifying the text, an input
> >+  //     event may be dispatched from keypress event handler of editor.
> >+  //     I.e., when an input event handler runs, the keypress event is
> >+  //     still being dispatched. However, preventDefault attribute of the
> >+  //     keypress event (if it was stored a variable at preceding keypress event
> >+  //     handler) returns true even though the content has never called
> >+  //     preventDefault() of the keypress event.
> >+  //     However, this is really rare case, we don't need to mind it.
> Nested events are not a rare case and we need to figure out some way to deal
> with this situation.

Nested event is not a rare case, but I meant referring other events from an event handler is a rare case. (I.e., referring keypress event's defaultPrevented from input event handler)

In this example, we can fix this issue by making editor call preventDefault() *after* dispatch an input event. So, I think that we need to check all event handlers one by one. I don't think that it's scope of this bug.

There is another possible idea, but I'm not sure this works fine in all cases. If nsDOMEvent::DefaultPrevented() is called only from JS, we can check if the caller is chrome. If the caller is chrome, it should return mDefaultPrevented. Otherwise, check the new member.

And then, for compatibility, GetDefaultPrevented() and GetPreventDefault() always return mDefaultPrevented.

So, the point which I'm not sure is, whether DefaultPrevented() is called only from script or not and GetDefaultPrevented() is called only from C++ or not.

Comment 4

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

We can have JS-only version of DefaultPrevented() if needed.
Just map the webidl's defaultPrevented to some FooBarJS() method using Bindings.conf

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Thank you for the really helpful information.

I'm writing this patch. The test result is here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=616c4e3b41aa

Then, I hit a problem with following tests:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/tests/test_texteditor_keyevent_handling.html?force=1
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1

They are mochitest-plain tests. Therefore, even in the system event listeners which are registered with SpecialPowers, the defaultPrevented returns false for content script.

If we can return default action result in such case, we can keep the compatibility perfectly. I think that if we can do it, it's the best solution. However, for it, nsDOMEvent::DefaultPreventedJS() needs to know whether the event is the latest dispatched event or not. I.e., nsEventListenerManager ot somebody needs to store the pointer to DOM event at dispatching it. But I'm not sure what way is thread-safe. If nsEventListenerManager were created for each thread, it'd fix this issue easily. But looks like it's not so. Do you have any idea for this?

An alternative hack is, we make the tests use getPreventDefault() instead of defaultPrevented. I think that modern apps should use only defaultPrevented. Therefore, I think that not changing getPreventDefault() behavior is better choice. Then, this way can fix the issue.

# I'm still not sure a cause of the orange of 5 on Mac, though.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Or, the tests should be switched back to mochitest-chrome again because they test chrome level behavior.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

Perhaps, we should make the editor tests chrome tests.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a chrome test

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Make test_*editor_keyevent_handling.html chrome tests since they tests behavior of system group event handler

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Fix new orange of test_bug448987.html on Mac since it doesn't set tab navigation setting

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screenshot at new orange of test_assign_event_data.html

Hmm, by making test_assign_event_data.html a content test, it causes new random orange on Mac :-(

See the screenshot, then, you can understand the reason.

The preceding test test_handlerApps.xhtml launches iCal and kill it. However, looks like it fails.
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/handlerApps.js

I think that I should file a bug for this issue and disables test_assign_event_data.html only on Mac...

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a chrome test

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 825282 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

This approach is, if the caller of defaultPrevented is content, it hides the fact that the event is prevented other actions by default action handler. Otherwise, i.e., chrome can know if the preventDefault() has already been called.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 825349 [details] [diff] [review]
part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a chrome test

This test should be mochitest-plain because it tests the behavior for web applications.

Additionally, this test tests calling preventDefault() from content case too.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 825284 [details] [diff] [review]
part.3 Make test_*editor_keyevent_handling.html chrome tests since they tests behavior of system group event handler

These events test if preventDefault() is called by default action handler in editor. Therefore, these events should be mochitest-chrome since the default action handler behavior is hidden from defaultPrevented attribute.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 825285 [details] [diff] [review]
part.4 Fix new orange of test_bug448987.html on Mac since it doesn't set tab navigation setting

I'm not sure why this test becomes orange on Mac by my patches.

The reason is, the test doesn't set accessibility.tabfocus for making image map tabbable on Mac. This patch just sets the pref and fix a bug when it fails to set focus by tab navigation. (Currently, if it fails, error message is printed repeatedly since the interval timer isn't canceled)

Comment 17

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

Comment on attachment 825282 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

I don't know what "default event action handler" is.
Why not just mDefaultPreventedByDefaultHandler
(that would mean anything in system event group or PostHandleEvent)

But I need to think this all a bit.
I don't like relying on nsContentUtils::ThreadsafeIsCallerChrome() calls.

Comment 18

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

Comment on attachment 825284 [details] [diff] [review]
part.3 Make test_*editor_keyevent_handling.html chrome tests since they tests behavior of system group event handler

Though this is a bit worrisome, since whatever the test is doing isn't
apparently possible after the change... but yeah, that is what the bug
is about, to change the web facing API.

Comment 19

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

Comment on attachment 825285 [details] [diff] [review]
part.4 Fix new orange of test_bug448987.html on Mac since it doesn't set tab navigation setting

This could land separately before any other patches, right?

Comment 20

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

Comment on attachment 825349 [details] [diff] [review]
part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a chrome test

I don't understand the changes to runNextTest(), and the timeout is scary.
refresh driver may not run within 100ms if we happen to run the test
on a debug build and GC/CC runs after the setTimeout call.

If you need refresh driver checks, use requestAnimationFrame

Comment 21

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

Comment on attachment 825282 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

So could we actually use implicitJSContext in the Bindings.conf and have
a JS only version of PreventDefault(). Then use the passed cx for the
chrome check, and C++ callers would always mean "default handler".

Also, doesn't DefaultPreventedJS() return wrong value if
content calls preventDefault() and then default handler / chrome calls it too.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #21)
> So could we actually use implicitJSContext in the Bindings.conf and have
> a JS only version of PreventDefault(). Then use the passed cx for the
> chrome check, and C++ callers would always mean "default handler".

How can I check if the context is in content or chrome with JSContext?

JSContext::runningWithTrustedPrincipals() sounds a method which I want. However, it's used only in under js. So, it looks a private method in the js module. What's the way you expected?

Comment 23

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

xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #23)
> xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))

Hmm, that causes crash in tests of workers only on debug build:
http://mxr.mozilla.org/mozilla-central/source/caps/include/nsJSPrincipals.h#25

> 22:01:44     INFO -  1912 INFO TEST-PASS | /tests/dom/workers/test/test_dataURLWorker.html | Got correct message
> 22:01:44     INFO -  1913 INFO TEST-INFO | MEMORY STAT vsize after test: 3758231552
> 22:01:44     INFO -  1914 INFO TEST-INFO | MEMORY STAT residentFast after test: 296636416
> 22:01:44     INFO -  1915 INFO TEST-END | /tests/dom/workers/test/test_dataURLWorker.html | finished in 137ms
> 22:01:44     INFO -  DefaultPrevented(JSContext*)
> 22:01:44     INFO -  ++DOMWINDOW == 27 (0x129296428) [pid = 894] [serial = 611] [outer = 0x12670e118]
> 22:01:44     INFO -  1916 INFO TEST-START | /tests/dom/workers/test/test_errorPropagation.html
> 22:01:44     INFO -  DefaultPrevented(JSContext*)
> 22:01:44     INFO -  DefaultPrevented(JSContext*)
> 22:01:44     INFO -  ++DOMWINDOW == 28 (0x1479da9b8) [pid = 894] [serial = 612] [outer = 0x12670e118]
> 22:01:44     INFO -  ++DOCSHELL 0x11ce591e0 == 9 [pid = 894] [id = 163]
> 22:01:44     INFO -  ++DOMWINDOW == 29 (0x1283bf788) [pid = 894] [serial = 613] [outer = 0x0]
> 22:01:44     INFO -  [894] WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file ../../../docshell/base/nsDocShell.cpp, line 8591
> 22:01:44     INFO -  ++DOMWINDOW == 30 (0x11dbe5a58) [pid = 894] [serial = 614] [outer = 0x1283bf788]
> 22:01:44     INFO -  1917 INFO TEST-PASS | /tests/dom/workers/test/test_errorPropagation.html | Correct message event.message
> 22:01:44     INFO -  1918 INFO TEST-PASS | /tests/dom/workers/test/test_errorPropagation.html | Correct message event.filename
> 22:01:44     INFO -  1919 INFO TEST-PASS | /tests/dom/workers/test/test_errorPropagation.html | Correct message event.lineno
> 22:01:44     INFO -  PreventDefault(JSContext*)
> 22:01:44     INFO -  Assertion failure: self->debugToken == DEBUG_TOKEN, at ../../../dist/include/nsJSPrincipals.h:25
> 22:01:46  WARNING -  TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/test_errorPropagation.html | application terminated with exit code 256
> 22:01:46     INFO -  INFO | runtests.py | Application ran for: 0:01:27.596645
> 22:01:46     INFO -  INFO | zombiecheck | Reading PID log: /var/folders/99/99J8qVNwGhyCKSm1qW-gHE+++-k/-Tmp-/tmpVnBh5Kpidlog
> 22:02:05  WARNING -  PROCESS-CRASH | /tests/dom/workers/test/test_errorPropagation.html | application crashed [@ xpc::AccessCheck::isChrome(JSCompartment*)]
> 22:02:05     INFO -  Crash dump filename: /var/folders/99/99J8qVNwGhyCKSm1qW-gHE+++-k/-Tmp-/tmpbx7wgR/minidumps/EA8C500F-795B-40E7-A1B9-23973D69F048.dmp
> 22:02:05     INFO -  Operating system: Mac OS X
> 22:02:05     INFO -                    10.6.8 10K549
> 22:02:05     INFO -  CPU: amd64
> 22:02:05     INFO -       family 6 model 23 stepping 10
> 22:02:05     INFO -       2 CPUs
> 22:02:05     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
> 22:02:05     INFO -  Crash address: 0x0
> 22:02:05     INFO -  Thread 49 (crashed)
> 22:02:05     INFO -   0  XUL!xpc::AccessCheck::isChrome(JSCompartment*) [nsJSPrincipals.h:6735250b8900 : 25 + 0x0]
> 22:02:05     INFO -      rbx = 0x00007fff7000a2f8   r12 = 0x000000011936af00
> 22:02:05     INFO -      r13 = 0x0000000149bff53a   r14 = 0x0000000127cf8600
> 22:02:05     INFO -      r15 = 0x000000010224f400   rip = 0x000000010282287a
> 22:02:05     INFO -      rsp = 0x0000000149bfec20   rbp = 0x0000000149bfec40
> 22:02:05     INFO -      Found by: given as instruction pointer in context
> 22:02:05     INFO -   1  XUL!nsDOMEvent::PreventDefault(JSContext*) [nsDOMEvent.cpp:6735250b8900 : 455 + 0x4]
> 22:02:05     INFO -      rbx = 0x000000011936afe0   r12 = 0x000000011936af00
> 22:02:05     INFO -      r13 = 0x0000000149bff53a   r14 = 0x0000000127cf8680
> 22:02:05     INFO -      r15 = 0x000000010224f400   rip = 0x0000000102d03477
> 22:02:05     INFO -      rsp = 0x0000000149bfec50   rbp = 0x0000000149bfec60
> 22:02:05     INFO -      Found by: call frame info
> 22:02:05     INFO -   2  XUL!mozilla::dom::EventBinding::preventDefault [EventBinding.cpp:6735250b8900 : 383 + 0xa]
> 22:02:05     INFO -      rbx = 0x0000000149bfec90   r12 = 0x000000011936af00
> 22:02:05     INFO -      r13 = 0x0000000149bff53a   r14 = 0x000000011936afe0
> 22:02:05     INFO -      r15 = 0x000000010224f400   rip = 0x000000010224f987
> 22:02:05     INFO -      rsp = 0x0000000149bfec70   rbp = 0x0000000149bfec80
> 22:02:05     INFO -      Found by: call frame info
> 22:02:05     INFO -   3  XUL!mozilla::dom::EventBinding::genericMethod [EventBinding.cpp:6735250b8900 : 663 + 0x4]
> 22:02:05     INFO -      rbx = 0x0000000125ba4098   r12 = 0x000000011936af00
> 22:02:05     INFO -      r13 = 0x0000000149bff53a   r14 = 0x000000011936afe0
> 22:02:05     INFO -      r15 = 0x000000010224f400   rip = 0x000000010224f4dd
> 22:02:05     INFO -      rsp = 0x0000000149bfec90   rbp = 0x0000000149bfecf0
> 22:02:05     INFO -      Found by: call frame info
> 22:02:05     INFO -   4  XUL!js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [jscntxtinlines.h:6735250b8900 : 220 + 0x5]
> 22:02:05     INFO -      rbx = 0x000000011936afe0   r12 = 0x000000011936af00
> 22:02:05     INFO -      r13 = 0x0000000149bff53a   r14 = 0x0000000149bff170
> 22:02:05     INFO -      r15 = 0x000000010224f400   rip = 0x0000000104301541
> 22:02:05     INFO -      rsp = 0x0000000149bfed00   rbp = 0x0000000149bfed30
> 22:02:05     INFO -      Found by: call frame info

Any ideas?

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The value is modified here:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/Principal.cpp#22

Comment 26

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

Oh right, that is main thread only. workers are simpler.

mIsMainThreadEvent ? xpc::AccessCheck::isChrome(js::GetContextCompartment(cx)) :
                     mozilla::dom::workers::IsCurrentThreadRunningChromeWorker();

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

Okay, this works fine!
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d2a6f07aaf57

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a plain test

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 825349 [details] [diff] [review]
> part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and
> make it a chrome test
> 
> I don't understand the changes to runNextTest(),

It tries the all same tests again but the event handler calls preventDefault(). So, the additional cases test Assign*EventData() with default prevented state.

> and the timeout is scary.
> refresh driver may not run within 100ms if we happen to run the test
> on a debug build and GC/CC runs after the setTimeout call.
> 
> If you need refresh driver checks, use requestAnimationFrame

Thank you, it works!

Comment 29

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

Comment on attachment 8342320 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler


>+bool
>+nsDOMEvent::IsChromeContext(JSContext* aCx) const
Since the context isn't actually chrome (but the compartment), could you rename to 
IsChrome(JSContext* aCx)

>+  // If this has already been consumed by non-default handler,
>+  // we shouldn't update mDefaultPreventedByDefaultHandler.
>+  if (!(mEvent->mFlags.mDefaultPrevented &&
>+        !mEvent->mFlags.mDefaultPreventedByDefaultHandler)) {
I don't understand !mEvent->mFlags.mDefaultPreventedByDefaultHandler
I'd expect 
  if (aCalledByDefaultHandler &&
      !mEvent->mFlags.mDefaultPrevented) {
    mEvent->mFlags.mDefaultPreventedByDefaultHandler = true;



>+  // Need to set an extra flag for drag events.
>+  if (mEvent->eventStructType == NS_DRAG_EVENT && IsTrusted()) {
>+    nsCOMPtr<nsINode> node = do_QueryInterface(mEvent->currentTarget);
>+    if (!node) {
>+      nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(mEvent->currentTarget);
>+      if (win) {
>+        node = win->GetExtantDoc();
>       }
>     }
>+    if (node && !nsContentUtils::IsChromeDoc(node->OwnerDoc())) {
>+      mEvent->mFlags.mDefaultPreventedByContent = true;
>+    }
We could probably probably simplify this by just using the information about
aCalledByDefaultHandler. But let's change that in a followup.


Please explain or fix
>+  if (!(mEvent->mFlags.mDefaultPrevented &&
>+        !mEvent->mFlags.mDefaultPreventedByDefaultHandler)) {

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8342320 [details] [diff] [review]
> part.1 Event.defaultPrevented should be false even if preventDefault() has
> been called by default action handler

> >+  // If this has already been consumed by non-default handler,
> >+  // we shouldn't update mDefaultPreventedByDefaultHandler.
> >+  if (!(mEvent->mFlags.mDefaultPrevented &&
> >+        !mEvent->mFlags.mDefaultPreventedByDefaultHandler)) {
> I don't understand !mEvent->mFlags.mDefaultPreventedByDefaultHandler
> I'd expect 
>   if (aCalledByDefaultHandler &&
>       !mEvent->mFlags.mDefaultPrevented) {
>     mEvent->mFlags.mDefaultPreventedByDefaultHandler = true;

No, if we do so, following case fails:

1. Chrome such as UI or Add-on call preventDefault(), then, both mDefaultPrevented and mDefaultPreventedByDefaultHandler are true.
2. Content such as Web apps call preventDefault(), then, mDefaultPreventedByDefaultHandler is still true. This is odd if content checks defaultPrevented attribute.

> >+  // Need to set an extra flag for drag events.
> >+  if (mEvent->eventStructType == NS_DRAG_EVENT && IsTrusted()) {
> >+    nsCOMPtr<nsINode> node = do_QueryInterface(mEvent->currentTarget);
> >+    if (!node) {
> >+      nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(mEvent->currentTarget);
> >+      if (win) {
> >+        node = win->GetExtantDoc();
> >       }
> >     }
> >+    if (node && !nsContentUtils::IsChromeDoc(node->OwnerDoc())) {
> >+      mEvent->mFlags.mDefaultPreventedByContent = true;
> >+    }
> We could probably probably simplify this by just using the information about
> aCalledByDefaultHandler. But let's change that in a followup.

Agreed.

> Please explain or fix
> >+  if (!(mEvent->mFlags.mDefaultPrevented &&
> >+        !mEvent->mFlags.mDefaultPreventedByDefaultHandler)) {

Okay, I'll explain it by comment.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

Comment 32

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

Comment on attachment 8343439 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler


>+  // In following cases, we need to update mDefaultPreventedByDefaultHandler:
>+  // 1. PreventDefault() hasn't been called yet.
>+  // 2. PreventDefault() has already been called by chrome, but it's also called
>+  //    again by content because if we don't mDefaultPreventedByDefaultHandler
>+  //    in this case, Event.defaultPrevented becomes false for content even
>+  //    though it called Event.preventDefault().
>+  // Please don't update mDefaultPreventedByDefaultHandler when PreventDefault()
>+  // has already called by content and it's called again by chrome.  If we
>+  // update it to true, Event.defaultPrevented becomes false even though content
>+  // called Event.preventDefault().

Would it be simpler to use mDefaultPreventedByContent
effectively

mEvent->mFlags.mDefaultPrevented = true;
if (!aCalledByDefaultHandler) {
  mEvent->mFlags.mDefaultPreventedByContent = true;
}


>+nsDOMEvent::DefaultPrevented(JSContext* aCx) const
>+{
...
>+
>+  // If preventDefault() has been called by content, just return true.
>+  if (!mEvent->mFlags.mDefaultPreventedByDefaultHandler) {
>+    return true;
>+  }
>+
>+  // Otherwise, change the result for each caller. If caller of this is content,
>+  // defaultPrevented attribute should be false.  Otherwise, i.e., caller of
>+  // this is chrome, defaultPrevented should be true.
>+  return IsChrome(aCx);
>+}

Then replace the stuff above with
  return mEvent->mFlags.mDefaultPreventedByContent || IsChrome(aCx);

Comment 33

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

And yes, that would require to remove that drag event special case.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I worry about the risk of using mDefaultPreventedByContent in this schedule. As I cced you to Jonathan's email, he needs this fix ASAP. Are you sure that it's safe to modify mDefaultPreventedByContent from all events??

Comment 35

[Jonathan Watt [:jwatt]]

Could that change be a follow-up bug?

Comment 36

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

Using mDefaultPreventedByContent should be safe. It is used currently only in one place in ESM.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler

Dispatching legacy mouse events which inherit defaultPrevented state needs to copy mDefaultPreventedByContent too. And it cannot be represented by nsEventStatus. So, I think that it is time to refactor nsEventStatus in follow up bug.

Comment 38

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

Comment on attachment 8344272 [details] [diff] [review]
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler


>    * @param aTargetFrame        The event target of wheel event.
>    * @param aEvent              The original Wheel event.
>-   * @param aStatus             The event status, must not be
>-   *                            nsEventStatus_eConsumeNoDefault.
>+   * @param aState              The event which should be set to the dispatching
>+   *                            event.  This also returns the dispatched event
>+   *                            state.
perhaps
inout event state to pass the old state and get new state.

Comment 39

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

And yes, it would be great to get rid of nsEventStatus. In a followup.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c16b9aeb5de9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/60c68bfe126e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97bf81b22f1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaff66026ba

Comment 41

[Jonathan Watt [:jwatt]]

Thank you both for you expedited work on this! Much appreciated!

Comment 42

[[e-mail]]

I have to say that I found the description of this bug difficult to read. Would it be fair for me to summarise it as follows:
1. When content accesses defaultPrevented, it only returns true if content called preventDefault() on the event.
2. When chrome or C++ accesses defaultPrevented, it returns true if anyone called preventDefault() on the event (in the case of C++ this refers to the version without the JS context).
3. When anyone access getPreventDefault(), it returns true if anyone called preventDefault() on the event (but this method is deprecated).

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Exactlly.

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c16b9aeb5de9
https://hg.mozilla.org/mozilla-central/rev/60c68bfe126e
https://hg.mozilla.org/mozilla-central/rev/97bf81b22f1f
https://hg.mozilla.org/mozilla-central/rev/2aaff66026ba

Comment 45

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/rev/c16b9aeb5de9