Closed Bug 1020422 Opened 10 years ago Closed 10 years ago

Clickable area of some in-content preferences too large

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: rittme, Assigned: rittme)

Details

(Whiteboard: p=2 s=33.1 [qa!])

Attachments

(4 files, 1 obsolete file)

Attached image security checkboxes
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. 

Some of them are:
 - Options > General > Tabs preferences
 - Options > Security
 - Options > Advanced > Network and Update
This should fix all of the checkboxes and radio buttons with full box width at the in-content options.
Attachment #8435838 - Flags: review?(mconley)
Flags: needinfo?(mconley)
Thanks - reviewing now.

Just a note that you don't need to both needinfo? and review? me. needinfo? is better if there's a question to be answered. review? does the right thing for what you've been using it for. :)
Assignee: nobody → bernardo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mconley)
Ok! Sorry about that!
Comment on attachment 8435838 [details] [diff] [review]
rev1 - width of in-content preference checkbox fixed

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

This looks pretty good! Just a few nits - please see below. Thanks!

::: browser/components/preferences/in-content/advanced.xul
@@ +296,5 @@
> +                    id="allowSmartSize"
> +                    onsyncfrompreference="return gAdvancedPane.readSmartSizeEnabled();"
> +                    label="&overrideSmartCacheSize.label;"
> +                    accesskey="&overrideSmartCacheSize.accesskey;"/>
> +          <spacer/>

A spacer without a size or flex specified doesn't do anything, so this can be removed.

::: browser/components/preferences/in-content/main.xul
@@ +192,5 @@
> +      <radio id="alwaysAsk"
> +             value="false"
> +             label="&alwaysAsk.label;"
> +             accesskey="&alwaysAsk.accesskey;"/>
> +      <spacer/>

As before, this spacer can be removed.

@@ +224,5 @@
>                accesskey="&switchToNewTabs.accesskey;"
>                preference="browser.tabs.loadInBackground"/>
>  
>  #ifdef XP_WIN
> +        <checkbox id="showTabsInTaskbar" label="&showTabsInTaskbar.label;"

Hm - the indentation of this block shouldn't have changed. Can you please revert this?

::: browser/components/preferences/in-content/privacy.xul
@@ +77,5 @@
>  
>  <!-- Tracking -->
>  <groupbox id="trackingGroup" data-category="panePrivacy" hidden="true" align="start">
>    <caption><label>&tracking.label;</label></caption>
> +  <radiogroup id="doNotTrackSelection" orient="vertical" align="start"

I don't think the doNotTrackSelection radio buttons have this bug. Can you confirm?

::: browser/components/preferences/in-content/security.xul
@@ +57,5 @@
>              oncommand="gSecurityPane.showAddonExceptions();"/>
>    </hbox>
>  
>    <separator class="thin"/>
> +  <hbox>

Are you sure this outer hbox is necessary? Why not just use the vbox and set align="start" on it?

@@ +68,5 @@
> +                label="&blockWebForgeries.label;"
> +                accesskey="&blockWebForgeries.accesskey;"
> +                preference="browser.safebrowsing.enabled" />
> +    </vbox>
> +    <spacer/>

Please remove this spacer.

@@ +77,5 @@
>  <groupbox id="passwordsGroup" orient="vertical" data-category="paneSecurity" hidden="true">
>    <caption><label>&passwords.label;</label></caption>
>  
>    <hbox id="savePasswordsBox">
> +    <checkbox id="savePasswords" 

Trailing whitespace.
Attachment #8435838 - Flags: review?(mconley) → review-
Thank you, Mike. 
I fixed all of your comments. 
While I was testing it I found out I forgot to check the non-connected sync screen. It turns out the labels also had a full block width, so I added them to this revision.
Attachment #8435838 - Attachment is obsolete: true
Attachment #8435977 - Flags: review?(mconley)
Comment on attachment 8435977 [details] [diff] [review]
rev2 - width of in-content preference checkbox fixed

Looks good! Thanks Bernardo!
Attachment #8435977 - Flags: review?(mconley) → review+
Flags: firefox-backlog+
Keywords: checkin-needed
Hey Jenn! Would you mind adding this to the current iteration? Thanks!
Flags: needinfo?(jchaulk)
https://hg.mozilla.org/mozilla-central/rev/0947342dbc1b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Flags: needinfo?(jchaulk)
Whiteboard: S=IT-32C-31A-30B.3
Hey mconley!  Can you give me a size please?  And is it qa+ or qa-?  Thanks!!
Flags: needinfo?(mconley)
(In reply to Jenn Chaulk from comment #10)
> Hey mconley!  Can you give me a size please?  And is it qa+ or qa-?  Thanks!!

Sure.

If I had done this, I would have rated it a p=2.

This bug was actually fixed by a volunteer contributor (and soon to be intern), so I'm not sure if that makes this a p=0 or not. That stuff's kinda been lost on me.

This is a qa+.

STR:

Visit the following areas of in-content preferences:
 - General > Tabs preferences
 - Security
 - Advanced > Network and Update
 - Sync (when no FxA account is connected)

Notice that for the checkboxes (and some of the links) in the above sections, if you are far to the right of the checkbox label or link, you are still able to toggle the checkbox / click the link.

This patch shrinkwraps the click area to just around the checkbox and label / link area.

Example GIF (for a different area that was not fixed in this bug, but it visually explains the behaviour we're fixing):  https://bug1010402.bugzilla.mozilla.org/attachment.cgi?id=8422607
Flags: needinfo?(mconley)
Whiteboard: S=IT-32C-31A-30B.3 → p=2 s=it-32c-31a-30b.3 [qa+]
Whiteboard: p=2 s=it-32c-31a-30b.3 [qa+] → p=2 s=33.1 [qa+]
QA Contact: camelia.badau
Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 and the following mentions should be done: 
a) for the General, Content, Security, Advanced (General, Data choices, Network) tabs, the clickable area is limited to the checkbox and it's label
b) for the Privacy tab ("Use custom settings for history" selected), the clickable area is a full block in width (the clickable area is NOT limited to the checkbox and it's label) -> See screenshot "A.PNG". 
c) for the Advanced - Update, the clickable area is NOT limited to the checkbox and it's label -> See screenshot "B.PNG" 
d) for the Sync tab (when FxA account is connected), the clickable area is NOT limited to the checkbox and it's label 
 
The actual patch doesn't fixed the issues b), c) and d) mentioned above. Should I file a separate bug for these issues or reopen this one?
Flags: needinfo?(mconley)
Thanks Camelia!

Hey Bernardo - interested in taking a look at these follow-ups?
Flags: needinfo?(mconley) → needinfo?(bernardo)
Yes, I would like that!
Should I add them to the same patch or create a new one?
Flags: needinfo?(bernardo)
(In reply to Bernardo Rittmeyer [:rittme] from comment #16)
> Yes, I would like that!
> Should I add them to the same patch or create a new one?

Since this bug is already marked RESOLVED FIXED, I think we'll need a new bug for the new issues (one bug will do), and a new patch. Do you mind filing it and getting started on the patch? I'll happily review!
Sure, I'll fill a new one right away.
Thank you!
Hi Bernardo, have you logged the new bug? Or do you want me to log it?
Hi Camelia, I logged it in bug 1023339.
Thanks Bernardo!
Status: RESOLVED → VERIFIED
Whiteboard: p=2 s=33.1 [qa+] → p=2 s=33.1 [qa!]
You need to log in before you can comment on or make changes to this bug.