613704 - Improve visual design for tab-modal buttons & dialog

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement)

Description

[Justin Dolske [:Dolske]]

Spun off from bug 598786...

The native buttons styles look kind of odd in the tab-modal prompts, especially on OS X. We want the prompts to look less like system dialogs anyway.

Comment 1

[Josh Tumath]

I assume you'll be using ones identical to the ones used in bug 601022?

Comment 2

[:Margaret Leibovic]

Attachment: patch

To address bug 598786 comment 36, we want these buttons to look the same on all platforms, since the goal is to have the prompts look like they're coming from the page, rather than the OS.

I talked with Stephen about this, and he said that these styles are correct.

Comment 3

[Dão Gottwald [:dao]]

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

It's a fallacy to think that native widget theming represents OS UI. It is in fact used for web pages.

I'm fine with doing this for OS X, as that has a tradition of context-sensitive button styling.

>--- a/toolkit/themes/pinstripe/global/tabprompts.css
>+++ b/toolkit/themes/pinstripe/global/tabprompts.css

>+tabmodalprompt button {

Make it just "button {"?

Focus styling appears to be missing.

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to comment #3)
> Comment on attachment 494767 [details] [diff] [review]
> patch
> 
> It's a fallacy to think that native widget theming represents OS UI. It is in
> fact used for web pages.

I didn't mean to imply that native buttons never appear in webpages. Just that it isn't the norm nor does it necessarily feel "weblike".

> I'm fine with doing this for OS X, as that has a tradition of context-sensitive
> button styling.

This sounds like a good solution to me :)

We aren't going to get the desired effect with this anyway. Any style we come up with could just as easily collide with the page's style and feel even more awkward.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Morphing this bug slightly, as we have a much more simplified and streamlined version of the dialogs after some design experiments by yours truly & shorlander over the past few days. (both the buttons and the dialog itself)

I'll let shorlander attach the new patch, and screenshots from the various platforms.

PS: Don't forget to change the font, shorlander — everything else looked great!

