Open Bug 1651033 Opened 4 years ago Updated 3 months ago

pointerup/pointerdown events are triggered when using the nodepicker

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

People

(Reporter: jdescottes, Unassigned)

References

()

Details

Attachments

(1 file)

When using the nodepicker to select an element, the content page should normally not receive any click event when picking the node, so that nothing is modified on the page when the user "clicks" in order to pick an element.

However it seems we still fire the pointerup/pointerdown events. So if the page reacts both to clicks and to pointer events, picking a node will still trigger unexpected behavior in the page.

This was initially reported on discourse: https://discourse.mozilla.org/t/issue-not-all-click-events-blocked-when-using-the-node-picker-select-element-tool/63308

As you can see in the example page provided on discourse, this can lead to navigate when picking a node.

I will attach a smaller test case.

You can use https://bug1651033.glitch.me/ to reproduce the issue.

Thank for logging the issue and filing a test case, Julian!

Indeed, the node picker does not explicitly handle pointer events:
https://searchfox.org/mozilla-central/rev/91d82d7cbf05a71954dfa49d0e43824c7c973e62/devtools/server/actors/highlighters.js#472-501

Seems like an easy fix to include them.

Severity: -- → S3
Depends on: 1607756
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cfaf03474399 Prevent default for pointerup/down events with DevTools node picker r=gl

The assert at https://searchfox.org/mozilla-central/rev/8df04ff073d0fa70f692935c5dcddc0e23cb0117/widget/WidgetEventImpl.cpp#574-581 makes this patch fail on DEBUG builds:

  if (mMessage == ePointerDown) {
    if (aCalledByDefaultHandler) {
      // Shouldn't prevent default on pointerdown by default handlers to stop
      // firing legacy mouse events. Use MOZ_ASSERT to catch incorrect usages
      // in debug builds.
      MOZ_ASSERT(false);
      return;
    }
    // ...
  }

This was implemented in Bug 1355497
smaug, masayuki, DevTools wants to prevent pointerdown/up events when users are using the node pickers, in the same way we prevent click events. We call preventDefault in two locations, from the content process (1, 2).

The aCalledByDefaultHandler boolean comes from the following check aCallerType == CallerType::System and I guess for devtools code running in the content process this will be false. How do you suggest to proceed here? Should we expose a chrome-only argument to preventDefault to bypass this?

Flags: needinfo?(masayuki)
Flags: needinfo?(jdescottes)
Flags: needinfo?(bugs)

Well, looks like that devtools runs as chrome script.

As explained as the inline comment, basically, pointerdown event shouldn't be canceled by chrome script because it can cause breaking web apps accidentally. On the other hand, indeed, this is an exception which needs to prevent pointerdown. If it can check whether aPrincipal is for devtools or not, it should be checked by the MOZ_ASSERT, I think.

Flags: needinfo?(masayuki)

It seems that we cannot distinguish whether from devtools or not with nsIPrincipal nor SystemPrincipal.
But there is an API BrowsingContext::WatchedByDevTools(). Cannot access this from WidgetEvent::mTarget?

If it were impossible, I'd recommend that change the MOZ_ASSERT to NS_ASSERTION because the assertion count is checked by automated tests in debug build. And please add automated tests for this. This should've been detected before land the patch.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

It seems that we cannot distinguish whether from devtools or not with nsIPrincipal nor SystemPrincipal.

That sounds right, I'll ask around but I don't think we can check if a principal is related to devtools or not.
Regarding the initial intent of Bug 1355497

But there is an API BrowsingContext::WatchedByDevTools(). Cannot access this from WidgetEvent::mTarget?

That's an option, but this means that as soon as DevTools are open, all chrome scripts could call preventDefault() for pointer events on documents watched by DevTools, which seems a bit risky?

If it were impossible, I'd recommend that change the MOZ_ASSERT to NS_ASSERTION because the assertion count is checked by automated tests in debug build. And please add automated tests for this. This should've been detected before land the patch.

The patch was backed out because tests in debug builds failed. But you bring an interesting point: I manually tested the fix on an opt build before landing, and it works fine. You can test it on https://bug1651033.glitch.me/ . So I could have added a test, but again it would have passed on opt builds.

So appart from the the MOZ_ASSERT check performed here on debug builds, nothing prevents DevTools code from using preventDefault() for pointerdown/up events. I wonder why this is the case?

Also reading the original bug, it seems part of the logic added was for addons, and another part was for webextensions. Now that we only have webextensions, could we remove the check for chrome scripts? Based on comments 9 to 12 from Bug 1355497, I am not sure if webextensions can call preventDefault on content events from chrome scripts?

(In reply to Julian Descottes [:jdescottes] from comment #10)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

But there is an API BrowsingContext::WatchedByDevTools(). Cannot access this from WidgetEvent::mTarget?

That's an option, but this means that as soon as DevTools are open, all chrome scripts could call preventDefault() for pointer events on documents watched by DevTools, which seems a bit risky?

Hmm, indeed.

If it were impossible, I'd recommend that change the MOZ_ASSERT to NS_ASSERTION because the assertion count is checked by automated tests in debug build. And please add automated tests for this. This should've been detected before land the patch.

The patch was backed out because tests in debug builds failed. But you bring an interesting point: I manually tested the fix on an opt build before landing, and it works fine. You can test it on https://bug1651033.glitch.me/ . So I could have added a test, but again it would have passed on opt builds.

Ah, as a Gecko's developer, testing with debug build is usual scenario, but not so for UI developers because of artifact build.

So appart from the the MOZ_ASSERT check performed here on debug builds, nothing prevents DevTools code from using preventDefault() for pointerdown/up events. I wonder why this is the case?

The motivation of doing the check is, chrome UI and default action handler in Gecko shouldn't prevent the default of pointerdown and detects it before shipping Nightly builds because such bug makes Nightly testers cannot do something with their pointing device. So, I think that it's better way that chaning MOZ_ASSERT to NS_ASSERTION and allow the assertion in each test which affects the node-picker.

Also reading the original bug, it seems part of the logic added was for addons, and another part was for webextensions. Now that we only have webextensions, could we remove the check for chrome scripts? Based on comments 9 to 12 from Bug 1355497, I am not sure if webextensions can call preventDefault on content events from chrome scripts?

If possible, I don't want to remove it because somebody may handle pointerdown accidentally and its symptom may be too serious even for Nightly testers. Intentional use like this bug is fine, but unexpected case should be detectable with automated tests.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jdescottes, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(jdescottes)

Removing r+, needs more work to avoid backout.

Flags: needinfo?(jdescottes)

Hard to see other options here than tweaking the assert to check if devtools are activated for that browsing context.

Flags: needinfo?(smaug)

I agree, although I'm not working on this at the moment. unassigning.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
See Also: → 1913263
See Also: 1913263
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: