Closed Bug 1118888 Opened 5 years ago Closed 5 years ago

Tracking Protection preferences UI uses a lot of blank space

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: jaws, Assigned: Paenglab, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(3 files, 2 obsolete files)

The Tracking Protection UI that was introduced in bug 1031033 has a lot of blank space around it and within it.

We should remove the <separator>s and an odd <spacer> that is in the Do Not Track section. While cleaning this up, the markup of the "Prevent sites from tracking me" and the "Tell sites that I do not want to be tracked" is quite different and can be made the same.

These changes should be made in both:
/browser/components/preferences/in-content/privacy.xul
/browser/components/preferences/privacy.xul
This is a screenshot of what we could make it look like if we remove the separators, the spacer, and indent the labels by 32px. 32px is calculated because the checkbox is 21px wide, has 1px of border-end-width, and 10px of margin-end.
That was my fault about the separators. That patch was originally written when DNT had a 3-state UI and someone asked for more separation between the 2.
Attached patch Bug1118888.patch (obsolete) — Splinter Review
I had to put a hbox around the text-link labels because the labels have !important margins. Directly applying the indent class to the labels would need to add the !important to .indent in CSS.

Is it okay like this or should I go with the change in CSS?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8546103 - Flags: review?(jaws)
Comment on attachment 8546103 [details] [diff] [review]
Bug1118888.patch

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

This change looks good. Can you please make the same change to the non-in-content preferences privacy.xul file too?
Attachment #8546103 - Flags: review?(jaws)
Attached patch Bug1118888.patch (obsolete) — Splinter Review
Non-in-content preferences privacy.xul added.
Attachment #8546103 - Attachment is obsolete: true
Attachment #8546164 - Flags: review?(jaws)
Comment on attachment 8546164 [details] [diff] [review]
Bug1118888.patch

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

r+ with the following change made. Thanks for picking this up and getting a fix turned around so quickly :)

::: browser/components/preferences/in-content/privacy.xul
@@ +106,5 @@
> +          class="indent">
> +      <label id="doNotTrackInfo"
> +             class="text-link"
> +             href="https://www.mozilla.org/dnt">
> +             &doNotTrackInfo.label;

Sorry, I should have said this in the previous review. This entity should only be indented two spaces from the start of the <label> tag, like it was before this patch. Otherwise right now it looks like it is an attribute on the element.

This should also be changed in the other file too.
Attachment #8546164 - Flags: review?(jaws) → review+
Attached patch Bug1118888.patchSplinter Review
Patch for check-in.
Attachment #8546164 - Attachment is obsolete: true
Attachment #8546180 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/76b4e802be97
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/76b4e802be97
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 37
Flags: qe-verify+
I'm not sure it makes sense to verify this now. It's already 2 months since it was fixed and is due to change again shortly.
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #10)
> I'm not sure it makes sense to verify this now. It's already 2 months since
> it was fixed and is due to change again shortly.

Thanks for the info Monica! Will mark it as qe-verify- then.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.