Right clicking on preview pane elements behaves unexpectedly
Categories
(Toolkit :: Printing, defect, P1)
Tracking
()
People
(Reporter: zeid, Assigned: jaws)
References
Details
(Whiteboard: [print2020_v81])
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
When I right click on the preview pane in the print preview modal, the context menu shows up in a different location (on the very right of the screen). It appears that the context menu is also based on the HTML element being clicked on (for example, clicking on a link will show menu items to follow the link, etc.)
In the screenshot attached, I right clicked on the red "COVID-19" image in the preview pane.
Expected behaviour: I expected not to be able to right click in the preview pane.
Using 81.0a1 (2020-08-05) (64-bit) on macOS Catalina 10.15.5.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 1•4 years ago
|
||
It looks like the preview being interactable is causing a number of accessibility issues, so I'm bumping up the priority of this bug, and will be linking some of those to this one.
Comment 2•4 years ago
|
||
There's a flip side to this. If the preview isn't focusable at all (even via f6), that would make it impossible for sighted keyboard only users to scroll the preview, assuming that works now. That's also an accessibility issue, albeit not one that impacts screen reader users. It's probably a less common requirement and thus lower severity, though.
Comment 3•4 years ago
|
||
Drats. Good point. Okay, we'll figure out a separate fix for bug 1660363… Thanks!
Comment 4•4 years ago
|
||
In case it's of any help, setting tabindex="-1" on the browser element prevents tabbing from focusing it, but it also prevents f6 from focusing it. It does not prevent right clicking from accessing the context menu or focusing the document.
Comment 5•4 years ago
|
||
Per team discussion, we are bumping this down to P2. Ideally we fix this and the accessibility issues, but if we have to choose for the initial release it's better for it to be accessible with a misplaced context menu than the inverse.
Comment 6•4 years ago
|
||
Updated•4 years ago
|
Comment 7•4 years ago
|
||
(In reply to Sean Voisen (:svoisen) from comment #5)
Ideally we fix this and the accessibility issues, but if we have to choose for the initial release it's better for it to be accessible with a misplaced context menu than the inverse.
For the record, the patch here doesn't break keyboard a11y, since it doesn't make it unfocusable.
Comment 9•4 years ago
|
||
Backed out 4 changesets (bug 1657459, bug 1660359) for browser_modal_print.js failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/44ee384376ce8b20ef64db6e4fef50af983c2088
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314153840&repo=autoland&lineNumber=23157
...
[task 2020-08-27T05:53:15.602Z] 05:53:15 INFO - TEST-INFO | Main app process: exit 0
[task 2020-08-27T05:53:15.603Z] 05:53:15 INFO - TEST-INFO | Confirming we saw 59 DOCSHELL created and 59 destroyed log strings.
[task 2020-08-27T05:53:15.603Z] 05:53:15 INFO - TEST-INFO | Confirming we saw 166 DOMWINDOW created and 166 destroyed log strings.
[task 2020-08-27T05:53:15.604Z] 05:53:15 ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = chrome://global/content/print.html?browsingContextId=51]
[task 2020-08-27T05:53:15.604Z] 05:53:15 ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = about:blank]
[task 2020-08-27T05:53:15.604Z] 05:53:15 ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = chrome://global/content/print.html?browsingContextId=45]
[task 2020-08-27T05:53:15.605Z] 05:53:15 INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | windows(s) leaked: [pid = 18300] [serial = 20], [pid = 18300] [serial = 23], [pid = 18300] [serial = 18], [pid = 18300] [serial = 25], [pid = 18300] [serial = 16], [pid = 18300] [serial = 13]
[task 2020-08-27T05:53:15.605Z] 05:53:15 ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 docShell(s) until shutdown
[task 2020-08-27T05:53:15.605Z] 05:53:15 INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | docShell(s) leaked: [pid = 18300] [id = {30261d5e-05f0-4696-8924-3d11a57203cd}], [pid = 18300] [id = {d3e90e56-acaf-416d-a417-471c29528435}]
[task 2020-08-27T05:53:15.606Z] 05:53:15 INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | This test created 1 hidden window(s)
[task 2020-08-27T05:53:15.606Z] 05:53:15 INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | This test created 1 hidden docshell(s)
...
Comment 11•4 years ago
|
||
The failure (leaked windows) in comment 9 was definitely due to this patch, not the patches in the other bug.
I really don't know how to fix this and I'm starting to wonder whether it's a platform bug. I've tried a bunch of things with no success:
- I wondered whether using an arrow function was causing a reference cycle which for some reason couldn't be cleaned up. So, I tried adding PrintEventHandler as the handler and implementing handleEvent.
- In addition to 1), I tried removing the event listener in PrintEventHandler.unload.
- In addition to 1) and 2), I wondered whether this was a quirk caused by the test, so I tried clearing the test's reference to the preview browser before closing the dialog.
- In addition to 1) and 2), I tried clearing the preview browser in unload and moved the removeEventListener before the async preview exit message.
I'm out of ideas here, and unless there's a simple fix here someone knows of that I'm missing, I think I'm unfortunately going to need to move onto something else. The patch here does "work" from the user perspective, but I obviously can't land it with these test failures (and the failures might indicate a real leak).
Another option (even an interim one) might be to go back to my original patch. That is, rather than adding an event listener, instead handle this in ContextMenuChild.jsm. That might not suffer from this problem, though I haven't tested that.
NI :jwatt as requested on Matrix.
Assignee | ||
Comment 12•4 years ago
|
||
Comment 13•4 years ago
|
||
Updated•4 years ago
|
Comment 14•4 years ago
|
||
bugherder |
Comment 16•4 years ago
|
||
Comment on attachment 9173276 [details]
Bug 1657459 - Remove context menu from print preview browser.
Beta/Release Uplift Approval Request
- User impact if declined: Users will see unusable context menu, it's totally bogus
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: Right click on the contents in the print preview window
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): the change is pretty simple, it just disabled context menu in the print preview
- String changes made/needed: none
Updated•4 years ago
|
Comment 17•4 years ago
|
||
(In reply to Sean Voisen (:svoisen) from comment #15)
Do we want to uplift this one?
Sure, I did request (just in case Jamie is AFK, it's on Thursday)
Comment 18•4 years ago
|
||
Note that the final patch which landed here was from :Jaws, not me. I've updated the assignee accordingly. I agree it is low risk.
Updated•4 years ago
|
Comment 19•4 years ago
|
||
Comment on attachment 9173276 [details]
Bug 1657459 - Remove context menu from print preview browser.
Approved for 81.0b6.
Comment 20•4 years ago
|
||
bugherder uplift |
Comment 21•4 years ago
|
||
Verified fixed with 82.0a1 (2020-09-04) and Fx 81.0b6 on Windows 10, macOS 10.14 and Ubuntu 18.04.
Updated•3 years ago
|
Description
•