Closed
Bug 1023339
Opened 10 years ago
Closed 10 years ago
Clickable area of some in-content preferences (still) too large
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 33
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
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•10 years ago
|
status-firefox32:
--- → affected
status-firefox33:
--- → affected
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bernardo
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
(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)
Assignee | ||
Comment 3•10 years ago
|
||
I'll check it out. Thank you!
Assignee | ||
Comment 4•10 years ago
|
||
This should cover all that was left. Finally :)
Attachment #8449017 -
Flags: review?(mconley)
Comment 5•10 years ago
|
||
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-
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
This should be better.
Attachment #8449646 -
Attachment is obsolete: true
Attachment #8449670 -
Flags: review?(mconley)
Comment 10•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•10 years ago
|
||
Added a commit message.
Attachment #8449670 -
Attachment is obsolete: true
Attachment #8449705 -
Flags: review+
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ba704d0b0b09
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 13•10 years ago
|
||
(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 14•10 years ago
|
||
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.)
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Here you go. Thank you!
Attachment #8450352 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/870b1d68f372 Better patch: https://hg.mozilla.org/integration/fx-team/rev/2ca0c30b7e07
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba704d0b0b09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•