Bug 1536871 - Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=dholbert
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 14 Jun 2019 09:01:44 +0000
changeset 479867 ec13a60035f66bcce7a30e9643aaad40d056aad5
parent 479866 52a73b4a9e9cb69d8653acf1f6104a02623df109
child 479868 d347259eac05960df216f83d4935530df333b7cc
push id36186
push userrmaries@mozilla.com
push dateSat, 22 Jun 2019 21:44:43 +0000
treeherdermozilla-central@eda066bb80dd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1536871
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1536871 - Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=dholbert Differential Revision: https://phabricator.services.mozilla.com/D25119
devtools/client/shared/test/browser_inplace-editor_maxwidth.js
layout/reftests/bugs/reftest.list
layout/style/nsComputedDOMStyle.cpp
modules/libpref/init/StaticPrefList.h
testing/web-platform/tests/css/cssom/getComputedStyle-line-height.html
testing/web-platform/tests/html/rendering/replaced-elements/the-select-element/select-1-line-height.html
--- a/devtools/client/shared/test/browser_inplace-editor_maxwidth.js
+++ b/devtools/client/shared/test/browser_inplace-editor_maxwidth.js
@@ -92,17 +92,17 @@ const testMaxWidth = async function(edit
  * Retrieve the current number of lines displayed in the provided textarea.
  *
  * @param {DOMNode} textarea
  * @return {Number} the number of lines
  */
 function getLines(textarea) {
   const win = textarea.ownerDocument.defaultView;
   const style = win.getComputedStyle(textarea);
-  return Math.floor(textarea.clientHeight / parseFloat(style.lineHeight));
+  return Math.floor(textarea.clientHeight / parseFloat(style.fontSize));
 }
 
 /**
  * Verify that the provided textarea has no vertical or horizontal scrollbar.
  *
  * @param {DOMNode} textarea
  */
 function checkScrollbars(textarea) {
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -647,17 +647,17 @@ asserts(4) == 368155-negative-margins-1.
 == 370525-rowspan-2b.html 370525-rowspan-2b-ref.html
 == 370525-rowspan-3.html 370525-rowspan-3-ref.html
 == 370525-rowspan-4.html 370525-rowspan-4-ref.html
 == 370525-sib.html 370525-sib-ref.html
 == 370586-1.xhtml 370586-1-ref.xhtml
 == 370629-1.html 370629-1-ref.html
 == 370629-2.html 370629-2-ref.html
 == 370692-1.xhtml 370692-1-ref.xhtml
-== 371041-1.html 371041-1-ref.html
+pref(layout.css.line-height.normal-as-resolved-value.enabled,false) == 371041-1.html 371041-1-ref.html
 == 371043-1.html 371043-1-ref.html
 == 371354-1.html 371354-1-ref.html
 == 371483-1.html about:blank # assertion test
 fails-if(Android&&!asyncPan) == 371561-1.html 371561-1-ref.html
 != 371681-1.xhtml about:blank
 == 371925-1a.html 371925-1-ref.html
 == 371925-1b.html 371925-1-ref.html
 == 372037-1.html 372037-1-ref.html
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -2099,18 +2099,27 @@ already_AddRefed<CSSValue> nsComputedDOM
 
   return val.forget();
 }
 
 bool nsComputedDOMStyle::GetLineHeightCoord(nscoord& aCoord) {
   AssertFlushedPendingReflows();
 
   nscoord blockHeight = NS_UNCONSTRAINEDSIZE;
-  if (StyleText()->mLineHeight.IsMozBlockHeight()) {
-    if (!mInnerFrame) return false;
+  const auto& lh = StyleText()->mLineHeight;
+
+  if (lh.IsNormal() &&
+      StaticPrefs::layout_css_line_height_normal_as_resolved_value_enabled()) {
+    return false;
+  }
+
+  if (lh.IsMozBlockHeight()) {
+    if (!mInnerFrame) {
+      return false;
+    }
 
     if (nsLayoutUtils::IsNonWrapperBlock(mInnerFrame)) {
       blockHeight = mInnerFrame->GetContentRect().height;
     } else {
       GetCBContentHeight(blockHeight);
     }
   }
 
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -5430,16 +5430,44 @@ VARCACHE_PREF(
 // Is support for -webkit-line-clamp enabled?
 VARCACHE_PREF(
   Live,
   "layout.css.webkit-line-clamp.enabled",
   layout_css_webkit_line_clamp_enabled,
   bool, true
 )
 
+// Whether the computed value of line-height: normal returns the `normal`
+// keyword rather than a pixel value based on the first available font.
+//
+// Only enabled on Nightly and early beta, at least for now.
+//
+// NOTE(emilio): If / when removing this pref, the GETCS_NEEDS_LAYOUT_FLUSH flag
+// should be removed from line-height (and we should let -moz-block-height
+// compute to the keyword as well, which shouldn't be observable anyway since
+// it's an internal value).
+//
+// It'd be nice to make numbers compute also to themselves, but it looks like
+// everybody agrees on turning them into pixels, see the discussion starting
+// from [1].
+//
+// [1]: https://github.com/w3c/csswg-drafts/issues/3749#issuecomment-477287453
+#ifdef EARLY_BETA_OR_EARLIER
+#define PREF_VALUE true
+#else
+#define PREF_VALUE false
+#endif
+VARCACHE_PREF(
+  Live,
+  "layout.css.line-height.normal-as-resolved-value.enabled",
+  layout_css_line_height_normal_as_resolved_value_enabled,
+  bool, PREF_VALUE
+)
+#undef PREF_VALUE
+
 //---------------------------------------------------------------------------
 // Prefs starting with "media."
 //---------------------------------------------------------------------------
 
 // These prefs use camel case instead of snake case for the getter because one
 // reviewer had an unshakeable preference for that. Who could that be?
 
 VARCACHE_PREF(
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom/getComputedStyle-line-height.html
@@ -0,0 +1,23 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test: line-height resolved value</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#resolved-values">
+<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/3749">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  div { font-size: 16px; }
+</style>
+<div style="line-height: normal" data-expected="normal"></div>
+<div style="line-height: 1" data-expected="16px"></div>
+<div style="line-height: 10px" data-expected="10px"></div>
+<div style="line-height: 10%" data-expected="1.6px"></div>
+<script>
+for (const e of document.querySelectorAll("div")) {
+  const specified = e.style.lineHeight;
+  test(function() {
+    const expected = e.getAttribute("data-expected");
+    assert_equals(getComputedStyle(e).lineHeight, expected, specified + " should compute to " + expected);
+  }, "line-height: " + specified);
+}
+</script>
--- a/testing/web-platform/tests/html/rendering/replaced-elements/the-select-element/select-1-line-height.html
+++ b/testing/web-platform/tests/html/rendering/replaced-elements/the-select-element/select-1-line-height.html
@@ -21,15 +21,16 @@ select { line-height:100px; }
 </head>
 <body>
 
 <select><option>aaaaaaaaaa<option>bbbbbbbbbb</select>
 
 <script>
 document.body.offsetHeight;
 var cv = window.getComputedStyle(document.querySelector('select')).lineHeight;
-if (cv == "normal" || parseInt(cv) > 50) {
+if (cv != "normal" && parseInt(cv) > 50) {
   document.body.appendChild(document.createTextNode(
-    "FAIL: got computed line-height '" + cv + "', expected a length <= 50px"));
+    "FAIL: got computed line-height '" + cv + "', " +
+    "expected 'normal' or a length <= 50px"));
 }</script>
 
 </body>
 </html>