Closed Bug 607252 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: XUL Widgets, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: alfredkayser, Assigned: Margaret)

References

Details

(Whiteboard: [doorhanger])

Attachments

(5 files, 2 obsolete files)

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.
Assignee: nobody → enndeakin
Blocks: 554937
blocking2.0: --- → ?
Depends on: 606343
Assignee: enndeakin → nobody
Component: XP Toolkit/Widgets: XUL → XUL Widgets
Product: Core → Toolkit
QA Contact: xptoolkit.xul → xul.widgets
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.
Assignee: nobody → margaret.leibovic
blocking2.0: ? → final+
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.
I'm marking this as a dupe because bug 606343 will fix the problem.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 606343
Did you mean a different bug? I don't plan on fixing this in bug 606343.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 597557
(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.
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?
(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?
Whiteboard: [should be fixed by 606343]
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.
I'm duping this bug to bug 606343, since the patch for that bug fixed the problem.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → DUPLICATE
Whiteboard: [should be fixed by 606343]
Duplicate of bug: 606343
Not fixed for me with small icons and the bookmarks toolbar hidden.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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?
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #495985 - Flags: review?(dao)
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 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?
Attachment #495985 - Flags: review?(dao) → review-
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #495985 - Attachment is obsolete: true
Attachment #496087 - Flags: review?(dao)
Attachment #496087 - Flags: review?(dao) → review?(enndeakin)
Comment on attachment 496087 [details] [diff] [review]
patch v2

Does this affect both Windows 7 and XP?
Attachment #496087 - Flags: review?(enndeakin) → review+
Attached patch patch v3Splinter Review
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.
Attachment #496087 - Attachment is obsolete: true
Attachment #496171 - Flags: review?(enndeakin)
Attachment #496175 - Flags: ui-review?(shorlander)
Attachment #496171 - Flags: review?(enndeakin) → review+
Attachment #496175 - Flags: ui-review?(shorlander) → ui-review?(faaborg)
Attachment #496175 - Flags: ui-review?(faaborg) → ui-review+
Blocks: 615476
http://hg.mozilla.org/mozilla-central/rev/90f6cd0ff3e7
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [doorhanger]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.