REGRESSION (r176751): line-height ignored in <button> elements

https://bugs.webkit.org/show_bug.cgi?id=144234

Reviewed by Antti Koivisto.

Source/WebCore:

Test: fast/forms/button-line-height.html

* platform/Theme.h: Changed controlFont to return an Optional so we can tell
when the theme is overriding the font. Otherwise if the font from the user-agent
style sheet and the font from the theme are the same, we will think we are not
overriding the font when we actually are.

* platform/mac/ThemeMac.h: Updated controlFont to return Optional.
* platform/mac/ThemeMac.mm:
(WebCore::ThemeMac::controlFont): Ditto.

* rendering/RenderTheme.cpp:
(WebCore::RenderTheme::adjustStyle): Set line height only if the font is
overriden by the theme, all the time for PushButtonPart on Mac, and not at all
for other parts. Also tightened up the logic a little since RenderStyle's
setFontDescription already does an "==" comparison; we don't have to do
that twice.

LayoutTests:

* fast/forms/button-line-height-expected.html: Added.
* fast/forms/button-line-height.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@183366 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 655effb09048fcd160525a88c48c438f739b1846..5d31204d998d7b35d09ce82d64c344f205d8b328 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2015-04-26  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r176751): line-height ignored in <button> elements
+        https://bugs.webkit.org/show_bug.cgi?id=144234
+
+        Reviewed by Antti Koivisto.
+
+        * fast/forms/button-line-height-expected.html: Added.
+        * fast/forms/button-line-height.html: Added.
+
 2015-04-26  Darin Adler  <darin@apple.com>
 
         REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter

diff --git a/LayoutTests/fast/forms/button-line-height-expected.html b/LayoutTests/fast/forms/button-line-height-expected.html
new file mode 100644 (file)
index 0000000..5deade1
--- /dev/null
+++ b/LayoutTests/fast/forms/button-line-height-expected.html
@@ -0,0 +1,4 @@
+<button style="height:24px; margin:0">Button with line-height</button>
+<button style="height:24px; margin:0">Button with height</button>
+<button style="height:124px; margin:0">Button with line-height</button>
+<button style="height:124px; margin:0">Button with height</button>

diff --git a/LayoutTests/fast/forms/button-line-height.html b/LayoutTests/fast/forms/button-line-height.html
new file mode 100644 (file)
index 0000000..cd856eb
--- /dev/null
+++ b/LayoutTests/fast/forms/button-line-height.html
@@ -0,0 +1,4 @@
+<button style="line-height:19px; margin:0">Button with line-height</button>
+<button style="height:24px; margin:0">Button with height</button>
+<button style="line-height:119px; margin:0">Button with line-height</button>
+<button style="height:124px; margin:0">Button with height</button>

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 91f7cd202c42a82f82dda6c34f27b0586bb3865c..f302375c5c382ac8899fc7d2294e0b361e53eeb6 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+2015-04-26  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r176751): line-height ignored in <button> elements
+        https://bugs.webkit.org/show_bug.cgi?id=144234
+
+        Reviewed by Antti Koivisto.
+
+        Test: fast/forms/button-line-height.html
+
+        * platform/Theme.h: Changed controlFont to return an Optional so we can tell
+        when the theme is overriding the font. Otherwise if the font from the user-agent
+        style sheet and the font from the theme are the same, we will think we are not
+        overriding the font when we actually are.
+
+        * platform/mac/ThemeMac.h: Updated controlFont to return Optional.
+        * platform/mac/ThemeMac.mm:
+        (WebCore::ThemeMac::controlFont): Ditto.
+
+        * rendering/RenderTheme.cpp:
+        (WebCore::RenderTheme::adjustStyle): Set line height only if the font is
+        overriden by the theme, all the time for PushButtonPart on Mac, and not at all
+        for other parts. Also tightened up the logic a little since RenderStyle's
+        setFontDescription already does an "==" comparison; we don't have to do
+        that twice.
+
 2015-04-26  Darin Adler  <darin@apple.com>
 
         REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter

diff --git a/Source/WebCore/platform/Theme.h b/Source/WebCore/platform/Theme.h
index 88346965551a2ab0750d8b672d44348c99893193..ece84cf658aff615a5dd97259711e4ba34b6a52b 100644 (file)
--- a/Source/WebCore/platform/Theme.h
+++ b/Source/WebCore/platform/Theme.h
@@ -34,6 +34,7 @@
 #include "LengthSize.h"
 #include "ThemeTypes.h"
 #include <wtf/Forward.h>
