Closed Bug 1467408 Opened 4 years ago Closed 4 years ago

Font editor: properties with keywords as value should be read from computed style

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(2 files)

Bug 1465397 introduced the ability to read values as defined in the rule view and fallback to computed style when populating the Font Editor UI.

It fails to account for declarations like "font-weight: bold". Instead of showing a numeric value in font-weight input it shows up blank.

The Font Editor should fallback to reading from computed styles whenever it encounters declarations where the property value is a keyword/identifier.
Product: Firefox → DevTools
Comment on attachment 8987872 [details]
Bug 1467408 - (Part 1) Skip keyword values and use computed styles when populating the font editor.

https://reviewboard.mozilla.org/r/253152/#review259898
Attachment #8987872 - Flags: review?(gl) → review+
Comment on attachment 8987873 [details]
Bug 1467408 - (Part 2) Add test for keyword values in font editor UI.

https://reviewboard.mozilla.org/r/253154/#review259900

::: devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js:4
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";

Add a new line before "use strict"

::: devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js:5
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +

We should add a comment describing what this unit test does. You can see an example here https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_grid-toggle_04.js#

::: devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js:20
(Diff revision 1)
> +
> +function getPropertyValue(viewDoc, name) {
> +  const selector = `#font-editor .font-value-slider[name=${name}]`;
> +  return {
> +    value: viewDoc.querySelector(selector).value,
> +    // Ensure unit dropdow exists before querying its value

s/dropdow/dropdown

::: devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js:29
(Diff revision 1)
> +}
> +
> +async function testKeywordValues(inspector, viewDoc) {
> +  await selectNode(".bold-text", inspector);
> +
> +  info("Test font-weight");

We should describe what is being tested in these 2 info().

Something like:
Check font-weight shows its computed style instead of keyword value.
Attachment #8987873 - Flags: review?(gl) → review+
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/510771e64350
(Part 1) Skip keyword values and use computed styles when populating the font editor. r=gl
https://hg.mozilla.org/integration/autoland/rev/0899ad0c29c7
(Part 2) Add test for keyword values in font editor UI. r=gl
https://hg.mozilla.org/mozilla-central/rev/510771e64350
https://hg.mozilla.org/mozilla-central/rev/0899ad0c29c7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.