1435688 - Replace listitem-iconic with richlistitem

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, task, P1)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

There are only two instances of usage of listitem-iconic.

- preferences/applicationManager.xul
- toolkit/profile/content/profileSelection.xul

It should be replacable with richlistitem binding.

Comment 1

[Marcos Caceres [:marcosc]]

(doing triage on behalf of DOM Team)

Checked the files, and it seems "listitem-iconic" is not longer there (but neither is richlistitem). 

@myk, git log shows that you worked on these files recently. Could you confirm if this has been resolved or other work is needed here?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry, This bug should be more descriptive.

These two XUL pages use <listitem class="listitem-iconic">s indirectly through <listbox>s. The work required here should have them use <richlistbox>s instead.

Comment 3

[Olli Pettay [:smaug]]

(Not an XBL bug.)

Comment 4

[:Paolo Amadini]

Attachment: Bug 1435688 - Part 1 - Replace "listitem-iconic" with "richlistitem" in the Windows handler application picker dialog.

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

Comment 5

[:Paolo Amadini]

Attachment: Bug 1435688 - Part 2 - Replace "listitem-iconic" with "richlistitem" in the application manager dialog and adjust the styling.

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

Comment 6

[:Paolo Amadini]

Attachment: Bug 1435688 - Part 3 - Remove "listitem-iconic" from the profile selection dialog.

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

Comment 7

[:Paolo Amadini]

Attachment: Bug 1435688 - Part 4 - Remove the "listitem-iconic" and "listcell-iconic" bindings.

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

Comment 8

[:Paolo Amadini]

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

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8987292 [details]
Bug 1435688 - Part 3 - Remove "listitem-iconic" from the profile selection dialog.

https://reviewboard.mozilla.org/r/252540/#review259008

Looks like this might be removing an image from the profile selection UI. The code changes look nice, but can you get UI review for this?

Comment 10

[:Paolo Amadini]

Attachment: Profile selection dialog before the change

Comment 11

[:Paolo Amadini]

Attachment: Profile selection dialog after the change

I'm removing this icon because we're removing the XBL binding that implements it.

This is an advanced user interface that we eventually plan to remove and replace with "about:profiles", and I don't think the icon adds any value. It also doesn't look right in HiDPI.

Comment 12

[Code Review Bot [:reviewbot]]

Comment on attachment 8987291 [details]
Bug 1435688 - Part 2 - Replace "listitem-iconic" with "richlistitem" in the application manager dialog and adjust the styling.

https://reviewboard.mozilla.org/r/252538/#review259012


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/applicationManager.js:42
(Diff revision 1)
>          continue;
>  
>        app.QueryInterface(Ci.nsIHandlerApp);
> -      var item = list.appendItem(app.name);
> -      item.setAttribute("image", gMainPane._getIconURLForHandlerApp(app));
> -      item.className = "listitem-iconic";
> +
> +      // Ensure the XBL binding is created eagerly.
> +      list.appendChild(MozXULElement.parseXULToFragment("<richlistitem/>"));

Error: 'mozxulelement' is not defined. [eslint: no-undef]

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8987293 [details]
Bug 1435688 - Part 4 - Remove the "listitem-iconic" and "listcell-iconic" bindings.

https://reviewboard.mozilla.org/r/252542/#review259014

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8987290 [details]
Bug 1435688 - Part 1 - Replace "listitem-iconic" with "richlistitem" in the Windows handler application picker dialog.

https://reviewboard.mozilla.org/r/252536/#review259010

::: toolkit/components/apppicker/content/appPicker.js:80
(Diff revision 1)
>                  continue;
>            } catch (err) {
>              continue;
>            }
>  
> -          var item = document.createElement("listitem");
> +          var item = document.createElement("richlistitem");

I guess this doesn't technically need to use MozXULElement.parseXULToFragment since `handlerApp` is just an expando property so we don't access and XBL props/methods before appending to the list.

