Closed Bug 1407347 Opened 7 years ago Closed 6 years ago

Remove tab in the sidebar causes the sidebar with iframes to rerender

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

bug1407347.patch
4.81 KB, patch
Details | Diff | Splinter Review
Bug 1407347 - Prevent iframes in the inspector sidebar from rerendering when a sidebar tab is removed.
59 bytes, text/x-review-board-request
Honza
: review+
Details 
STR:
1. Add "this.sidebar.removeTab("computedview") in http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#1632
2. Build and open DevTools with Firefox
3. Click on the "Add Node"/"+" button on top of the markup view in the inspector.
4. Navigate to the Animation Inspector tab which is an iframe.
5. Notice that content got re-render/reloaded - you will see missing strings, and the animation inspector is not initialized. Some event listeners do work. It is slightly puzzling.

Expected:
Removing a tab in the sidebar should not re-render an iframe.


@Needinfo honza, I tried looking into this quite deeply without much luck. I could use some fresh eyes on this.
This might be important if webextensions are able to add/remove sidebar tabs.
Flags: needinfo?(odvarko)
Attached patch bug1407347.patch — — Splinter Review 
I am attaching a prototype that shows the problem.

There are two issues:

1) The patch implements 'onUnmount' prop in InspectorTabPanel component so, the animation iframe's content (wrapped by this component) can be properly destroyed.
See: ToolSidebar.onSidePanelUnmounted(), this method calls `win.destroy`

This allows new initialization in case the Animation tab panel is unmounted and mounted again. It works well, but the content is reset. This is already progress, but the best would be to avoid unmounting entirely.


2) The patch partially shows how to avoid unmounting of a panel in the Tabs component.

First, we need to use stable `key` for a panel in Tabs component. An index doesn't work correctly since by removing a panel index changes, so the key changes and some panels are unmounted when re-rendered (those after the removed panel).

That's why the patch uses `tab.key` instead. `tab.id` might be better, I am not sure which value is always there. A combination like "tab.key || tab.id" could also work well.

The second part isn't implemented in the patch. It's related to the 'created' array.
This array allows lazy creation of panels, so a panel is created lazily when the user selects it for the first time. This array remembers what panels have been created. So, e.g. if panel at index 2 is selected then: this.created[2] = true;

Again, removing a panel changes indexes and the `created` array should be updated accordingly - to always reflect the list of tabs. Otherwise values are shifted and doesn't correspond to actual set of panels.

You can test this with the patch. Make sure the panel *before* the Animation panel is selected at least once and then remove "computedview" (which is also before the Animation panel). The Animation panel stays nicely the same.

So, you need to update the 'created' array somehow. I am not sure what is the best way here. Any suggestions?

Perhaps we could use an object/record and 'tab.id || tab.key' as the key (again not index), so removing a panel doesn't change order of things. But, we would still need to ensure that the object is updated, so the number of keys inside doesn't grow.

Does that make sense?

Honza
Flags: needinfo?(odvarko)
status-firefox57: --- → fix-optional
Priority: -- → P3
Assignee: nobody → gl
Status: NEW → ASSIGNED
Blocks: 1433716
No longer blocks: 1369945
Comment on attachment 8946161 [details]
Bug 1407347 - Prevent iframes in the inspector sidebar from rerendering when a sidebar tab is removed.

https://reviewboard.mozilla.org/r/216152/#review221900


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

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


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


::: devtools/client/shared/components/tabs/Tabs.js:129
(Diff revision 1)
> -      // (it's 0-based index).
> -      if (typeof tabActive === "number") {
> -        let panels = children.filter((panel) => panel);
> +      // update the state's created array with the latest tab ids and whether or not
> +      // the tab is already created.
> +      if (this.state.created.length != panels.length) {
> +        created = panels.map(panel => {
> +          // Get whether or not the tab has already been created from the previous state.
> +          let createdEntry = this.state.created.find(created => {

Error: 'created' is already declared in the upper scope. [eslint: no-shadow]
Comment on attachment 8946161 [details]
Bug 1407347 - Prevent iframes in the inspector sidebar from rerendering when a sidebar tab is removed.

https://reviewboard.mozilla.org/r/216152/#review221972

Looks good to me.

R+ assuming try is green.

Thanks!
Honza
Attachment #8946161 - Flags: review?(odvarko) → review+
Comment on attachment 8946161 [details]
Bug 1407347 - Prevent iframes in the inspector sidebar from rerendering when a sidebar tab is removed.

https://reviewboard.mozilla.org/r/216152/#review222052

Looks good, thanks!

Honza
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4114e1e168ea
Prevent iframes in the inspector sidebar from rerendering when a sidebar tab is removed. r=Honza
https://hg.mozilla.org/mozilla-central/rev/4114e1e168ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: