Closed Bug 1188071 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools Graveyard :: WebIDE, defect)

42 Branch
defect
Not set
normal

Tracking

(firefox42 affected, firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: bstoroz, Assigned: bstoroz)

Details

Attachments

(3 files, 1 obsolete file)

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)
Assignee: nobody → bstoroz
Summary: WebIDE project panel can't detect project icon when first adding it to sidebar → WebIDE project panel doesn't remove placeholder icon or text when adding a new app
Attached patch removePlaceholderContent.patch (obsolete) — Splinter Review
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
Attachment #8644472 - Flags: feedback?(jryans)
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
Attachment #8644472 - Flags: feedback?(jryans) → feedback+
Added a check for the panel child elements before creating new ones in `renderProjectItem` and fixed coding style nit.
Attachment #8644472 - Attachment is obsolete: true
Attachment #8646612 - Flags: review?(jryans)
Comment on attachment 8646612 [details] [diff] [review]
removePlaceholderContent-v2.patch

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

Great, thanks for working on this!
Attachment #8646612 - Flags: review?(jryans) → review+
Keywords: checkin-needed
I believe you'll need to include a try run before the sheriffs will land this.
Flags: needinfo?(bstoroz)
tru dat
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/652302739d75
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.