Closed Bug 976722 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jgruen, Assigned: mstange)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached image 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
Blocks: 969593
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.
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #8386888 - Flags: review?(roc)
Attached image screenshot with fix
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.
Component: Sync → Widget: Cocoa
Flags: needinfo?(shorlander)
Product: Firefox → Core
Attachment #8386889 - Flags: review?(dao) → review+
(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.
Flags: needinfo?(shorlander)
https://hg.mozilla.org/mozilla-central/rev/ab82acb70e68
https://hg.mozilla.org/mozilla-central/rev/d71d8f2a955d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1086960
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.
(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
This broke the ability to set a background image on the toolbox on Mac.

Any thoughts on how to work around that?
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?
(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.
Depends on: 1187064
You need to log in before you can comment on or make changes to this bug.