Improve visual design for tab-modal buttons & dialog

RESOLVED FIXED in mozilla2.0b12

Status

()

Toolkit
Themes
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Dolske, Assigned: fryn)

Tracking

(Depends on: 1 bug)

unspecified
mozilla2.0b12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Component: General → Themes
QA Contact: general → themes

Comment 1

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

Updated

7 years ago
Assignee: dolske → margaret.leibovic

Comment 2

7 years ago
Created attachment 494767 [details] [diff] [review]
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.
Attachment #494767 - Flags: review?(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.
Attachment #494767 - Flags: review?(dao) → review-
(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.

Updated

7 years ago
Severity: normal → enhancement
OS: All → Mac OS X
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)
Summary: Visual design for tab-modal prompt buttons → Improve visual design for tab-modal buttons & dialog
(Assignee)

Comment 6

7 years ago
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.
Depends on: 613749
Created attachment 503565 [details] [diff] [review]
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
Created attachment 503566 [details]
Screenshot of changes
Attachment #503565 - Flags: feedback?(limi)
Attachment #503565 - Flags: feedback?(dolske)

Updated

7 years ago
Assignee: margaret.leibovic → shorlander
(Reporter)

Updated

7 years ago
Attachment #494767 - Attachment is obsolete: true
(Reporter)

Comment 9

7 years ago
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.
Attachment #503565 - Flags: feedback?(dolske) → feedback-
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.
Attachment #503565 - Flags: feedback?(limi) → feedback+

Updated

7 years ago
Status: NEW → ASSIGNED

Comment 11

7 years ago
Mac buttons need more top/bottom padding, IMO. Looks odd as it is.
(Assignee)

Comment 12

6 years ago
(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. :)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 620614
(Reporter)

Comment 14

6 years ago
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.
Attachment #503565 - Flags: feedback- → feedback+
(Reporter)

Comment 15

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

Comment 16

6 years ago
Since we are now doing all platforms in this bug, changing platform to All (in hopes of avoiding bug 620614-esque dupes).
OS: Mac OS X → All
(Assignee)

Comment 17

6 years ago
(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.
(Assignee)

Comment 18

6 years ago
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.
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
Created attachment 511016 [details] [diff] [review]
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.
Attachment #503565 - Attachment is obsolete: true
Attachment #511016 - Flags: review?(dolske)
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.
(Assignee)

Comment 22

6 years ago
Created attachment 511031 [details] [diff] [review]
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.
Attachment #511016 - Attachment is obsolete: true
Attachment #511031 - Flags: review?(dolske)
Attachment #511031 - Flags: review?(dao)
Attachment #511016 - Flags: review?(dolske)
(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?
Assignee: shorlander → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

6 years ago
Attachment #511031 - Flags: review?(dolske)
Attachment #511031 - Flags: review?(dao)
(Assignee)

Comment 24

6 years ago
Created attachment 511046 [details] [diff] [review]
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.)
Assignee: nobody → fryn
Attachment #511031 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #511046 - Flags: review?(dolske)
Attachment #511046 - Flags: review?(dao)
(Assignee)

Comment 25

6 years ago
Created attachment 511050 [details] [diff] [review]
patch v4

Forgot to remove unneeded selector.
Attachment #511046 - Attachment is obsolete: true
Attachment #511050 - Flags: review?(dolske)
Attachment #511050 - Flags: review?(dao)
Attachment #511046 - Flags: review?(dolske)
Attachment #511046 - Flags: review?(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.
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.)
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)
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.
> See bug 594325 comment 47.

(which has been addressed in bug 612854)
(Assignee)

Comment 31

6 years ago
Created attachment 511907 [details] [diff] [review]
patch v5
Attachment #511050 - Attachment is obsolete: true
Attachment #511907 - Flags: review?(dolske)
Attachment #511050 - Flags: review?(dolske)
Attachment #511050 - Flags: review?(dao)
(Reporter)

Updated

6 years ago
Attachment #511907 - Attachment is obsolete: true
Attachment #511907 - Flags: review?(dolske)
(Reporter)

Comment 32

6 years ago
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.
Attachment #511050 - Attachment is obsolete: false
Attachment #511050 - Flags: review+
(Reporter)

Updated

6 years ago
Attachment #511050 - Flags: approval2.0+
(Reporter)

Comment 33

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

Filed bug 633687 for the sizing fixup.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
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?
(Assignee)

Comment 35

6 years ago
(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.
(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.
Depends on: 635910
Depends on: 643909
(Assignee)

Updated

5 years ago
Depends on: 781493
You need to log in before you can comment on or make changes to this bug.