Closed Bug 1657459 Opened 1 year ago Closed 1 year ago

Right clicking on preview pane elements behaves unexpectedly

Categories

(Toolkit :: Printing, defect, P1)

Firefox 81
x86_64
macOS
defect

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox81 --- verified
firefox82 --- verified

People

(Reporter: zeid, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [print2020_v81])

Attachments

(2 files, 1 obsolete file)

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.

Blocks: 133787
Severity: -- → S4
Priority: -- → P3
Whiteboard: [print2020_v81]

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.

Severity: S4 → S1
Priority: P3 → P1

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.

Drats. Good point. Okay, we'll figure out a separate fix for bug 1660363… Thanks!

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.

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.

Priority: P1 → P2
Assignee: nobody → jteh
Status: NEW → ASSIGNED

(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.

No longer blocks: 1660363
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/971e2d1a6140
Don't show context menus in print preview documents. r=mstriemer

Backed out 4 changesets (bug 1657459, bug 1660359) for browser_modal_print.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-fis-e10s%2Cbc3&fromchange=da1424ee1d1106d720c4542773dda1d9a4720e4b&tochange=44ee384376ce8b20ef64db6e4fef50af983c2088&selectedTaskRun=K21W1kmUSmuwXJesCqRCGQ.0

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)
...
Flags: needinfo?(jteh)
Duplicate of this bug: 1661647

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:

  1. 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.
  2. In addition to 1), I tried removing the event listener in PrintEventHandler.unload.
  3. 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.
  4. 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.

Flags: needinfo?(jteh) → needinfo?(jwatt)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1994875aa273
Remove context menu from print preview browser. r=Jamie
Attachment #9172073 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Do we want to uplift this one?

Flags: needinfo?(jteh)

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
Attachment #9173276 - Flags: approval-mozilla-beta?
Flags: qe-verify+

(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)

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.

Assignee: jteh → jaws
Flags: needinfo?(jteh)
QA Whiteboard: [qa-triaged]

Comment on attachment 9173276 [details]
Bug 1657459 - Remove context menu from print preview browser.

Approved for 81.0b6.

Attachment #9173276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified fixed with 82.0a1 (2020-09-04) and Fx 81.0b6 on Windows 10, macOS 10.14 and Ubuntu 18.04.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(jwatt)
Priority: P2 → P1
You need to log in before you can comment on or make changes to this bug.