Replace listitem-iconic with richlistitem

RESOLVED FIXED in Firefox 63

Status

()

P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: timdream, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

58 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(6 attachments)

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.
(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?
Flags: needinfo?(myk)
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.
Flags: needinfo?(myk)
Component: XBL → XUL Widgets
Product: Core → Toolkit
(Not an XBL bug.)
Priority: -- → P5
(Assignee)

Updated

9 months ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P5 → P1
Comment hidden (mozreview-request)

Comment 9

9 months ago
mozreview-review
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?
(Assignee)

Comment 11

9 months ago
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.
Attachment #8987296 - Flags: ui-review?(shorlander)

Comment 12

9 months ago
mozreview-review
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

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

https://reviewboard.mozilla.org/r/252542/#review259014
Attachment #8987293 - Flags: review?(bgrinstead) → review+

Comment 14

9 months ago
mozreview-review
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?
Attachment #8987290 - Flags: review?(bgrinstead) → review+

Comment 15

9 months ago
mozreview-review
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
Attachment #8987292 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 16

9 months ago
(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

9 months ago
mozreview-review
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?
(Assignee)

Comment 18

9 months ago
(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

9 months ago
mozreview-review
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
Attachment #8987291 - Flags: review?(bgrinstead) → review+

Comment 20

9 months ago
Pushed by paolo.mozmail@amadzone.org:
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

9 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

8 months ago
Depends on: 1475920
You need to log in before you can comment on or make changes to this bug.