Please find the updated specs below: http://people.mozilla.org/~mmaslaney/doorhanger-specs.png Sketch files: https://www.dropbox.com/s/n5czi3zrti10777/Firefox_Onboarding_v9.zip

Didn't we just change the style in bug 958673? Why are we updating it again?

Justin, we're refining the designs to better display the updated copy and buttons. The goal is to make the doorhanger feel more consistent with our current UI.

I should have a patch today.

This implements the majority of what's in the spec but there are probably some dimensions off by a pixel or two. I'd like to handle any non-trivial pixel polish in a follow-up that bwinton volunteered to work on.

Michael, do you agree this isn't worse than before this patch?

I'll try to get to this review tomorrow if Blair doesn't.

Looks like some of the alignment of elements is still off. For instance, "not now" should be left aligned with the copy block in the top. See: http://cl.ly/image/0r2t2J0F410p I'll let Michael explain any further style and alignment details.

(In reply to Holly Habstritt [:Habber] from comment #9) > Looks like some of the alignment of elements is still off. For instance, > "not now" should be left aligned with the copy block in the top. See: > http://cl.ly/image/0r2t2J0F410p > > I'll let Michael explain any further style and alignment details. I think that was a coincidence in the spec as it doesn't scale to more than two buttons. You can see in http://grab.by/vabQ that it doesn't line up and that the dimensions are measuring offsets from the right so my understanding was that the buttons were right-aligned.

(In reply to Holly Habstritt [:Habber] from comment #9) > Looks like some of the alignment of elements is still off. For instance, > "not now" should be left aligned with the copy block in the top. See: > http://cl.ly/image/0r2t2J0F410p I think we need to avoid this. Once we localize, that extra space is is going to be *very* important for some locales.

Comment on attachment 8390916 [details] [diff] [review] v.1 New style with primary button colour and footer background Review of attachment 8390916 [details] [diff] [review]: ----------------------------------------------------------------- Just a couple of small fixups. And I agree any additional pixel polishing is best left to followups. ::: browser/base/content/browser.xul @@ +205,5 @@ > role="alert"> > + <vbox> > + <hbox pack="end"> > + <toolbarbutton id="UITourTooltipClose" class="close-icon" > + tooltiptext="&uiTour.infoPanel.close;"/> I think I'd consider this an accessibility regression for those using screenreaders - since they'd get the close button first, without getting any context about the panel. Just adding a tabindex attribute so it isn't first should fix that. ::: browser/themes/linux/browser.css @@ -2070,5 @@ > } > > #UITourTooltipClose { > - -moz-appearance: toolbarbutton; > - list-style-image: url("moz-icon://stock/gtk-close?size=menu"); Just double checked in a VM - you do in fact need this. The close-icon class isn't defined anywhere for Linux (helpful, eh?).

Thanks Blair! (In reply to Blair McBride [:Unfocused] from comment #12) > Review of attachment 8390916 [details] [diff] [review]: > > ::: browser/base/content/browser.xul > @@ +205,5 @@ > > role="alert"> > > + <vbox> > > + <hbox pack="end"> > > + <toolbarbutton id="UITourTooltipClose" class="close-icon" > > + tooltiptext="&uiTour.infoPanel.close;"/> > > I think I'd consider this an accessibility regression for those using > screenreaders - since they'd get the close button first, without getting any > context about the panel. Just adding a tabindex attribute so it isn't first > should fix that. OK, what value should I set it to? I've not dealt with this specific case where we want to be able to tab to an item but don't want it first. > ::: browser/themes/linux/browser.css > @@ -2070,5 @@ > > } > > > > #UITourTooltipClose { > > - -moz-appearance: toolbarbutton; > > - list-style-image: url("moz-icon://stock/gtk-close?size=menu"); > > Just double checked in a VM - you do in fact need this. The close-icon class > isn't defined anywhere for Linux (helpful, eh?). Yeah, I knew that, sorry for not saying so. Bug 879921 should be done any day now (hopefully tomorrow by me) and I didn't want to have to handle both sizes of close buttons so I just depended on that bug. I believe without bug 879921 there was no close button present which I thought may be fine until bug 879921 lands as it's what we had just a few days ago before bug 935823 landed. Yes, I'm trying to cut corners to get this in the first beta. Thoughts?

Agreed about needing room for translation. I see what Matt means as well. Looks like the margins + doorhanger width just worked out that way for the 'not now' to appear left aligned in the design mock-up. For mac, it looks like the 30px right margin is reflected in the attachment. For Windows, however, the margin between the button and right side of the doorhanger is what is most noticeable. Compare this - http://cl.ly/image/1N0O0H3m0M2T to Michael's spec - http://cl.ly/image/0r2t2J0F410p. Fixing that right margin for Windows should make the buttons better aligned with the copy above them. It will also give the 'x' more spacing in the right column. We can work with Michael to make any alignment adjustments after Beta. Michael, can you clarify if you also wanted the 30px margin between 'not now' and the green button? The new doorhanger style is a huge improvement! (In reply to Blair McBride [:Unfocused] from comment #11) > (In reply to Holly Habstritt [:Habber] from comment #9) > > Looks like some of the alignment of elements is still off. For instance, > > "not now" should be left aligned with the copy block in the top. See: > > http://cl.ly/image/0r2t2J0F410p > > I think we need to avoid this. Once we localize, that extra space is is > going to be *very* important for some locales.

For comment 13 (especially the tabindex part).

(In reply to Matthew N. [:MattN] (away March 10 - 11) from comment #15) > For comment 13 (especially the tabindex part). This is non trivial because items with a tabindex attribute get navigated first. You'd need to add a tabindex attribute to all the buttons. See: http://www.w3.org/TR/html401/interact/forms.html#h-17.11.1

Since the panel has noautofocus="true", how does one even tab between the contents?

I see it still seems to get auto-focused on Windows but I then discovered that the close button isn't currently tabbable anyways (lacking the "tabbable" class) so I don't consider this a regression and filed a follow-up to make it tabbable (bug 984042). I just landed this and I'll finish bug 879921 today.

Comment on attachment 8390916 [details] [diff] [review] v.1 New style with primary button colour and footer background Multiple parties would like this in the first beta 29 since we will be doing A/B testing and don't want design changes to skew that experiment. [Approval Request Comment] Bug caused by (feature/regressing bug #): new UITour info panel design User impact if declined: The primary button will be less compelling and possibly lead to less users taking the tour. Testing completed (on m-c, etc.): Locally by MattN and Unfocused. Risk to taking this patch (and alternatives if risky): Low risk styling change primarily String or IDL/UUID changes made by this patch: None

Why tracking-firefox29-? This was nominated for tracking because it was wanted for the first beta 29 (see comment 20). Now that it is fixed, removing the flag seems better than minusing it if you don't want to track it. Since this is the first thing beta users will see I think it makes sense to track it so it can get more attention by QA and other parties.

Comment on attachment 8390923 [details] Screen Shot of v.1 Marked-up image posted in the bug: • Please double check padding • The Firefox icon should be hi-res (might be my screen) • Green primary button should not have a gray rule, or the rule should be the same green. • Green Primary button height is off

We should also align the buttons with the content, as marked.

Verified as fixed using the following environment: FF Beta 29.0b4 Build Id:20140331125246 OS: Win 7 x64, Ubuntu 13.04 x 64, Mac Os 10.8.5

Verified as fixed on Firefox 30 beta 7 (buildID 20140522105902) under Win 7 64-bit, Ubuntu 32-bit and Mac OS 10.9.2.