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

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Razvan Caliman [:rcaliman]]

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.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1467408 - (Part 1) Skip keyword values and use computed styles when populating the font editor.

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

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1467408 - (Part 2) Add test for keyword values in font editor UI.

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

Comment 3

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

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

Comment 4

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

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.

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 8987873 [details]
Bug 1467408 - (Part 2) Add test for keyword values in font editor UI.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/510771e64350
https://hg.mozilla.org/mozilla-central/rev/0899ad0c29c7