Font editor: Empty state for FontEditor mistakenly shows up when its pref is OFF

VERIFIED FIXED in Firefox 62

Status

defect
P1
normal
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

({regression})

unspecified
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 verified, firefox63 verified, firefox64 verified)

Details

Attachments

(2 attachments)

Assignee

Description

11 months ago
The patch for Bug 1464796 accidentally removed a guard that now causes the FontEditor to show up in its empty state with the message "No fonts were found for the current element" even when the FontEditor pref is OFF. 

This has slipped into the 62 build, currently in Beta.
Comment hidden (mozreview-request)

Comment 2

11 months ago
mozreview-review
Comment on attachment 8988513 [details]
Bug 1471914 - Check that the font editor pref is on before rendering it.

https://reviewboard.mozilla.org/r/253780/#review260420

Looks good to me, and you confirmed that this worked fine (I didn't test it myself).
Please let me know if you need any help getting it uplifted since this will be your first beta uplift request.
Attachment #8988513 - Flags: review?(pbrosset) → review+

Comment 3

11 months ago
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/988ea9310c36
Check that the font editor pref is on before rendering it. r=pbro

Comment 4

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/988ea9310c36
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee

Comment 5

11 months ago
Comment on attachment 8988513 [details]
Bug 1471914 - Check that the font editor pref is on before rendering it.

Approval Request Comment
[Feature/Bug causing the regression]:
Patch for Bug 1464796 accidentally removed a condition which makes the default empty state of the Font editor (which is in development) visible to users who don't have the pref turned on to see it.

[User impact if declined]:
Users see a confusing message in the Fonts panel that "No fonts were found for this element" right above the list of fonts which were found.

[Is this code covered by automated tests?]:
Not completely. The existing Font panel has tests. The upcoming Font Editor tests are being written now.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
Yes.

Steps:
1. Open a new tab page
2. Open DevTools and select the "<body>" tag in the Elements panel
3. Switch to "Fonts" sidebar panel

Expected:
The message "No fonts were found for the current element" should not appear in the Fonts panel. A list of one or more fonts should show up.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Unlikely. It restores UI as it was prior to this bug occurring.

[Why is the change risky/not risky?]:
It reintroduces a condition that was previously available. Instead of its boolean value coming from checking an array's length, it now comes from checking the pref value for enabling the font editor: devtools.inspector.fonteditor.enabled

[String changes made/needed]:
None
Attachment #8988513 - Flags: approval-mozilla-beta?
Comment on attachment 8988513 [details]
Bug 1471914 - Check that the font editor pref is on before rendering it.

Put back a needed pref check, please uplift for beta 7.
Attachment #8988513 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: catalin.sasca
I successfully reproduced the issue on Nightly 63.0a1 (2018-06-28) under Windows 10 (x64) using STR from Comment 5.

The issue is no longer reproducible on Firefox 62.0, Firefox Beta 63.0b4 and latest Nightly 64.0a1 (2018-09-10) under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.13.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.