1472750 - Convert simple "listbox" instances to "richlistbox"

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[:Paolo Amadini]

This covers the simple cases from bug 1472554.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

These simple lists are converted to normal layout by setting a fixed height that isn't a multiple of the row height, which is already the case for most other lists in the user interface.

Review commit: https://reviewboard.mozilla.org/r/254264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254264/

Comment 2

[Brian Grinstead [:bgrins]]

Is it always the case that 1 row = 20px, or does this change the height in some cases?

Comment 3

[:Paolo Amadini]

No, the lists are generally slightly taller than before. The exact item height generally depends on the font size, which can change a bit between platforms.

Comment 4

[:Paolo Amadini]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

I've actually realized that there are additional focus-related styles applied to "listitem" but not "richlistitem". I'm thinking of adding an attribute like styling="listbox" on the "richlistbox" elements I'm converting here.

Comment 5

[:Paolo Amadini]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254264/diff/1-2/

Comment 6

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c501f0f215e7cccd2a0ac452fe760df017e261de

Comment 7

[:Paolo Amadini]

I've ported the fancy selection styling from "listbox" mainly for the benefit of the "Edit This Bookmark" panel in bug 1472748. The richlistboxes updated here are secondary or rarely used UI, so the basic style might have worked as well, but I've included it here so all the lists appear exactly as before.

I've not ported one or two styles for disabled items because they are unused.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

https://reviewboard.mozilla.org/r/254264/#review262386

::: browser/components/preferences/siteDataRemoveSelected.xul:48
(Diff revision 2)
> -    <separator />
> +    <separator/>
>  
> -    <vbox flex="1">
> -      <label data-l10n-id="site-data-removing-table"/>
> +    <label data-l10n-id="site-data-removing-table"/>
> -      <separator class="thin"/>
> +    <separator class="thin"/>
> -      <listbox id="removalList"/>
> +    <richlistbox id="removalList" styled="true" flex="1"/>

`[styled=true]` isn't very descriptive of what's happening here. I'd suggest (1) using a class instead of an attribute since this is only used for styling purposes and (2) renaming it to something that more explains what's going on - maybe `.listbox`, or `.theme-listbox`, or something?

Comment 9

[:Paolo Amadini]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254264/diff/2-3/

Comment 10

[:Paolo Amadini]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254264/diff/3-4/

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8989220 [details]
Bug 1472750 - Convert simple "listbox" instances to "richlistbox".

https://reviewboard.mozilla.org/r/254264/#review262650

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b45728b7210
Convert simple "listbox" instances to "richlistbox". r=bgrins

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0b45728b7210