+#include <wtf/Optional.h>
 
 namespace WebCore {
 
@@ -82,9 +83,9 @@ public:
     // Methods used to adjust the RenderStyles of controls.
     
     // The font description result should have a zoomed font size.
-    virtual FontDescription controlFont(ControlPart, const FontCascade& font, float /*zoomFactor*/) const { return font.fontDescription(); }
+    virtual Optional<FontDescription> controlFont(ControlPart, const FontCascade&, float /*zoomFactor*/) const { return Nullopt; }
     
-    // The size here is in zoomed coordinates already.  If a new size is returned, it also needs to be in zoomed coordinates.
+    // The size here is in zoomed coordinates already. If a new size is returned, it also needs to be in zoomed coordinates.
     virtual LengthSize controlSize(ControlPart, const FontCascade&, const LengthSize& zoomedSize, float /*zoomFactor*/) const { return zoomedSize; }
     
     // Returns the minimum size for a control in zoomed coordinates.  

diff --git a/Source/WebCore/platform/mac/ThemeMac.h b/Source/WebCore/platform/mac/ThemeMac.h
index b196e4c5df0a2fb7774417f9c7905d8d50925ebf..1dff6ba5a924b7f376dd831b8b2415a7112f251d 100644 (file)
--- a/Source/WebCore/platform/mac/ThemeMac.h
+++ b/Source/WebCore/platform/mac/ThemeMac.h
@@ -42,7 +42,7 @@ public:
     
     virtual int baselinePositionAdjustment(ControlPart) const;
 
-    virtual FontDescription controlFont(ControlPart, const FontCascade&, float zoomFactor) const;
+    virtual Optional<FontDescription> controlFont(ControlPart, const FontCascade&, float zoomFactor) const;
     
     virtual LengthSize controlSize(ControlPart, const FontCascade&, const LengthSize&, float zoomFactor) const;
     virtual LengthSize minimumControlSize(ControlPart, const FontCascade&, float zoomFactor) const;

diff --git a/Source/WebCore/platform/mac/ThemeMac.mm b/Source/WebCore/platform/mac/ThemeMac.mm
index 255d3702c8c60178150188221c0800875811ba2d..182468764838ebf91821127883a0fc6892d4e0b5 100644 (file)
--- a/Source/WebCore/platform/mac/ThemeMac.mm
+++ b/Source/WebCore/platform/mac/ThemeMac.mm
@@ -667,7 +667,7 @@ int ThemeMac::baselinePositionAdjustment(ControlPart part) const
     return Theme::baselinePositionAdjustment(part);
 }
 
-FontDescription ThemeMac::controlFont(ControlPart part, const FontCascade& font, float zoomFactor) const
+Optional<FontDescription> ThemeMac::controlFont(ControlPart part, const FontCascade& font, float zoomFactor) const
 {
     switch (part) {
         case PushButtonPart: {

diff --git a/Source/WebCore/rendering/RenderTheme.cpp b/Source/WebCore/rendering/RenderTheme.cpp
index 991e3577ebce00887e4ae392b41404328b66b850..b1a43aa7e73719b9efa0d942c800353c5fb51242 100644 (file)
--- a/Source/WebCore/rendering/RenderTheme.cpp
+++ b/Source/WebCore/rendering/RenderTheme.cpp
@@ -170,14 +170,12 @@ void RenderTheme::adjustStyle(StyleResolver& styleResolver, RenderStyle& style,
             style.setMinHeight(minControlSize.height());
                 
         // Font
-        FontDescription controlFont = m_theme->controlFont(part, style.fontCascade(), style.effectiveZoom());
-        if (controlFont != style.fontCascade().fontDescription()) {
-            // Now update our font.
-            if (style.setFontDescription(controlFont))
-                style.fontCascade().update(0);
+        if (auto themeFont = m_theme->controlFont(part, style.fontCascade(), style.effectiveZoom())) {
+            // If overriding the specified font with the theme font, also override the line height with the standard line height.
+            style.setLineHeight(RenderStyle::initialLineHeight());
+            if (style.setFontDescription(themeFont.value()))
+                style.fontCascade().update(nullptr);
         }
-        // Reset our line-height
-        style.setLineHeight(RenderStyle::initialLineHeight());
         style.setInsideDefaultButton(part == DefaultButtonPart);
     }
     break;