(preview: it looks like this: http://grab.by/8k2G — shorlander has patch + the other platforms)

Comment 6

[Frank Yan (:fryn)]

Just a heads up: I'm rewriting the tab-modal prompt XUL in HTML (hard blocker), so that will affect how the patch for this bug is written.

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: Changes to the tabmodal dialog appearance

Changes to the tabmodal prompt as discussed in IRC:

- Removed background "spotlight" effect
- Remove inset shadow and bottom inset highlight
- Remove icon
- Fix margins/padding
- Custom buttons for OS X

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: Screenshot of changes

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 503565 [details] [diff] [review]
Changes to the tabmodal dialog appearance

Basically looks fine, the one general comment would be that I think the Windows dialog should continue to have the buttons centered at the bottom, and so that's where Windows users will expect them to be.

Comment 10

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 503565 [details] [diff] [review]
Changes to the tabmodal dialog appearance

Looks great — the font seems a bit small on OS X. We shouldn't have any UI text that is less than 12px for things that expect to actually be read. :)

I also agree with dolske that we should respect button placement on Windows & OS X.

ui-review+ with those changes.

Comment 11

[Ethan Sisson]

Mac buttons need more top/bottom padding, IMO. Looks odd as it is.

Comment 12

[Frank Yan (:fryn)]

(In reply to comment #6)
> Just a heads up: I'm rewriting the tab-modal prompt XUL in HTML (hard blocker),
> so that will affect how the patch for this bug is written.

The tab-modal prompt XUL ended up not being rewritten in HTML, so most of this patch should still work. I'd be happy to fix up the final patch (after review comments are addressed) so that it applies without errors on top of the patch for bug 613749. :)

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 503565 [details] [diff] [review]
Changes to the tabmodal dialog appearance

Changing to feedback+, as discussed earlier... System prompts have the buttons right-aligned on Windows these days (> XP?), so my objection was rather dated. I'd still quibble a bit, perhaps, on not using the darker bottom on OS X, as some might object to it being too Windowsy, but meh. The dialog starts to look disturbingly barren without the icon, so it helps as a bit of style.

Comment 15

[Justin Dolske [:Dolske]]

Also, filed bug 627280 on fixing dialog button alignment for Windows.

Comment 16

[Gábor Stefanik]

Since we are now doing all platforms in this bug, changing platform to All (in hopes of avoiding bug 620614-esque dupes).

Comment 17

[Frank Yan (:fryn)]

(In reply to comment #14)
> The dialog starts to look
> disturbingly barren without the icon, so it helps as a bit of style.

The darker background around the buttons helps with that too.

Comment 18

[Frank Yan (:fryn)]

Stephen, I just pushed the patch for bug 613704, which also removed the icons from the dialogs. Unfortunately, that entirely bitrots your patch for this bug. Let me know if you need help fixing it up.

Comment 19

[Frank Yan (:fryn)]

(In reply to comment #9)
> Comment on attachment 503565 [details] [diff] [review]
> Changes to the tabmodal dialog appearance
> 
> Basically looks fine, the one general comment would be that I think the Windows
> dialog should continue to have the buttons centered at the bottom, and so
> that's where Windows users will expect them to be.

Modal dialog buttons are right-aligned in IE 9, Chrome 9, and all the major Windows applications I've used.

Comment 20

[Frank Yan (:fryn)]

Attachment: patch v2

Unbitrotted, cleaned up, and simplified the code.

Stephen, why are both :focus and [focused=true] necessary for the button styling?

(In reply to comment #10)
> Comment on attachment 503565 [details] [diff] [review]
> Changes to the tabmodal dialog appearance
> 
> Looks great — the font seems a bit small on OS X. We shouldn't have any UI text
> that is less than 12px for things that expect to actually be read. :)

Addressed.

> I also agree with dolske that we should respect button placement on Windows &
> OS X.

As I noted in comment 19, the patch already respected button placement.

Comment 21

[Dão Gottwald [:dao]]

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

> tabmodalprompt {
>+  font-family: sans-serif;

What exactly is this trying to achieve?

>+.topContainer {
>+  padding: 20px 25px 20px 20px;

Is this correct for RTL?

>+tabmodalprompt button {

This stylesheet can't affect anything outside of tabmodalprompt.

> .mainContainer {
>+  background-color: hsla(0,0%,100%,.95);

The 5% transparency still triggers grayscale anti-aliasing on text, which looks pretty bad at least on Windows.

Comment 22

[Frank Yan (:fryn)]

Attachment: patch v3

(In reply to comment #21)
> Comment on attachment 511016 [details] [diff] [review]
> patch v2
> 
> > tabmodalprompt {
> >+  font-family: sans-serif;
> 
> What exactly is this trying to achieve?

I don't know. Pasted it from Stephen's. Now removed.

> >+.topContainer {
> >+  padding: 20px 25px 20px 20px;
> 
> Is this correct for RTL?

Replaced with |padding: 20;|, since symmetry looks nice, and it used to be |.mainContainer { padding: 20px; }| anyway.

> 
> >+tabmodalprompt button {
> 
> This stylesheet can't affect anything outside of tabmodalprompt.

Addressed.

> > .mainContainer {
> >+  background-color: hsla(0,0%,100%,.95);
> 
> The 5% transparency still triggers grayscale anti-aliasing on text, which looks
> pretty bad at least on Windows.

I see that grayscale anti-aliasing :( Replaced it with white. I could also do hsl(0,0%,##%) if that looks better.

Comment 23

[Stephen Horlander [:shorlander]]

(In reply to comment #22)
> I don't know. Pasted it from Stephen's. Now removed.

It is used since we don't want to use the system font for webpage alerts.


> > >+.topContainer {
> > >+  padding: 20px 25px 20px 20px;
> > 
> > Is this correct for RTL?
> 
> Replaced with |padding: 20;|, since symmetry looks nice, and it used to be
> |.mainContainer { padding: 20px; }| anyway.

The extra 5px right padding was to account for the extra space created by the hidden icon box. Not sure if that is still an issue?

Comment 24

[Frank Yan (:fryn)]

Attachment: patch v4

Added font-family: sans-serif; back in with a comment explaining its purpose: to use the sans-serif content font, not the system UI font.

By default, these are:
On OS X 10.6: sans-serif = Helvetica; system UI font = Lucida Grande
On Windows 7: sans-serif = Arial;     system UI font = Segoe UI

(In reply to comment #21)
> The 5% transparency still triggers grayscale anti-aliasing on text, which looks
> pretty bad at least on Windows.

I thought I was seeing this, but I'm actually not. I am running Windows 7 and have DirectWrite enabled. Would having DirectWrite disabled or running Windows XP reveal the problem? (I'm fine with leaving the background-color as white to be safe; I just want to understand the problem better.)

Comment 25

[Frank Yan (:fryn)]

Attachment: patch v4

Forgot to remove unneeded selector.

Comment 26

[Dão Gottwald [:dao]]

> > The 5% transparency still triggers grayscale anti-aliasing on text, which looks
> > pretty bad at least on Windows.
> 
> I thought I was seeing this, but I'm actually not. I am running Windows 7 and
> have DirectWrite enabled. Would having DirectWrite disabled or running Windows
> XP reveal the problem? (I'm fine with leaving the background-color as white to
> be safe; I just want to understand the problem better.)

I was on Win 7 with DirectWrite and Direct2D enabled. I also verified that removing the transparency restored sub-pixel AA.

Comment 27

[Dão Gottwald [:dao]]

Attachment 503566 [details] also shows this on Windows as well as OS X. (The Windows screenshot actually shows grayscale AA across the whole UI. This shouldn't happen anymore.)

Comment 28

[Emanuel Hoogeveen [:ehoogeveen]]

With bug 622482 and bug 612846 both landed, shouldn't sub-pixel AA always work? (there was some earlier work to hoist the color of an underlying opaque surface as well, but I don't know how many cases that covers)

Comment 29

[Dão Gottwald [:dao]]

Bug 622482 fixed the general lack of sub-pixel AA on glass that I mentioned in comment 27. Your text still needs an opaque background, though. See bug 594325 comment 47.

Comment 30

[Dão Gottwald [:dao]]

> See bug 594325 comment 47.

(which has been addressed in bug 612854)

Comment 31

[Frank Yan (:fryn)]

Attachment: patch v5

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 511050 [details] [diff] [review]
patch v4

r+ on this version (not the following v.5)

Was talking with fryn about a some odd behavior with the "superwide" testcase on http://dolske.net/mozilla/tests/prompt/sizes.html, but patch v.5 didn't change it and I see there is already a problem with how things work today.

So let's land this as-is, and tweak the sizing algorithm in a followup.

Comment 33

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/644a1f424797

Filed bug 633687 for the sizing fixup.

Comment 34

[Pardal Freudenthal (:ShareBird)]

Any reason to use this spacer:
<spacer anonid="buttonSpacer" flex="1"/>
instead of positioning the buttons setting the -moz-box-pack CSS property to the .buttonContainer box?
I suppose using -moz-box-pack could automatically position the buttons also for rtl localizations?

Comment 35

[Frank Yan (:fryn)]

(In reply to comment #34)
> Any reason to use this spacer:
> <spacer anonid="buttonSpacer" flex="1"/>
> instead of positioning the buttons setting the -moz-box-pack CSS property to
> the .buttonContainer box?
> I suppose using -moz-box-pack could automatically position the buttons also for
> rtl localizations?

-moz-box-pack doesn't help when we make the other buttons visible for more complex or customized dialogs in the future.

Comment 36

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #35)
> -moz-box-pack doesn't help when we make the other buttons visible for more
> complex or customized dialogs in the future.
Oh. I see... You are meaning the case when anonid="button3" is visible. I agree.
Thanks for clarifying this and sorry for the spam.