1471893 - Font editor: disabling CSS properties in the Rule view should disable corresponding editing capabilities

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Razvan Caliman [:rcaliman]]

Attachment: font-editor_disabled-properties.mp4

Steps:
- Go to https://v-fonts.com/
- Inspect the element for the first font preview
- Open the Font Editor
- Change the font size by with the Size slider
- Switch to the Rule view and uncheck the 'font-size' property of the inline style
- Switch back to the Font Editor and continue editing with the Size slider

Expected:
- The Size slider is disabled because changes won't have any effect.

Actual:
- The Size slider shows the fallback value that applies when the font-size from the inline style is disabled (correct). Dragging on the slider changes the value of the disabled font-style property, but no results show on the page (correct). After a brief moment, the slider jumps back to the actual font-size (confusing)


Take care about values that can be written either to `font-variation-settings` or `font-*` properties. Ex: Value of WGHT axis should be written to `font-weight` if `font-variation-settings` is disabled and vice-versa.

Comment 1

[Martin Balfanz [:mbalfanz]]

I don't think disabling sliders is the best solution.

Instead, I suggest that our editor creates a new property instead of writing to the disabled one.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1471893 - Write to a new property if the one we're trying is disabled

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8993676 [details]
Bug 1471893 - Write to a new property if the one we're trying is disabled

this seems to do the trick, but I'm not certain whether this will work in all cases. 
Razvan, what are your thoughts on this?
Also, I most probably won't have the time to push this to completion (especially writing a test) since I'm going to be off end of next week.
So feel free to pick it up if you have time.

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: property-overwrite.mp4

Your attached solution is nice and elegant. I like it!

It works for most cases. The check needs to be updated to ensure the property isn't overwritten either, else the wrong TextProperty is still selected as shown in the attached video.

.find(prop => prop.name === name && prop.enabled);

needs to become:

.find(prop => prop.name === name && prop.enabled && !prop.overridden);

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1471893 - Write to a new property if the one we're trying is disabled or overwritten.

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

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8994215 [details]
Bug 1471893 - Write to a new property if the one we're trying is disabled or overwritten.

https://reviewboard.mozilla.org/r/258830/#review265770

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/500fb6bfc871
Write to a new property if the one we're trying is disabled or overwritten. r=pbro

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/500fb6bfc871

Comment 9

[Catalin Sasca, Desktop QA [:csasca]]

I successfully reproduced the issue on Firefox Nightly 63.0a1 (2018-06-28) under Windows 10 (x64) using the STR from Comment 0.
The issue is no longer reproducible on Firefox Beta 63.0b13 and latest Nightly 64.0a1 (2018-10-08). Tests were performed under Windows 10 (x64), macOS 10.13, and Ubuntu 16.04 (x64).