607252 - Doorhanger arrow is within content area, so easy to simulate by a webpage

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Alfred Kayser]

Attachment: Doorhanger in default theme

See attached images.

How to reproduce:
Disabled bookmarks toolbar, use default theme in Windows (but other may have same problem).
Drag&drop an extension into the webpage to install it, or
go to addons.mozilla.org to install an extension.

Result:
The arrow/doorhanger panel is completely within the content space.
This allows websites to simulate this panel.

Expected result: 
The arrow is on top of Firefox chrome.

Comment 1

[Alfred Kayser]

Attachment: How it should look (using Nautipolis theme)

Comment 2

[pino_sb]

You are right at the moment, but this is due to a bug: the arrow is misaligned: https://bugzilla.mozilla.org/show_bug.cgi?id=606343 
The arrow is supposed to be over the chrome as in your second screenshot. When the bug I mentioned is fixed, this should no longer be a problem.

Comment 3

[:Margaret Leibovic]

I think we just need to fix bug 606343. We could use a negative margin in the browser theme code for a temporary fix, but that won't fix the underlying alignment problem.

Comment 4

[:Margaret Leibovic]

I'm marking this as a dupe because bug 606343 will fix the problem.

Comment 5

[Neil Deakin]

Did you mean a different bug? I don't plan on fixing this in bug 606343.

Comment 6

[:Margaret Leibovic]

(In reply to comment #5)
> Did you mean a different bug? I don't plan on fixing this in bug 606343.

Once bug 606343 is fixed this won't be a problem anymore, since the arrow will point to the anchor and overlap with the browser chrome. I saw this when I applied the patch from bug 606343, so I think you are fixing the problem :)

I'll leave this bug open for now, in case bug 606343 somehow doesn't manage to fix it, but I think it will.

Comment 7

[Neil Deakin]

OK, a specific case of this was caused by the extra padding on Windows, which I've needed to remove to fix an issue in the patch for bug 606343.

Is this bug referring to more that this though, on other platforms?

Comment 8

[:Margaret Leibovic]

(In reply to comment #7)
> OK, a specific case of this was caused by the extra padding on Windows, which
> I've needed to remove to fix an issue in the patch for bug 606343.
> 
> Is this bug referring to more that this though, on other platforms?

I don't think this problem occurs on other platforms. When I was working on my other styling patches I experimented with removing that padding to fix this problem, but Dão and I decided to leave it alone in the hopes that it would be fixed by bug 606343.

With that change, are any of the panels still completely in the content area?

Comment 9

[Justin Dolske [:Dolske]]

I think we should go ahead and close this, unless you're wanting to use this bug to address a particular remaining style glitch.

Doorhangers are not being used for the input of sensitive info, so the most a spoofed doorhanger can do is maybe get a user to click an unprivledged button in content. Given the widespread fake "your computer has a virus" OS dialogish tricks, I don't think doorhangers are a particularly interesting vector for spoofing anyway.

Comment 10

[:Margaret Leibovic]

I'm duping this bug to bug 606343, since the patch for that bug fixed the problem.

Comment 11

[Dão Gottwald [:dao]]

Not fixed for me with small icons and the bookmarks toolbar hidden.

Comment 12

[:Margaret Leibovic]

Attachment: arrow point on Windows vs OSX (small icons enabled)

The tip of the arrow points to a different part of the anchor icon on Windows and OSX. I think we should try to get the Windows popup to do what the OSX popup is doing.

Enn, would you know how to adjust this? I tried adding a negative margin to the panel in popup.css, but that caused the arrow to disappear sometimes. It seemed to alternately appear and disappear every time I opened the panel. Is there a bug on that?

Comment 13

[:Margaret Leibovic]

Attachment: patch

This patch moves the arrow point closer to the center of the anchor. I don't know if this is a hacky approach, since pinstripe doesn't need these margins to achieve the same effect, but it gets the job done.

Comment 14

[:Margaret Leibovic]

It seems like the reason for the difference on Windows is just that the arrow image includes more pixels of shadow by the point of the arrow, so it looks farther away from the anchor icon. Therefore, I think that this negative margin is the best bet to fix that (or we could change the images, but I think we still want that shadow to be there).

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 495985 [details] [diff] [review]
patch

Need to do the same for side=left/right. And the margin should probably be set on the panel rather than arrow?

Comment 16

[:Margaret Leibovic]

Attachment: patch v2

Here's a patch that includes the same negative margins for left/right arrows.

I first tried setting the margin on the panel itself, but that caused the arrow to disappear. I agree that seems like a more sensible place for the margin, but the panels behave correctly when the margin is set on the arrows.

Comment 17

[Neil Deakin]

Comment on attachment 496087 [details] [diff] [review]
patch v2

Does this affect both Windows 7 and XP?

Comment 18

[:Margaret Leibovic]

Attachment: patch v3

The old patch affected Windows 7 and XP the same, but testing on XP, I realized that the arrow was 1px too high, so I changed the margin to -4px for that platform. I'll upload a screenshot for ui-review, but here's the revised patch.

Comment 19

[:Margaret Leibovic]

Attachment: screenshot with new patch (osx arrow included for comparison)

Comment 20

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/90f6cd0ff3e7