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

RESOLVED FIXED in Firefox 62

Status

defect
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

unspecified
Firefox 62

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [good first verify])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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.
Attachment #8980568 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
mozreview-review-reply
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

a year ago
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cbdaf5a7938
Reset editor when no fonts found on node. r=gl

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cbdaf5a7938
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.