Closed Bug 1536871 Opened 7 months ago Closed 4 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: dbaron, Assigned: emilio)

References

Details

(Keywords: site-compat)

Attachments

(1 file, 2 obsolete files)

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.

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

Priority: -- → P3

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

Assignee: nobody → emilio
Flags: needinfo?(emilio)
Summary: 'line-height: normal' should be returned as a keyword from getComputedStyle → Hide line-height: -moz-block-height UA-only, and return the keyword value for line-height: normal in getComputedStyle.

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

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

Flags: needinfo?(emilio)

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

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

Summary: Hide line-height: -moz-block-height UA-only, and return the keyword value for line-height: normal in getComputedStyle. → Return the keyword value for line-height: normal in getComputedStyle.
Depends on: 1540093

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.

Attachment #9053984 - Attachment is obsolete: true

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.

Attachment #9053983 - Attachment is obsolete: true

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

Attachment #9053985 - Attachment description: Bug 1536871 - Make line-height always return the computed value on Nightly, for now. r=#style,dholbert,mats → Bug 1536871 - Make line-height always return the computed value on Nightly and Early Beta, for now. r=#style,dholbert,mats
Attachment #9053985 - Attachment description: Bug 1536871 - Make line-height always return the computed value on Nightly and Early Beta, for now. r=#style,dholbert,mats → Bug 1536871 - Make line-height always return the computed value for 'normal' on Nightly and Early Beta, for now. r=#style,dholbert,mats
Attachment #9053985 - Attachment description: Bug 1536871 - Make line-height always return the computed value for 'normal' on Nightly and Early Beta, for now. r=#style,dholbert,mats → Bug 1536871 - Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=#style,dholbert,mats
Pushed by ealvarez@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17320 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Backout by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc71dba721d7
Backed out changeset 64b2f8306217 for dt failures at browser_fontinspector.js. CLOSED TREE
Upstream PR was closed without merging

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 :)
Flags: needinfo?(emilio) → needinfo?(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.

Flags: needinfo?(rcaliman)
No longer blocks: 1559589
Depends on: 1559589
Pushed by ealvarez@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1560791
Pushed by emilio@crisal.io:
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
Upstream PR merged
Regressions: 1564443
Depends on: 1579624
You need to log in before you can comment on or make changes to this bug.