Sync options checkbox list displays scrollbar

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Preferences
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ciaran Welch, Assigned: Charles Chan)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 597986 [details]
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.

Updated

6 years ago
Component: Untriaged → Preferences
QA Contact: untriaged → preferences

Comment 1

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

Comment 2

6 years ago
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

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

Comment 4

6 years ago
Created attachment 600847 [details] [diff] [review]
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!
Attachment #600847 - Flags: review?

Comment 5

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

Comment 9

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

Comment 10

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

Comment 11

6 years ago
Created attachment 601164 [details]
Sync Option (before & after)

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

Comment 12

6 years ago
Created attachment 601166 [details] [diff] [review]
Fix sync options checkbox list-2

Here's the patch, using
  height: auto
Attachment #600847 - Attachment is obsolete: true
Attachment #600847 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #601166 - Flags: review?

Comment 13

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

Comment 14

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

Comment 15

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

Updated

6 years ago
Attachment #601166 - Attachment is obsolete: true
Attachment #601166 - Flags: review?

Updated

6 years ago
Attachment #601185 - Flags: review? → review+

Updated

6 years ago
Assignee: nobody → charles.wh.chan
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/43766cfb6d30
Keywords: checkin-needed
Target Milestone: --- → Firefox 13
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.