1718011 - Proton "browser update" popup looks like phishing malware

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P2)

Description

[gserg.g]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

Starting with 89, the Proton look is applied to not only the page dialogs created by prompt() etc, but also to the "Browser update" popup that Firefox itself generates to let you know that an update is available for the browser.

This is extremely confusing, as the browser update popup now looks very much like something a webpage might have created with prompt(), which raises suspicion immediately, and may be used for phishing.

Prior to 89, the Update browser dialog had a distinct OS look that a web page could not have created, and was clearly not a part of the web page.

Please relieve the browser update dialog of the Proton look.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Toolkit::Application Update' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

Attachment: update notification with bookmarks bar.PNG

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Attachment: update notification no bookmarks bar.PNG

Comment 4

[Robin Steuber (they/them) [:bytesized]]

Hmm, I had never noticed this because I have the bookmark bar enabled. As you can see from the attached screenshots, the doorhanger overlaps with the browser chrome when the bookmark bar is enabled. But it doesn't overlap when the bookmark bar is not enabled.

I think that we should consider doing something about this. I'm not sure whether the Application Update team ought to handle this or not. I think that historically we have owned this. But we didn't make the changes to it for Proton. I'll ask around and see if the team that made these changes could take this bug.

Comment 5

[Robin Steuber (they/them) [:bytesized]]

Attachment: doorhanger placement.png

I have talked with Romain about how this should be changed. He wants the doorhanger moved up just slightly to be right under hamburger menu button. This image illustrates the difference in where the top of the doorhanger should be.

Comment 6

[Robin Steuber (they/them) [:bytesized]]

Attachment: modal placement.png

I had concerns about how noticeable this amount of overlap would be. But apparently this is the amount of overlap used by our modal dialogs (as shown). Since this was reviewed by our security team, I will consider this amount of overlap to be acceptable.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

I've verified this problem on Windows and Linux, but it looks like the doorhanger already appears in the correct place on macOS.

The underlying problem seems to be a 4 pixel margin that the doorhanger is given that pushes it below the bottom of the chrome. I believe that the 4 pixels are needed to display a shadow "under" the doorhanger. But it looks like that wasn't done on macOS, for some reason.

I should have a patch up for this shortly.

Comment 8

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1718011 - Move doorhangers upwards so that they overlap with the browser chrome r=dao!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37df72add403
Move doorhangers upwards so that they overlap with the browser chrome r=dao

Comment 10

[Marian-Vasile Laza]

Backed out changeset 37df72add403 (Bug 1718011) for causing mochitest failures.
Backout link
Push with failures - c3, bc2
Failure Log c3
Failure Log bc2

Comment 11

[Robin Steuber (they/them) [:bytesized]]

I'll take a look.

Comment 12

[Robin Steuber (they/them) [:bytesized]]

Ok, it looks like there are two test failures here. I spent pretty much all day going down a rabbit hole trying to figure out the right thing to do for the datetime picker test failure. The root of the problem is that this code tests that the datetime picker is positioned correctly, and I have changed the positioning. It is trying to make sure that the panel lines up with anchor. I would argue that it is more correct for the content of the panel to line up with the anchor since most people, I think, would not consider the panel's shadow to be a part of the panel. I adjusted the linked code above to look more like this to check against the positioning of the content rather than the panel:

let panelBoundingRect = helper.panel.getBoundingClientRect();
let panelContent = helper.panel.shadowRoot.querySelector("::part(content)");
let contentBoundingRect = panelContent.getBoundingClientRect();
let contentOffsetX = contentBoundingRect.x - panelBoundingRect.x;
let contentOffsetY = contentBoundingRect.y - panelBoundingRect.y;
let contentScreenX = helper.panel.screenX + contentOffsetX;
let contentScreenY = helper.panel.screenY + contentOffsetY;
is_close(contentScreenX, inputRect.left, "datepicker x position");
is_close(contentScreenY, inputRect.bottom, "datepicker y position");

