Closed Bug 1023339 Opened 5 years ago Closed 5 years ago

Clickable area of some in-content preferences (still) too large

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox32 --- affected
firefox33 --- fixed

People

(Reporter: rittme, Assigned: rittme)

Details

Attachments

(1 file, 5 obsolete files)

The hit area of some in-content preferences is a full block in width but should be limited to just the checkbox and it's label. 

Bug 1020422 took care of most of it, but there are still some that need fixing:

- for the Privacy tab ("Use custom settings for history" selected), the clickable area is a full block in width

- for the Advanced - Update, the clickable area is NOT limited to the checkbox and it's label

- for the Sync tab (when FxA account is connected), the clickable area is NOT limited to the checkbox and it's label
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → bernardo
Mike, now that I have been granted editbug permissions, if I want to work on a bug can I just assign it to myself?
Flags: needinfo?(mconley)
(In reply to Bernardo Rittmeyer [:rittme] from comment #1)
> Mike, now that I have been granted editbug permissions, if I want to work on
> a bug can I just assign it to myself?

Totally! And congrats on getting editbugs. :) You should also think about getting Level 1 Commit Access (http://www.mozilla.org/hacking/committer/). Let me know if you need any help with that (I can vouch for you, for example).
Flags: needinfo?(mconley)
I'll check it out. Thank you!
This should cover all that was left. Finally :)
Attachment #8449017 - Flags: review?(mconley)
Comment on attachment 8449017 [details] [diff] [review]
rev1 - width of in-content preference elements fixed

This is a good start - but I'm noticing that we're crunching in the buttons for the privacy settings now:

http://i.imgur.com/p5vuoC0.png

Are you sure that vbox surrounding the privacy settings is required?
Attachment #8449017 - Flags: review?(mconley) → review-
Sorry about that. 
Don't know how I haven't seen this.

I used that vbox because I did not find another way to align the buttons to the right without it.
Without the extra vbox their parent alignment is "start" and I can't get to align them.

I'll try to figure out another way to do it.
So I used hboxes instead and it worked nicelly. 
But I'm not sure it's a good practice.
Attachment #8449017 - Attachment is obsolete: true
Attachment #8449646 - Flags: review?(mconley)
Comment on attachment 8449646 [details] [diff] [review]
rev2 - width of in-content preference checkbox fixed (using hbox)

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

I think this is fine - just fix the alignment problems, upload a new patch, and let's get this landed. Thanks!

::: browser/components/preferences/in-content/privacy.xul
@@ +164,5 @@
> +            <checkbox id="rememberHistory"
> +                      label="&rememberHistory2.label;"
> +                      accesskey="&rememberHistory2.accesskey;"
> +                      preference="places.history.enabled"/>
> +              <spacer flex="1"/>

Nit - alignment

@@ +172,5 @@
> +                      label="&rememberSearchForm.label;"
> +                      accesskey="&rememberSearchForm.accesskey;"
> +                      preference="browser.formfill.enable"/>
> +            <spacer flex="1"/>
> +        </hbox>

Nit - alignment
Attachment #8449646 - Flags: review?(mconley) → review+
This should be better.
Attachment #8449646 - Attachment is obsolete: true
Attachment #8449670 - Flags: review?(mconley)
Comment on attachment 8449670 [details] [diff] [review]
rev3 - width of in-content preference checkbox fixed (using hbox)

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

Yeah, looks good. Thanks rittme!
Attachment #8449670 - Flags: review?(mconley) → review+
Added a commit message.
Attachment #8449670 - Attachment is obsolete: true
Attachment #8449705 - Flags: review+
(In reply to Bernardo Rittmeyer from comment #7)
> I used hboxes instead and it worked nicelly. 
> But I'm not sure it's a good practice.

No, it's not, unless you want to have extra elements in the same row.

1. <hbox><checkbox/></hbox>
+ Checkbox only consumes the minimum necessary width
- Checkbox cannot wrap on to the next line (potential l10n issue)
2. <hbox><checkbox flex="1"/></hbox>
+ Checkbox will wrap if necessary
- Checkbox consumes the full width
3. <vbox><checkbox/></vbox>
+ Checkbox will wrap if necessary
- Checkbox consumes the full width
4. <vbox align="start"><checkbox/></vbox>
+ Checkbox only consumes the minimum necessary with
+ Checkbox will wrap if necessary
Comment on attachment 8449705 [details] [diff] [review]
rev 4 - with commit message, r=mconey

>       <vbox class="indent">
>+        <hbox>
>+          <checkbox id="privateBrowsingAutoStart"
>+                    label="&privateBrowsingPermanent2.label;"
>+                    accesskey="&privateBrowsingPermanent2.accesskey;"
>+                    preference="browser.privatebrowsing.autostart"
>+                    oncommand="gPrivacyPane.updateAutostart()"/>
>+          <spacer flex="1"/>
>+        </hbox>
>         <vbox class="indent">
>+          <hbox>
>+            <checkbox id="rememberHistory"
>+                      label="&rememberHistory2.label;"
>+                      accesskey="&rememberHistory2.accesskey;"
>+                      preference="places.history.enabled"/>
>+            <spacer flex="1"/>
>+          </hbox>
>+          <hbox>
>+            <checkbox id="rememberForms"
>+                      label="&rememberSearchForm.label;"
>+                      accesskey="&rememberSearchForm.accesskey;"
>+                      preference="browser.formfill.enable"/>
>+            <spacer flex="1"/>
>+          </hbox>
So here I would suggest something like this:
<vbox class="indent">
  <vbox align="start">
    <checkbox id="privateBrowsingAutoStart" ... />
  </vbox>
  <vbox class="indent">
    <vbox align="start">
      <checkbox id="rememberHistory" ... />
      <checkbox id="rememberForms" ... />
    </vbox>

(Also the spacers were unnecessary as there wasn't anything on the other side to space away from the checkbox, unlike in the subsequent cases where there was a right-aligned button.)
Hm, I guess we both learned something today, rittme. Sorry if I led you down the wrong path.

Can you try Neil's suggestion to see if it works? If so, we can back-out this patch and land a new one.
Flags: needinfo?(bernardo)
No problem, Mike.
I tried out Neil's suggestion and it worked nicely.
Attachment #8449705 - Attachment is obsolete: true
Attachment #8450352 - Flags: review?(mconley)
Flags: needinfo?(bernardo)
Comment on attachment 8450352 [details] [diff] [review]
rev 5 - using vboxes to adjust checkboxes width at privacy preferences

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

Thanks rittme! r=me with the trailing space removed. Once you upload the new patch, I'll take care of the backout and landing.

::: browser/components/preferences/in-content/privacy.xul
@@ +216,5 @@
>                      preference="pref.privacy.disable_button.view_cookies"/>
>            </hbox>
>            <hbox id="clearDataBox"
>                  align="center">
> +            <checkbox id="alwaysClear" 

Nit - trailing space.
Attachment #8450352 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/ba704d0b0b09
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.