Closed Bug 1130741 Opened 5 years ago Closed 5 years ago

Sync pane "Terms of Service" & "Privacy notice" links are too small

Categories

(Firefox :: Theme, defect, P4)

defect

Tracking

()

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- verified

People

(Reporter: ntim, Assigned: lemcgregor3, Mentored)

References

Details

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

Attachments

(1 file, 1 obsolete file)

Screenshot : http://i.imgur.com/2RUP3p1.png
This isn't so much of a regression from the windowed preferences, but it would still be nice to fix since in-content preferences are new. However, we won't block shipping for this.
Blocks: 718011
No longer blocks: ship-incontent-prefs
Was this fixed by the work in bug 1136670?
Flags: needinfo?(ntim007)
The alignment is fixed, but not the small font size.
Flags: needinfo?(ntim007)
Summary: Sync pane "Terms of Service" & "Privacy notice" links are too small and aren't aligned with the help button → Sync pane "Terms of Service" & "Privacy notice" links are too small
To fix this bug :
In [0], remove the small class from the links. There also needs to be more margin on top and bottom of these links, but I think it's better to handle that later, since this is a bit tricky on windows.


[0] : http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul#313
Mentor: ntim007
Whiteboard: [good first bug][lang=css]
I would like to take this bug. I'm still new to working with the source code.
(In reply to Léon McGregor from comment #5)
> I would like to take this bug. I'm still new to working with the source code.
Hi Léon,
Thanks for your interest in this bug.

You can take a look at http://codefirefox.com , it provides great tutorials. Or if you prefer reading written docs : https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

If you need help, you can reach #introduction on IRC (irc.mozilla.org).
(In reply to Tim Nguyen [:ntim] from comment #4)
> To fix this bug :
> In [0], remove the small class from the links. There also needs to be more
> margin on top and bottom of these links, but I think it's better to handle
> that later, since this is a bit tricky on windows.
> 
> 
> [0] :
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> in-content/sync.xul#313

You mentioned changing the margins. Do you think this should be in a separate bug, or should I attempt to fix them at this point as well?
This removes the "small" classes from the relevant links
Attachment #8573505 - Flags: review?(ntim007)
Comment on attachment 8573505 [details] [diff] [review]
bug1130741_correctTOSTextSize.diff

Review of attachment 8573505 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me ! But my review is worth nothing.
Attachment #8573505 - Flags: review?(ntim007)
Attachment #8573505 - Flags: review?(jaws)
Attachment #8573505 - Flags: feedback+
Comment on attachment 8573505 [details] [diff] [review]
bug1130741_correctTOSTextSize.diff

Review of attachment 8573505 [details] [diff] [review]:
-----------------------------------------------------------------

Please update /browser/themes/preferences/*/preferences.css to have a comment at the lines for `label.small` with "These lines should be removed as part of fixing bug 1140495".

With the above changes then it should be ready for r+
Attachment #8573505 - Flags: review?(jaws) → feedback+
Includes commenting in preferences.css for bug 1140495
Attachment #8573505 - Attachment is obsolete: true
Attachment #8574106 - Flags: review?(jaws)
Comment on attachment 8574106 [details] [diff] [review]
bug1130741_correctTOSTextSize.diff - with comments for 1140495

Review of attachment 8574106 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8574106 - Flags: review?(jaws) → review+
What steps should I take now? This is the first patch I've made, so I don't have any commit access rights.
Flags: needinfo?(jaws)
Nothing more is needed on your behalf for this bug. Congrats on your first bug :)

I've assigned bug 1115925 for you to work on next. I'm looking forward to your continued contributions :)
Assignee: nobody → lemcgregor3
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/784963e85352
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/784963e85352
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 39
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 39.0a1 (buildID: 20150317030206).
Status: RESOLVED → VERIFIED
Comment on attachment 8574106 [details] [diff] [review]
bug1130741_correctTOSTextSize.diff - with comments for 1140495

(requesting approval per discussion with Jared in our meeting)

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: tiny (hard to read) sync links at the bottom of the in-content-prefs sync pane
[Describe test coverage new/current, TreeHerder]: nope, css only
[Risks and why]: very small, removing attributes that trigger CSS that makes them so tiny
[String/UUID change made/needed]: nope
Attachment #8574106 - Flags: approval-mozilla-aurora?
Attachment #8574106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.