This fixed the test failure saying that the y positioning of the panel is wrong, but introduced a new test failure saying that the x positioning is wrong. This sort of makes sense, since I only gave the panel a negative margin in one direction: towards the side indicated by the panel's side attribute.

I spent a little time looking into what it would take to move the panel to the correct position in the other direction. There does not seem to be a convenient attribute to apply styles to in this direction, like how I used the side attribute to apply styles in the original direction. Which suggests to me that this fix will be more complicated than adding a styling rule. Actually, to be slightly more accurate, there are attributes that seem to convey some of this information, but they do not seem to be updated like the side attribute does when the panel flips.

Additionally, while messing around with the datetime picker, I discovered that the margin-bottom rule that I added doesn't seem to work properly. I don't really know why. The inspector says that the rule is active. But I still see the margin for the shadow appearing between the datetime picker and its anchor when I position the datetime picker's anchor such that the picker flips upwards.

---

At this point, I've strayed pretty far from what I set out to do here. Correctly positioning the datetime picker is WAY outside of my and my team's responsibilities. So I am going to do one of these three things:

1. Implement the above datetime test fix in the Y direction, but leave X direction test how it is now. Probably with a big ugly comment about why things are the way they are. Then spend some time on the other test failure and possibly do something just as ugly.
2. Add the update doorhanger to the list of panels that are manually put back in the correct position and leave the general problem for someone else to fix some other time.
3. If neither of the above are acceptable, I'm going to file a blocking bug about more generalized panel placement problems, file it in another component, and this is just going to have to wait on that being fixed.

@Dao - Could you tell me which of those options sounds the most attractive to you?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #12)

Additionally, while messing around with the datetime picker, I discovered that the margin-bottom rule that I added doesn't seem to work properly. I don't really know why. The inspector says that the rule is active. But I still see the margin for the shadow appearing between the datetime picker and its anchor when I position the datetime picker's anchor such that the picker flips upwards.

Hmm, I guess the XUL popup positioning code doesn't account for that. Does margin-top: 4px; work?

So I am going to do one of these three things:

1. Implement the above datetime test fix in the Y direction, but leave X direction test how it is now. Probably with a big ugly comment about why things are the way they are. Then spend some time on the other test failure and possibly do something just as ugly.
2. Add the update doorhanger to the list of panels that are manually put back in the correct position and leave the general problem for someone else to fix some other time.
3. If neither of the above are acceptable, I'm going to file a blocking bug about more generalized panel placement problems, file it in another component, and this is just going to have to wait on that being fixed.

@Dao - Could you tell me which of those options sounds the most attractive to you?

I would suggest going with option 1.

Comment 14

[Robin Steuber (they/them) [:bytesized]]

(In reply to Dão Gottwald [::dao] from comment #13)

Hmm, I guess the XUL popup positioning code doesn't account for that. Does margin-top: 4px; work?

Top margins do generally seem to work properly.

I would suggest going with option 1.

I'll do that. Thanks.

Comment 15

[Robin Steuber (they/them) [:bytesized]]

Well, I went to debug the second test, and I have once again hit a frustrating problem. I'm no longer entirely sure that my generalized patch is actually correct. That panel uses a more "traditional" arrow panel, in that it actually has an arrow. And it seems that adding the -4px margin to the panel has moved the arrow 4px too high.

I have no idea if we actually use any panels with arrows any more. But I am at the point where I do not feel that I can justify putting more time into this patch. I'm think I want to just put this panel on the list of panels individually moved back to their correct positions and be done with it.

Comment 16

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1718011 - Move update doorhangers upwards so that they overlap with the browser chrome r=dao!

Comment 17

[Dão Gottwald [:dao]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #15)

I have no idea if we actually use any panels with arrows any more.

We don't. I'll try to clean this up in https://bugzilla.mozilla.org/show_bug.cgi?id=1734835.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43f66f1b4f39
Move update doorhangers upwards so that they overlap with the browser chrome r=dao

Comment 19

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/43f66f1b4f39