1462328 - Font editor: crash when trying to toggle font editor on <img> element

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Razvan Caliman [:rcaliman]]

Steps:

- Go to: https://meyerweb.com/eric/thoughts/2017/03/20/handiwork/
- Inspect one of the images, for example <img id="book" />
- Click the toggle for the font editor on the rule with selector #book


Expected:
- Font editor shows up.

Result
- Font editor shows up blank. Subsequent toggle on other rules no longer works.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1462328 - Reset editor when no fonts found on node.

- Render when no fonts found on selected node, regardless of node type.
- Kickstart render when sidebar tab is selected.
- Avoid needless renders if sidebar tab is not visible.

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

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8980568 [details]
Bug 1462328 - Reset editor when no fonts found on node.

https://reviewboard.mozilla.org/r/246718/#review253122

::: devtools/client/inspector/fonts/fonts.js:112
(Diff revision 1)
>  
>      // Expose the provider to let inspector.js use it in setupSidebar.
>      this.provider = provider;
>  
>      this.inspector.selection.on("new-node-front", this.onNewNode);
> +    this.inspector.sidebar.on("select", this.onSidebarTabSelected);

We can avoid checking the tabID if we listened for "fontinspector-selected".

https://searchfox.org/mozilla-central/source/devtools/client/inspector/toolsidebar.js#317

::: devtools/client/inspector/fonts/fonts.js:571
(Diff revision 1)
>      const node = this.inspector.selection.nodeFront;
>      const fonts = await this.getFontsForNode(node, options);
> +    if (!fonts.length) {
> +      this.store.dispatch(resetFontEditor());
> +      return;
> +    }

Add a new line after this.

::: devtools/client/inspector/fonts/fonts.js:625
(Diff revision 1)
>      let otherFonts = [];
>  
>      let { fontOptions } = this.store.getState();
>      let { previewText } = fontOptions;
>  
> -    if (!this.isSelectedNodeValid() || !this.isPanelVisible()) {
> +    if (!this.isSelectedNodeValid()) {

Let's rearrange this a bit:

if (!this.inspector || !this.store) {
  return;
}

let fonts = [];
let otherFonts = [];

if (!this.isSelectedNodeValid()) {
  this.store.dispatch(updateFonts(fonts, otherFonts));
  return;
}

let node = ..
let { fontOptions } = ..
let { previewText } = ..

I think we should just specify fonts, otherFonts so we can infer what types of arguments updateFonts is taking. We can also early return without having call getSTate and do all the destructuring.

Comment 3

[Razvan Caliman [:rcaliman]]

Comment on attachment 8980568 [details]
Bug 1462328 - Reset editor when no fonts found on node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246718/diff/1-2/

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 8980568 [details]
Bug 1462328 - Reset editor when no fonts found on node.

https://reviewboard.mozilla.org/r/246718/#review253122

> We can avoid checking the tabID if we listened for "fontinspector-selected".
> 
> https://searchfox.org/mozilla-central/source/devtools/client/inspector/toolsidebar.js#317

Good catch!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cbdaf5a7938
Reset editor when no fonts found on node. r=gl

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7cbdaf5a7938