1536871 - Return the keyword value for line-height: normal in getComputedStyle.

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, enhancement, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Based on the discussion in https://github.com/w3c/csswg-drafts/issues/3749 we should change getComputedStyle so that for 'line-height: normal', it returns the keyword 'normal' rather than a length.

(It should continue to return lengths for <number> values of line-height.)

But before doing this, we should double-check that it matches Chromium.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... or maybe we should wait until the issue is resolved, but either way it might be good to provide some feedback there.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

We should also make line-height: -moz-block-height internal while at this.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1536871 - Refactor some enabledness checks. r=#style,dholbert,mats

There are some common checks that could get some easy-to-use aliases.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1536871 - Unship line-height: -moz-block-height. r=#style,dholbert,mats

They're only used in forms.css, and only for some anonymous content, which are
not content-accessible in the first place.

The only place where this could be exposed is calling
getComputedStyle(input, "::placeholder"), so I think this should be pretty safe,
but I've added a pref just in case.

While at it, also derive the Parse implementation. Less code is better.

Depends on D25117

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1536871 - Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=#style,dholbert,mats

I want to sort out:

https://github.com/w3c/csswg-drafts/issues/3749#issuecomment-477287453

Before potentially shipping this.

Depends on D25118

Comment 6

[Mats Palmgren (inactive)]

Unshipping -moz-block-height seems fine. I disagree with returning 'normal'
though and think that Chrome should get with the program and implement CSS2
instead. I've commented on the spec issue.
https://github.com/w3c/csswg-drafts/issues/3749#issuecomment-477512684

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I'll move the -moz-block-height to a different bug.

Comment 8

[Phabricator Automation]

Comment on attachment 9053984 [details]
Bug 1536871 - Unship line-height: -moz-block-height. r=#style,dholbert,mats

Revision D25118 was moved to bug 1540093. Setting attachment 9053984 [details] to obsolete.

Comment 9

[Phabricator Automation]

Comment on attachment 9053983 [details]
Bug 1536871 - Refactor some enabledness checks. r=#style,dholbert,mats

Revision D25117 was moved to bug 1540093. Setting attachment 9053983 [details] to obsolete.

Comment 10

[Cameron McCormack (:heycam)]

This was discussed in the WG again today, and the conclusion was to try again with changing gCS to return "normal".

https://github.com/w3c/csswg-drafts/issues/3749#issuecomment-499138167

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64b2f8306217
Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=dholbert

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17320 for changes under testing/web-platform/tests

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 64b2f8306217 (Bug 1536871) for dt failures at browser_fontinspector.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/c2465689746a3cb50bb27cf84a869655259c3c39

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=64b2f83062174be63c9ef05d9e2b03b6bfda9344&selectedJob=251745490

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251745490&repo=autoland&lineNumber=1071

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc71dba721d7
Backed out changeset 64b2f8306217 for dt failures at browser_fontinspector.js. CLOSED TREE

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 17

[Emilio Cobos Álvarez (:emilio)]

So this is throwing here: https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/devtools/client/inspector/fonts/fonts.js#318

Because now normal is not returning a numeric value. Razvan, do you know what other dependencies in the font inspector may be?

The issue is that normal is not quite just as setting a number (normal does other things), so we want to stop returning the pixel value from getComputedStyle (see the relevant csswg-drafts issue in https://github.com/w3c/csswg-drafts/issues/3749).

But it looks like the font inspector relies on getting it to a number value. I could fix it in multiple ways:

• Assuming normal is 1.2em is a reasonably close guess per this, though normal also accounts for some font information and this wouldn't, so it may not exactly always match the value that we return now. That being said, turning normal into a number is not idempotent, so this kind-of already happens.
• Adapting the font inspector UI to have a keyword control would be the correct thing, but I have no idea of how to do that.
• Maybe something else? Ideas welcome :)

Comment 18

[Razvan Caliman [:rcaliman]]

Adapting the font inspector UI to have a keyword control would be the correct thing, but I have no idea of how to do that.

We have a similar treatment for the React component corresponding to letter-spacing to handle keywords such as "normal" (I just noticed there's leftover naming as LineHeight instead of LetterSpacing in that component due to forking the original LineHeight.js component; apologies for the confusion. I'll fix that.) We can adapt the component for line-height to accept "normal" as a value.

We also need to handle that unitless conversion you pointed to in the font editor setup phase to watch out for keyword values (it now assumes gCS will return a numeric value)

I can work on this in bug 1559589.
With travel and the All Hands event coming up next week, I may be slower to do this. Let me know if this requires added urgency.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec13a60035f6
Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=dholbert

Comment 20

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ec13a60035f6

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5be8ef19ca92
Enable layout.css.line-height.normal-as-resolved-value.enabled on getComputedStyle-line-height.html. r=me

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5be8ef19ca92

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged