665342 - In the sync setup window, delete the border between upper and lower sections, and change background color of lower area

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Michael Monreal [:micmon]]

Attachment: Screenshot

The bottom of the sync setup window is blue (-> Screenshot), it should be gray (=bgcolor) like the upper region.

Comment 1

[Erin W. McCall]

Attachment: Suggested patch for bug

This is my first patch attempt and I would greatly appreciate any & all feedback!

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 604812 [details] [diff] [review]
Suggested patch for bug

> .wizard-buttons {
>-  border-top: 2px solid #ccd9ea;
>-  background-color: #f1f5fb;
>+  border-top: 2px solid #990099;

That's quite an ugly color, I don't think we can use that. ;-)

Also, looks like the button should be 1px thick or removed entirely.

>+  background-color: Window;

How about something like rgba(0,0,0,0.1)?

Comment 3

[Erin W. McCall]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 604812 [details] [diff] [review]
> Suggested patch for bug
> 
> > .wizard-buttons {
> >-  border-top: 2px solid #ccd9ea;
> >-  background-color: #f1f5fb;
> >+  border-top: 2px solid #990099;
> 
> That's quite an ugly color, I don't think we can use that. ;-)

Oh my. How embarrassing. I was just playing around making sure I was changing the correct thing and meant to change that back to original color. Will fix and resubmit patch!

> Also, looks like the button should be 1px thick or removed entirely.
> 
> >+  background-color: Window;
> 
> How about something like rgba(0,0,0,0.1)?

Not quite sure what you mean here about the button looking like it should be 1px thick or removed...

Comment 4

[Dão Gottwald [:dao]]

I meant "border" despite writing "button"...

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #2)
> How about something like rgba(0,0,0,0.1)?

That is probably the right color for Mac and Linux. The blue for Windows was intentional I think.

(In reply to Dão Gottwald [:dao] from comment #4)
> I meant "border" despite writing "button"...

Yeah I believe the original design was for a 1px border and a slight 1px inset shadow. Not sure how we ended up with a double border :)

Comment 6

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > How about something like rgba(0,0,0,0.1)?
> 
> That is probably the right color for Mac and Linux. The blue for Windows was
> intentional I think.

To some extent, yes; it doesn't make sense for all Windows versions and flavours and the border is messed up, as you mentioned. So I'd support just ripping it out across the board unless someone wants to do it the right way.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Stephen Horlander from comment #5)
> > (In reply to Dão Gottwald [:dao] from comment #2)
> > > How about something like rgba(0,0,0,0.1)?
> > 
> > That is probably the right color for Mac and Linux. The blue for Windows was
> > intentional I think.
> 
> To some extent, yes; it doesn't make sense for all Windows versions and
> flavours and the border is messed up, as you mentioned. So I'd support just
> ripping it out across the board unless someone wants to do it the right way.

That works for me.

Comment 8

[Erin W. McCall]

Attachment: Removed border and changed lower area color to suggested value

I removed the border completely and changed the lower area background color to the suggested rgba(0,0,0,0.1). I don't think this is the "final answer" though..... still looks kind of ugly, just not quite as bad. Any other suggestions to improve the look of this thing? Maybe changing the color to match the upper region and add a super thin plain ol' black border?

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 605797 [details] [diff] [review]
Removed border and changed lower area color to suggested value

This patch applies on top of the previous patch. Instead, we need a patch that applies on mozilla-central tip.

Otherwise this seems fine, further improvements can happen in new bugs.

Comment 10

[Erin W. McCall]

(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 605797 [details] [diff] [review]
> Removed border and changed lower area color to suggested value
> 
> This patch applies on top of the previous patch. Instead, we need a patch
> that applies on mozilla-central tip.

Apologies again. I don't understand what you mean by "applies on top of the previous patch" & "applies on mozilla-central tip"?

Comment 11

[Dão Gottwald [:dao]]

The patch tries to remove this line, for instance:

  border-top: 2px solid #990099;

Such a line doesn't exist on mozilla-central, see <http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/syncSetup.css>.

Comment 12

[Erin W. McCall]

(In reply to Dão Gottwald [:dao] from comment #11)
> The patch tries to remove this line, for instance:
> 
>   border-top: 2px solid #990099;
> 
> Such a line doesn't exist on mozilla-central, see
> <http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/
> syncSetup.css>.

Ohhhhhhhhhh. Got it. Ok. Will fix.

Comment 13

[Erin W. McCall]

Attachment: Updated changes to reflect mozilla-central contents.

This patch reflects what is in mozilla-central instead of the previous changes to my working copy.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 605831 [details] [diff] [review]
Updated changes to reflect mozilla-central contents.

Thanks!

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f9ceb4f6eae3

Comment 16

[Dão Gottwald [:dao]]

Landed a second time with the correct bug number in the commit message.

http://hg.mozilla.org/integration/mozilla-inbound/rev/e36239fc97af

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e36239fc97af