727997 - Sync options checkbox list displays scrollbar

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Ciaran Welch]

Attachment: Sync options Fx11

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120216 Firefox/13.0a1
Build ID: 20120216040245

Steps to reproduce:

Opened Firefox -> options.
Clicked on sync tab


Actual results:

Checkbox window below "Sync My" displays scrollbar


Expected results:

Checkbox window should have been made slightly bigger to remove scrollbar.

Comment 1

[:Margaret Leibovic]

Stephen had some good ideas for making this whole interface look nicer, but this is definitely a start. Admittedly, the styling on this box isn't very robust - the height for this box is already declared in CSS, so the easy thing to do would be to just make that larger (I don't think this was accounted for when the option to sync add-ons was added):

http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/preferences/preferences.css#185
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/preferences/preferences.css#243
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/preferences/preferences.css#175

I'm also cc'ing Jared to keep the MSU folks working on prefs in the loop.

Comment 2

[Charles Chan]

Hello, I am curious to take on this issue by have a couple of quick questions first. 

1) What should the fix be? Simply updating the (3) .css files and update the height by 10-15%?

2) It appears there are 3 themes, are these for different OS platforms? (What is pinstripe?)

3) (Related to #2) Is there an automated test to verify cross platform UI changes?

Thanks!

Comment 3

[:Margaret Leibovic]

(In reply to Charles Chan from comment #2)

> 1) What should the fix be? Simply updating the (3) .css files and update the
> height by 10-15%?

Yes, I think that's the quickest and easiest way to fix this. Since we've increased the list from 5 items to 6 items, you probably need to increase the height by about 20% :)

> 2) It appears there are 3 themes, are these for different OS platforms?
> (What is pinstripe?)

Yes, those are different OS platforms. Pinstripe is the Mac OS X theme.

> 3) (Related to #2) Is there an automated test to verify cross platform UI
> changes?

Unfortunately, there isn't an automated test for theme changes like this. However, if you upload a patch, I could help verify that it works on other platforms. Which platform are you using for development?

> Thanks!

Thanks for taking interest!

Comment 4

[Charles Chan]

Attachment: Fix sync options checkbox list

Please review the patch. 

I am running Ubuntu. After increasing the spacing by 20% (from 10em to 12em), the scrollbar is gone. (Not sure if the extra blank is okay?) :)

Glad to be able to learn more about Firefox and help at the same time!

Comment 5

[:Margaret Leibovic]

(In reply to Charles Chan from comment #4)

> I am running Ubuntu. After increasing the spacing by 20% (from 10em to
> 12em), the scrollbar is gone. (Not sure if the extra blank is okay?) :)

Did increasing it to 11em get rid of the scrollbar? Could you attach a screenshot so that we can see what it looks like with your patch?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

This is a screenshot of the patch on Windows.

When the options dialog opens, the General pane is selected by default. When switching to the Sync pane, the window gets taller, and the height doesn't change when switching back to the General pane. This doesn't look *too* bad, but it is a weird side effect of visiting the Sync pane.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to Margaret Leibovic [:margaret] from comment #5)
> (In reply to Charles Chan from comment #4)
> 
> > I am running Ubuntu. After increasing the spacing by 20% (from 10em to
> > 12em), the scrollbar is gone. (Not sure if the extra blank is okay?) :)
> 
> Did increasing it to 11em get rid of the scrollbar? Could you attach a
> screenshot so that we can see what it looks like with your patch?

If we just remove the height property it should default to "auto" and then the contents of the box will set the proper height without a scrollbar.

Comment 8

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] from comment #6)
> Created attachment 600917 [details]
> Screenshot of patch
> 
> This is a screenshot of the patch on Windows.
> 
> When the options dialog opens, the General pane is selected by default. When
> switching to the Sync pane, the window gets taller, and the height doesn't
> change when switching back to the General pane. This doesn't look *too* bad,
> but it is a weird side effect of visiting the Sync pane.

This happens currently on Windows for any pref pane that is taller than the default height (e.g. Advanced).

Comment 9

[:Margaret Leibovic]

(In reply to Stephen Horlander from comment #7)

> If we just remove the height property it should default to "auto" and then
> the contents of the box will set the proper height without a scrollbar.

Charles, can you try this out? I think this would be a better fix.

Comment 10

[Charles Chan]

Sounds good, I will try it out tonight ('auto'). 

> When the options dialog opens, the General pane is selected by default. When
> switching to the Sync pane, the window gets taller, and the height doesn't
> change when switching back to the General pane. This doesn't look *too* bad,
> but it is a weird side effect of visiting the Sync pane.

Didn't really notice it on Linux (but wasn't paying attention either), will check tonight after the fix; and will attach a screenshot as well.

Comment 11

[Charles Chan]

Attachment: Sync Option (before & after)

'auto' seems to do the do the trick, please see the before and after image on Ubuntu.

Comment 12

[Charles Chan]

Attachment: Fix sync options checkbox list-2

Here's the patch, using
  height: auto

Comment 13

[:Margaret Leibovic]

Thanks for the updated patch! Comment 7 may have been unclear, but I believe if you remove the height declaration altogether, the style will default to height: auto; for you. This means that you can just get rid of all of those #syncEnginesList style rules, and the height should work out to be the right thing. Can you try that out? Sorry for the extra cycles!

Comment 14

[Charles Chan]

Yeah, I was contemplating whether to remove the blocks. Seeing as some of the styles kept were specified with their default values (eg. 0), I determined (incorrectly) it might be better to keep the block to follow the established coding standards.

No problem, I will remove and test again.

Comment 15

[Charles Chan]

Attachment: Fix sync options checkbox list-3

One more time, with the block removed. :)

(The UI appears the same as 'after', so I won't bother with another screenshot.)

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/43766cfb6d30

Comment 17

[Matt Brubeck (:mbrubeck)]

Landed in mozilla-central for Firefox 13.  The fix should be included in tomorrow's Nightly build.  Thanks!
https://hg.mozilla.org/mozilla-central/rev/43766cfb6d30