Closed Bug 727997 Opened 11 years ago Closed 11 years ago

Sync options checkbox list displays scrollbar

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: bubonicfred, Assigned: charles.wh.chan)

Details

(Whiteboard: [good first bug][mentor=margaret][lang=css])

Attachments

(4 files, 2 obsolete files)

Attached image 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.
Component: Untriaged → Preferences
QA Contact: untriaged → preferences
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [good first bug][mentor=margaret][lang=css]
Version: 11 Branch → Trunk
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!
(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!
Attached patch Fix sync options checkbox list (obsolete) — Splinter Review
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!
Attachment #600847 - Flags: review?
(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?
Attached image 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.
(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.
(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).
(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.
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.
'auto' seems to do the do the trick, please see the before and after image on Ubuntu.
Attached patch Fix sync options checkbox list-2 (obsolete) — Splinter Review
Here's the patch, using
  height: auto
Attachment #600847 - Attachment is obsolete: true
Attachment #600847 - Flags: review?
Attachment #601166 - Flags: review?
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!
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.
One more time, with the block removed. :)

(The UI appears the same as 'after', so I won't bother with another screenshot.)
Attachment #601185 - Flags: review?
Attachment #601166 - Attachment is obsolete: true
Attachment #601166 - Flags: review?
Attachment #601185 - Flags: review? → review+
Assignee: nobody → charles.wh.chan
Keywords: checkin-needed
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.