pointerup/pointerdown events are triggered when using the nodepicker
Categories
(DevTools :: Inspector, defect, P2)
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.
Reporter | ||
Comment 1•4 years ago
|
||
You can use https://bug1651033.glitch.me/ to reproduce the issue.
Comment 2•4 years ago
|
||
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.
Updated•4 years ago
|
Reporter | ||
Comment 4•4 years ago
|
||
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Backed out for failure at browser_accessibility_walker.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/ff9f096616c62af11f4406392f04f6c2c405cd5b
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cfaf034743998a1112575d7c64f5c8c675d9c8ea&searchStr=dt&selectedTaskRun=Dfge_X41T2Gf_vY_IHxXZA.0
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312608698&repo=autoland&lineNumber=21873
Reporter | ||
Comment 7•4 years ago
|
||
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?
Comment 8•4 years ago
|
||
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.
Comment 9•4 years ago
•
|
||
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.
Reporter | ||
Comment 10•4 years ago
•
|
||
(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
norSystemPrincipal
.
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 fromWidgetEvent::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
toNS_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?
Comment 11•4 years ago
|
||
(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 fromWidgetEvent::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
toNS_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.
Comment 12•4 years ago
|
||
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.
Reporter | ||
Comment 13•4 years ago
|
||
Removing r+, needs more work to avoid backout.
Comment 14•1 years ago
|
||
Hard to see other options here than tweaking the assert to check if devtools are activated for that browsing context.
Reporter | ||
Comment 15•1 years ago
|
||
I agree, although I'm not working on this at the moment. unassigning.
Description
•