I don't think we need to change it here - this seems to be a common pattern in the frontend. I'm just surprised we hadn't run into more bugs that prompted a helper like `parseXULToFragment` earlier.

::: toolkit/components/apppicker/content/appPicker.xul:36
(Diff revision 1)
>        </vbox>
>      </hbox>
>  
>      <label id="sendto-message" value="&SendMsg.label;" control="app-picker-listbox"/>
>  
> -    <listbox id="app-picker-listbox" rows="5"
> +    <richlistbox id="app-picker-listbox"

Does removing the [rows=5] change anything in the actual UI here?

::: toolkit/themes/shared/appPicker.css:15
(Diff revision 1)
>  #content-description {
>    font-weight: bold;
>  }
>  
> +#app-picker-listbox {
> +  height: 212px;

Where's this number come from? And does it look right on all platforms?

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8987292 [details]
Bug 1435688 - Part 3 - Remove "listitem-iconic" from the profile selection dialog.

https://reviewboard.mozilla.org/r/252540/#review259016

Code change looks fine

Comment 16

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> I guess this doesn't technically need to use
> MozXULElement.parseXULToFragment since `handlerApp` is just an expando
> property so we don't access and XBL props/methods before appending to the
> list.
> 
> I don't think we need to change it here - this seems to be a common pattern
> in the frontend. I'm just surprised we hadn't run into more bugs that
> prompted a helper like `parseXULToFragment` earlier.

Yeah, I think that's what happens, and I agree in all the cases where we do this it is not obvious that we can only use expando properties, but not the properties of the binding.

> Does removing the [rows=5] change anything in the actual UI here?

This has no effect on the richlistbox, so I had to set the height using CSS.

> ::: toolkit/themes/shared/appPicker.css:15
> > +  height: 212px;
> 
> Where's this number come from? And does it look right on all platforms?

This is 5 rows plus 2 pixels for the border. It looks correct on Windows and Mac, I actually haven't tested Linux, but I don't expect it to be different because richlistitems have no special padding or margins.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8987291 [details]
Bug 1435688 - Part 2 - Replace "listitem-iconic" with "richlistitem" in the application manager dialog and adjust the styling.

https://reviewboard.mozilla.org/r/252538/#review259018

::: browser/themes/linux/preferences/applications.css:72
(Diff revision 1)
> +
> +#appList {
> +  min-height: 212px;
> +}
> +
> +#appList > richlistitem {

I almost want to share this (richlistitem/image/label) styling with a class on richlistitem in a shared preference CSS file somewhere. But given that there are only two consumers after this patch set and the relatively simple style I'd be OK to not.

Sidenote: is applications.css the right place for this? Given that the applicationManager.xul file wasn't previously loading this sheet, is there somewhere else we should put these styles?

Comment 18

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Sidenote: is applications.css the right place for this? Given that the
> applicationManager.xul file wasn't previously loading this sheet, is there
> somewhere else we should put these styles?

This is part of the same logical grouping of "handler applications". I thought about creating a new stylesheet just for this dialog, but in the end I went for having fewer files in the source tree, especially given the small number of rules.

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8987291 [details]
Bug 1435688 - Part 2 - Replace "listitem-iconic" with "richlistitem" in the application manager dialog and adjust the styling.

https://reviewboard.mozilla.org/r/252538/#review259442

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4483793aad53
Part 1 - Replace "listitem-iconic" with "richlistitem" in the Windows handler application picker dialog. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf903b7f772a
Part 2 - Replace "listitem-iconic" with "richlistitem" in the application manager dialog and adjust the styling. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/a33a55fb08a6
Part 3 - Remove "listitem-iconic" from the profile selection dialog. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/070524bca3b8
Part 4 - Remove the "listitem-iconic" and "listcell-iconic" bindings. r=bgrins

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4483793aad53
https://hg.mozilla.org/mozilla-central/rev/cf903b7f772a
https://hg.mozilla.org/mozilla-central/rev/a33a55fb08a6
https://hg.mozilla.org/mozilla-central/rev/070524bca3b8