976722 - OSX Merge Warning Sheet should appear at the bottom of browser chrome, not top or middle

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[John Gruen [:jgruen]]

Attachment: sheet-position.001.jpg

Current Behavior
--
Merge warning sheet dialog appears from the top or middle of the browser chrome. 

Expected Behavior
--
Dialog should appear from the bottom edge.

see attachment for details

Comment 1

[Markus Stange [:mstange]]

This affects all sheets in the browser, not only those by Firefox Sync.

At the moment the sheet opening position is tied to the unified titlebar/toolbar gradient. This gradient only covers the tab bar in tabs-on-top mode. If the sheet should open further down, we need a new -moz-appearance value, maybe something like -moz-mac-toolbox-container, or -moz-mac-sheet-attachment-edge, or maybe -moz-mac-sheet-containing-area for the inverse.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 1: Make -moz-appearance:toolbox influence the sheet attachment edge

I decided to just use -moz-appearance:toolbox for this. It wasn't supported on Mac before, there was just some dead rendering code which I'm removing.

Comment 3

[Markus Stange [:mstange]]

Attachment: part 2: Set toolbox { -moz-appearance: toolbox; }

Comment 4

[Markus Stange [:mstange]]

Attachment: screenshot with fix

Comment 5

[Justin Dolske [:Dolske]]

Hmm, I thought one of the reasons why these prompts are attached to chrome is to prevent spoofing by content -- for example, it makes it clear that an HTTP Auth prompt is not being faked by a web page. (See also some redesign work starting in bug 977037). I'm not sure it makes sense to remove that.

I think the updated attachment point is correct for OS X apps, but... These prompts are kind of strange for a web browser, because you generally don't want to be confusing unprivileged prompts from content with privileged prompt from chrome. By attaching them to the line between the two, both cases are unclear. I wonder if that's part of why Chrome and Safari switched to popup windows instead of sheets?

Perhaps the better question is why Sync is wanting to add window-modal prompt in the first place. Seems like this should just be a tab-modal prompt, so that it doesn't block user interaction with the rest of the browser.

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Hmm, I thought one of the reasons why these prompts are attached to chrome
> is to prevent spoofing by content -- for example, it makes it clear that an
> HTTP Auth prompt is not being faked by a web page. (See also some redesign
> work starting in bug 977037). I'm not sure it makes sense to remove that.

To be honest I don't know why it goes over the rest of the chrome. I don't know of any conscious choice to avoid spoofing. I always figured it was a bug in that it originates from the titlebar instead of our chrome.

I don't think spoofing is a huge issue here. It's a native dialog that behaves in a native way. Sure someone could fake that but we have plenty of easier places where someone could spoof our UI e.g. Infobars. Tab modal prompts could also be spoofed. 

> I think the updated attachment point is correct for OS X apps, but... These
> prompts are kind of strange for a web browser, because you generally don't
> want to be confusing unprivileged prompts from content with privileged
> prompt from chrome. By attaching them to the line between the two, both
> cases are unclear. I wonder if that's part of why Chrome and Safari switched
> to popup windows instead of sheets?

AFAIK only Chrome has switched to tab model prompts and only for HTTP auth.

> Perhaps the better question is why Sync is wanting to add window-modal
> prompt in the first place. Seems like this should just be a tab-modal
> prompt, so that it doesn't block user interaction with the rest of the
> browser.

I am not generally in favor of window modal prompts but I think in this case it is the correct usage. The action here applies to your Firefox account covering the entire application not just the FxA sign-in tab. Considering the implications it's also probably something we want to encourage a prompt answer to.

Comment 7

[Markus Stange [:mstange]]

Attachment: part 1: Make -moz-appearance:toolbox influence the sheet attachment edge, r=roc

updated

Comment 8

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab82acb70e68
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71d8f2a955d

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ab82acb70e68
https://hg.mozilla.org/mozilla-central/rev/d71d8f2a955d

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: Mispositioned Sheet

I am still seeing this on Nightly.

Comment 11

[Markus Stange [:mstange]]

What OS X version are you on? I'm on 10.10.3 Beta, maybe our positioning hint was ignored on earlier 10.10 versions.

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Markus Stange [:mstange] from comment #11)
> What OS X version are you on? I'm on 10.10.3 Beta, maybe our positioning
> hint was ignored on earlier 10.10 versions.

10.10.2

Comment 13

[Mike Kaply [:mkaply]]

This broke the ability to set a background image on the toolbox on Mac.

Any thoughts on how to work around that?

Comment 14

[Markus Stange [:mstange]]

You could set -moz-appearance: none and live with the wrong sheet position. Or we could special case some -moz-appearance values on some platforms to apply in addition to background instead of replacing it. But the latter requires Gecko changes and wouldn't easily be upliftable. What versions do you need this fixed in?

Comment 15

[Mike Kaply [:mkaply]]

(In reply to Markus Stange [:mstange] from comment #14)
> You could set -moz-appearance: none and live with the wrong sheet position.
> Or we could special case some -moz-appearance values on some platforms to
> apply in addition to background instead of replacing it. But the latter
> requires Gecko changes and wouldn't easily be upliftable. What versions do
> you need this fixed in?

I'll figure out a workaround for current versions, but it would be nice to have this fixed at some point. I'll open a separate bug.

I'm also wondering if it broke any themes...

I'll open a separate bug.