Closed Bug 1442542 Opened 2 years ago Closed 2 years ago

The options from Clear Data dialogue aren't selected and highlighted when navigating using Tab key

Categories

(Firefox :: Preferences, defect, P1)

60 Branch
x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: roxana.leitan, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v2])

Attachments

(4 files)

Attached image Untitled.png
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180301100139

[Affected versions]:
Nightly 60.0a1

[Affected platforms]:
Windows 7 x86, Windows 10 x64

[Steps to reproduce]:
1.Launch Nightly 60.0a1 with a new profile
2.Go to about:preferences#privacy and click "Clear Data" button from Cookies and Dite Data section
3.Press Tab key to navigate through Clear Data options ("Cookies and Site Data" and "Cached Web Content")

[Expected result]:
The options should be selected and highlighted

[Actual result]:
The options aren't selected and highlighted but a comma appear next to option checkbox
Whiteboard: [storage-v2][triage]
Priority: -- → P2
Whiteboard: [storage-v2][triage] → [storage-v2][
Whiteboard: [storage-v2][ → [storage-v2]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8968534 [details]
Bug 1442542 - Make checkboxes in the clear site data dialog use a label attribute.

https://reviewboard.mozilla.org/r/237220/#review243298

::: browser/themes/windows/preferences/preferences.css:82
(Diff revision 1)
>  }
> +
> +/* Clear Site Data Dialog */
> +
> +/* We're not using regular checkbox labels (because we want to
> + * have a custom hint below the label), which is messing up

This also breaks clicking the label to toggle the checkbox. Why are we center-aligning the checkbox with both labels rather than only with the primary one?

::: browser/themes/windows/preferences/preferences.css:83
(Diff revision 1)
> +
> +/* Clear Site Data Dialog */
> +
> +/* We're not using regular checkbox labels (because we want to
> + * have a custom hint below the label), which is messing up
> + * focus rings on Windows for some reason, so we'll just draw our own. */

The reason is that we draw the focus ring around .checkbox-label-box, as you already seem to have realized.

It's a problem on Linux too.

::: browser/themes/windows/preferences/preferences.css:84
(Diff revision 1)
> +/* Clear Site Data Dialog */
> +
> +/* We're not using regular checkbox labels (because we want to
> + * have a custom hint below the label), which is messing up
> + * focus rings on Windows for some reason, so we'll just draw our own. */
> +#ClearSiteDataDialog checkbox:focus > .checkbox-check {

Should be :-moz-focusring (except that I don't think this is the right solution in the first place)
Attachment #8968534 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #2)
> Comment on attachment 8968534 [details]
> Bug 1442542 - Fix focusring for checkboxes in the clear site data dialog.
> 
> https://reviewboard.mozilla.org/r/237220/#review243298
> 
> ::: browser/themes/windows/preferences/preferences.css:82
> (Diff revision 1)
> >  }
> > +
> > +/* Clear Site Data Dialog */
> > +
> > +/* We're not using regular checkbox labels (because we want to
> > + * have a custom hint below the label), which is messing up
> 
> This also breaks clicking the label to toggle the checkbox.

You're right, we should file a bug about that, I'm pretty sure the control attribute should take care of it...

> Why are we
> center-aligning the checkbox with both labels rather than only with the
> primary one?

Because that's what the design spec says. I don't think it's an unreasonable request to allow checkboxes to be controlled by label elements that were not set via label="". It works in HTML and it should work in XUL.
(In reply to Johann Hofmann [:johannh] from comment #3)
> > This also breaks clicking the label to toggle the checkbox.
> 
> You're right, we should file a bug about that, I'm pretty sure the control
> attribute should take care of it...

Well, we can file a bug on it, but we can't necessarily expect it to be fixed. XUL is on life support.

> > Why are we
> > center-aligning the checkbox with both labels rather than only with the
> > primary one?
> 
> Because that's what the design spec says.

That's circular reasoning. Why does the spec say so? Can it be revisited?
I agree with the spec that it looks better with both aligned, but I guess there's a point in that XUL won't ever support this. *sigh*
(In reply to Johann Hofmann [:johannh] from comment #5)
> I agree with the spec that it looks better with both aligned,

If you take into account accessibility, namely that users might expect the label to be clickable, then I think it makes quite a bit more sense to align the primary label with the checkbox. The secondary label just provides context and shouldn't be clickable.
Comment on attachment 8968534 [details]
Bug 1442542 - Make checkboxes in the clear site data dialog use a label attribute.

https://reviewboard.mozilla.org/r/237220/#review243700

The FTL looks good. Also CCed Zibi in case he wants to take a look at the code (not sure if you want to actually flag him)
Attachment #8968534 - Flags: review?(francesco.lodolo) → review+
Can you add this to your patch? It allows us to migrate strings from old .properties and DTDs.

These recipes live in
https://hg.mozilla.org/mozilla-unified/file/central/python/l10n/fluent_migrations
Yay! That looks awesome!

I did plan to do a bit more about unit formatting via mozIntl, and the result will affect localization here because we'll end up with sth like:

```
key = Cookies and Site Data ({ UNIT($amount) })
```

and:

```
document.l10n.setAttributes(element, "key", {
  amount: FluentUnit(amount, { unit })
});
```

but that's a more sizable project to get to (because I didn't write UnitFormat in bug 1415813 yet ;)), so let's land it now :)
Comment on attachment 8968534 [details]
Bug 1442542 - Make checkboxes in the clear site data dialog use a label attribute.

https://reviewboard.mozilla.org/r/237220/#review243890

::: browser/components/preferences/clearSiteData.xul:25
(Diff revision 2)
>    <script type="application/javascript" src="chrome://global/content/l10n.js"></script>
>  
>    <script src="chrome://browser/content/preferences/clearSiteData.js"/>
>  
>    <stringbundle id="bundlePreferences"
>                  src="chrome://browser/locale/preferences/preferences.properties"/>

Can you also remove this <stringbundle>?
Comment on attachment 8968534 [details]
Bug 1442542 - Make checkboxes in the clear site data dialog use a label attribute.

https://reviewboard.mozilla.org/r/237220/#review244156

::: browser/components/preferences/clearSiteData.xul:36
(Diff revision 3)
> -                 control="clearSiteData"
> -                 data-l10n-id="clear-site-data-cookies" />
> -          <description class="option-description" data-l10n-id="clear-site-data-cookies-info" />
> -        </vbox>
> +      </vbox>
> -      </hbox>
> -      <hbox class="option">
> +      <vbox class="option">
> +        <checkbox data-l10n-id="clear-site-data-cache-empty" id="clearCache" checked="true" />

nit: remove space before /> while you're touching this
Attachment #8968534 - Flags: review?(dao+bmo) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 16499ea8a9d0bd737a93d01bd118f8380d662e03 -d b758bc75b054: rebasing 459524:16499ea8a9d0 "Bug 1442542 - Make checkboxes in the clear site data dialog use a label attribute. r=dao,flod" (tip)
merging browser/locales/jar.mn
warning: conflicts while merging browser/locales/jar.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ddb032c4520
Make checkboxes in the clear site data dialog use a label attribute. r=dao,flod
https://hg.mozilla.org/mozilla-central/rev/2ddb032c4520
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Attached image highlight.png
Retested this issue on Windows 10 x64 and Windows 7 x64 using the latest Nightly 61.0a1(2018-04-25). 
Using the Tab key,the options are selected but the checkboxes aren't highlighted.
On mouse hover, the options are selected and the checkboxes are highlighted. ( please see the screenshot)

Johann, should I open a new bug for the highlight behaviour?
Flags: needinfo?(jhofmann)
(In reply to roxana.leitan@softvision.ro from comment #21)
> Retested this issue on Windows 10 x64 and Windows 7 x64 using the latest
> Nightly 61.0a1(2018-04-25). 
> Using the Tab key,the options are selected but the checkboxes aren't
> highlighted.
> On mouse hover, the options are selected and the checkboxes are highlighted.
> ( please see the screenshot)
> 
> Johann, should I open a new bug for the highlight behaviour?

No, I don't think that's unexpected :)
Flags: needinfo?(jhofmann)
Based on comment 21 and comment 22 I will mark this bug Verified fixed on Fx61.
You need to log in before you can comment on or make changes to this bug.