1912405 - gProtectionsHandler onClick does not work due to bubbling

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P3)

Description

[Tom Schuster (MoCo)]

onClick reacts to specific ids, but if somone clicks on children elements of those elements it might not work? I need to test this.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1904798

Comment 2

[Abhishek Madan[:abhishekmadan]]

Hi, could you please verify this Paul, I think its because HTML onClicks are not allowed and EventListener would be need instead.

Comment 3

[:Gijs (he/him)]

This could only break if we changed the element on which we added the handler and passed the event along to some existing function, right? Looking at https://searchfox.org/mozilla-central/rev/b1b87f95ecea00828298d1b3cd3d8718f9fcc3fc/browser/base/content/browser-siteProtections.js#2043-2053 we only do this for onHeaderClicked which passes things to https://searchfox.org/mozilla-central/rev/b1b87f95ecea00828298d1b3cd3d8718f9fcc3fc/browser/base/content/browser-siteProtections.js#1782 which eventually passes it as triggerEvent to PanelMultiView.openPopup which won't care about the exact target ID, only about whether it was a click or keypress that triggered opening a panel.

So I think we're fine? I also audited the other patches in bug 1904798 and I think they're all good. But maybe I'm missing something?

Comment 4

[Tom Schuster (MoCo)]

So, I think the problem is the following:

• The user clicks on "Site information for example.com" (<html:span id="protections-popup-mainView-panel-header-span"/>)
• The onClick function runs
• The event.target.id is protections-popup-mainView-panel-header-span , so the case for protections-popup-mainView-panel-header doesn't match
• Thus onHeaderClicked doesn't run

Comment 5

[Paul Zühlcke [:pbz]]

I can reproduce the issue

STR:

1. Visit example.com
2. open the protections panel
3. Toggle ETP off or on
4. When the toast appears click it

Expected:
The protections panel opens

Actual: The toast disappears after a while but the protections panel does not open.

Comment 6

[Paul Zühlcke [:pbz]]

Tom since you're the author of the regressing bug, would you like to take this one?

Comment 7

[Tom Schuster (MoCo)]

Attachment: Bug 1912405 - Use individual event listeners for gProtectionsHandler click handlers. r?pbz,Gijs

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f817754b1da7
Use individual event listeners for gProtectionsHandler click handlers. r=Gijs

Comment 9

[Tom Schuster (MoCo)]

This needs a tests.

Comment 10

[Tom Schuster (MoCo)]

[Tracking Requested - why for this release]: Broken UI interactions.

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f817754b1da7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

FYI, we build beta8 tomorrow, so an uplift request soon would be appreciated!

Comment 13

[Tom Schuster (MoCo)]

Comment on attachment 9419580 [details]
Bug 1912405 - Use individual event listeners for gProtectionsHandler click handlers. r?pbz,Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Some UI interactions with the protections menu (the shield icon in the URL bar) would be broken
• 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: See https://bugzilla.mozilla.org/show_bug.cgi?id=1912405#c5
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The new code is simpler and closer to the original code
• String changes made/needed:
• Is Android affected?: No

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9419580 [details]
Bug 1912405 - Use individual event listeners for gProtectionsHandler click handlers. r?pbz,Gijs

Approved for 130.0b8.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/f988e31338b5

Comment 16

[Tom Schuster (MoCo)]

Attachment: Bug 1912405 - Test that clicking the protection mini panel opens the whole panel. r?pbz

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a26dbdf29960
Test that clicking the protection mini panel opens the whole panel. r=pbz

Comment 19

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on an affected nightly build from 2024-08-09, on Win 10 using the STR from Comment 5.
Verified as fixed on Firefox Nightly 131.0a1 (20240820214025) and Beta 130.0b8 (20240821091813) on Win 10, macOS 11 and Ubuntu 22.04. The protection panel opens when clicking the toast.

Comment 20

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/a26dbdf29960

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/214c485f2693