1188071 - WebIDE project panel doesn't remove placeholder icon or text when adding a new app

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Brittany Storoz [:bstoroz]]

Attachment: projectAddedToWebIDE.png

When we first add a project in WebIDE, it seems like the project panel in the sidebar doesn't realize it has successfully grabbed the project icon. It loads a placeholder icon, a span tag with '--', and the application icon. (See screenshot attachment projectAddedToWebIDE)

If you close WebIDE and reopen it, the problem is resolved. The placeholder icon and span tag are gone, and it shows just the application icon and name. (attachment projectExistsInWebIDE)

Comment 1

[Brittany Storoz [:bstoroz]]

Attachment: projectExistsInWebIDE.png

Comment 2

[Brittany Storoz [:bstoroz]]

Attachment: removePlaceholderContent.patch

The issue seems to be caused by calling the `_renderProjectItem` method several times when adding a new app.  It looks like some of the repetitiveness is meant to maintain support for the drop-down panels that the sidebars are replacing, but 4 times is a little excessive...

Previously the code accounted for missing name & icon properties by falling back to default values (the placeholder content) and rendering the project item anyway. https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/modules/project-list.js?offset=1300#341-345 

But right beneath that we're doing another check for those same missing properties to use as an indicator of whether or not the app has been validated and its information is available https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/modules/project-list.js?offset=1300#346-357

This causes the new project item to be rendered with both the placeholder content, and the app information once it's available.

In my patch I changed this conditional to check for the existence of a `validationStatus` property. It seems to always be undefined until the new application has gone through validation and its information is stored. This makes sure `_renderProjectItem` is only called once the application information is available.

Another possible solution is just checking for the existence of those child elements in the project item node, and replacing their textContent instead of appending new ones on the subsequent call to `_renderProjectItem` https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/modules/project-list.js?offset=1700#131-139

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8644472 [details] [diff] [review]
removePlaceholderContent.patch

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

I think the fix you have here seems fine to me, but it would also be nice to make `_renderProjectItem` more robust, so it can't cause these issues in the first place.

So, maybe do this *and* something like the alternative?  It could fix up existing child elements, or it could start by removing all children and always appending new ones.

::: browser/devtools/webide/modules/project-list.js
@@ +349,5 @@
>                icon: project.icon
>              });
>            });
>          }
> +        else {

Nit: DevTools style uses } else {

You can set up ESLint to warn for style things[1].

[1]: https://wiki.mozilla.org/DevTools/CodingStandards

Comment 4

[Brittany Storoz [:bstoroz]]

Attachment: removePlaceholderContent-v2.patch

Added a check for the panel child elements before creating new ones in `renderProjectItem` and fixed coding style nit.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8646612 [details] [diff] [review]
removePlaceholderContent-v2.patch

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

Great, thanks for working on this!

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I believe you'll need to include a try run before the sheriffs will land this.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

tru dat

Comment 8

[Brittany Storoz [:bstoroz]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/652302739d75

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/652302739d75