Bug 1536871 - Make 'line-height: normal' return the 'normal' keyword from getComputedStyle() on Nightly and Early Beta, for now. r=dholbert
☠☠ backed out by bc71dba721d7 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 13 Jun 2019 18:30:07 +0000
changeset 478752 64b2f83062174be63c9ef05d9e2b03b6bfda9344
parent 478751 3876241cc1ecfbc452fdcc3b05c9df44eb8f579e
child 478753 8746baa7c837e7a1f40779a56ddde2f514a903b0
push id87876
push userealvarez@mozilla.com
push dateThu, 13 Jun 2019 18:30:41 +0000
treeherderautoland@64b2f8306217 [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
@@ -2094,18 +2094,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
@@ -5244,16 +5244,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>