Closed Bug 930374 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: [qa-])

Attachments

(6 files, 6 obsolete files)

part.3 Make test_*editor_keyevent_handling.html chrome tests since they tests behavior of system group event handler
4.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.4 Fix new orange of test_bug448987.html on Mac since it doesn't set tab navigation setting
3.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Screenshot at new orange of test_assign_event_data.html
1.81 MB, image/png
Details
part.2 Fix test_assign_event_data.html for new defaultPrevented behavior and make it a plain test
15.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler
18.64 KB, patch
Details | Diff | Splinter Review
part.1 Event.defaultPrevented should be false even if preventDefault() has been called by default action handler
25.72 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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.
Attached patch Patch (obsolete) — — Splinter Review 
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.
Attachment #824060 - Flags: review?(bugs)
Status: NEW → ASSIGNED
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.
Attachment #824060 - Flags: review?(bugs) → review-
(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.
We can have JS-only version of DefaultPrevented() if needed.
Just map the webidl's defaultPrevented to some FooBarJS() method using Bindings.conf
Attached patch Patch (obsolete) — — Splinter Review 
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.
Attachment #824060 - Attachment is obsolete: true
Attachment #824638 - Flags: feedback?(bugs)
Or, the tests should be switched back to mochitest-chrome again because they test chrome level behavior.
Perhaps, we should make the editor tests chrome tests.
Attachment #824638 - Attachment is obsolete: true
Attachment #824638 - Flags: feedback?(bugs)
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 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.
Attachment #825282 - Flags: review?(bugs)
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.
Attachment #825349 - Flags: review?(bugs)
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.
Attachment #825284 - Flags: review?(bugs)
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)
Attachment #825285 - Flags: review?(bugs)
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 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.
Attachment #825284 - Flags: review?(bugs) → review+
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?
Attachment #825285 - Flags: review?(bugs) → review+
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
Attachment #825349 - Flags: review?(bugs) → review-
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.
Attachment #825282 - Flags: review?(bugs) → review-
Blocks: 935506
(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?
Flags: needinfo?(bugs)
xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))
Flags: needinfo?(bugs)
(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?
Flags: needinfo?(bugs)
Oh right, that is main thread only. workers are simpler.

mIsMainThreadEvent ? xpc::AccessCheck::isChrome(js::GetContextCompartment(cx)) :
                     mozilla::dom::workers::IsCurrentThreadRunningChromeWorker();
Flags: needinfo?(bugs)
(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!
Attachment #825349 - Attachment is obsolete: true
Attachment #8342321 - Flags: review?(bugs)
Blocks: 946390
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)) {
Attachment #8342320 - Flags: review?(bugs) → review-
Attachment #8342321 - Flags: review?(bugs) → review+
(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 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);
Attachment #8343439 - Flags: review?(bugs)
And yes, that would require to remove that drag event special case.
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??
Flags: needinfo?(bugs)
Could that change be a follow-up bug?
Using mDefaultPreventedByContent should be safe. It is used currently only in one place in ESM.
Flags: needinfo?(bugs)
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.
Attachment #8344272 - Flags: review?(bugs)
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.
Attachment #8344272 - Flags: review?(bugs) → review+
And yes, it would be great to get rid of nsEventStatus. In a followup.
Thank you both for you expedited work on this! Much appreciated!
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).
Depends on: 948839
Depends on: 948864
https://hg.mozilla.org/mozilla-central/rev/c16b9aeb5de9
Flags: in-testsuite+
Whiteboard: [qa-]
Depends on: 985463
Depends on: 985988
Depends on: 996185
No longer depends on: 996185
Blocks: 1024488
No longer depends on: 933303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: