Clean up line-height and minimumFontSize functions
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jul 2017 22:03:23 +0000 (22:03 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jul 2017 22:03:23 +0000 (22:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174535

Reviewed by Simon Fraser.

No behavior change.

No new tests because there is no behavior change.

* css/StyleBuilderConverter.h:
(WebCore::StyleBuilderConverter::convertLineHeight):
* css/StyleResolver.cpp:
(WebCore::StyleResolver::styleForKeyframe):
(WebCore::StyleResolver::adjustRenderStyle):
(WebCore::StyleResolver::pseudoStyleRulesForElement):
(WebCore::StyleResolver::applyMatchedProperties):
(WebCore::StyleResolver::cascadedPropertiesForRollback):
(WebCore::StyleResolver::applyProperty):
(WebCore::StyleResolver::checkForZoomChange):
(WebCore::StyleResolver::createFilterOperations):
(WebCore::StyleResolver::CascadedProperties::set):
(WebCore::StyleResolver::applyCascadedProperties):
* style/StyleFontSizeFunctions.cpp:
(WebCore::Style::computedFontSizeFromSpecifiedSize):
(WebCore::Style::computedFontSizeFromSpecifiedSizeForSVGInlineText):
(): Deleted.

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

Source/WebCore/ChangeLog
Source/WebCore/css/StyleBuilderConverter.h
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/style/StyleFontSizeFunctions.cpp

index b4eab05..4037fcb 100644 (file)
@@ -1,3 +1,32 @@
+2017-07-15  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Clean up line-height and minimumFontSize functions
+        https://bugs.webkit.org/show_bug.cgi?id=174535
+
+        Reviewed by Simon Fraser.
+
+        No behavior change.
+
+        No new tests because there is no behavior change.
+
+        * css/StyleBuilderConverter.h:
+        (WebCore::StyleBuilderConverter::convertLineHeight):
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::styleForKeyframe):
+        (WebCore::StyleResolver::adjustRenderStyle):
+        (WebCore::StyleResolver::pseudoStyleRulesForElement):
+        (WebCore::StyleResolver::applyMatchedProperties):
+        (WebCore::StyleResolver::cascadedPropertiesForRollback):
+        (WebCore::StyleResolver::applyProperty):
+        (WebCore::StyleResolver::checkForZoomChange):
+        (WebCore::StyleResolver::createFilterOperations):
+        (WebCore::StyleResolver::CascadedProperties::set):
+        (WebCore::StyleResolver::applyCascadedProperties):
+        * style/StyleFontSizeFunctions.cpp:
+        (WebCore::Style::computedFontSizeFromSpecifiedSize):
+        (WebCore::Style::computedFontSizeFromSpecifiedSizeForSVGInlineText):
+        (): Deleted.
+
 2017-07-14  Jonathan Bedard  <jbedard@apple.com>
 
         Add iOS 11 SPI
index 7c2005f..b9bef03 100644 (file)
@@ -1434,14 +1434,22 @@ inline std::optional<Length> StyleBuilderConverter::convertLineHeight(StyleResol
             length = Length(length.value() * multiplier, Fixed);
         return length;
     }
+
+    // Line-height percentages need to inherit as if they were Fixed pixel values. In the example:
+    // <div style="font-size: 10px; line-height: 150%;"><div style="font-size: 100px;"></div></div>
+    // the inner element should have line-height of 15px. However, in this example:
+    // <div style="font-size: 10px; line-height: 1.5;"><div style="font-size: 100px;"></div></div>
+    // the inner element should have a line-height of 150px. Therefore, we map percentages to Fixed
+    // values and raw numbers to percentages.
     if (primitiveValue.isPercentage()) {
         // FIXME: percentage should not be restricted to an integer here.
         return Length((styleResolver.style()->computedFontSize() * primitiveValue.intValue()) / 100, Fixed);
     }
-    if (primitiveValue.isNumber()) {
-        // FIXME: number and percentage values should produce the same type of Length (ie. Fixed or Percent).
+    if (primitiveValue.isNumber())
         return Length(primitiveValue.doubleValue() * multiplier * 100.0, Percent);
-    }
+
+    // FIXME: The parser should only emit the above types, so this should never be reached. We should change the
+    // type of this function to return just a Length (and not an Optional).
     return std::nullopt;
 }
 
index fa83852..5277935 100644 (file)
@@ -425,7 +425,7 @@ std::unique_ptr<RenderStyle> StyleResolver::styleForKeyframe(const RenderStyle*
     // decl, there's nothing to override. So just add the first properties.
     CascadedProperties cascade(direction, writingMode);
     cascade.addNormalMatches(result, 0, result.matchedProperties().size() - 1);
-    
+
     // Resolve custom properties first.
     applyCascadedProperties(cascade, CSSPropertyCustom, CSSPropertyCustom, &result);
 
@@ -1048,7 +1048,7 @@ void StyleResolver::adjustRenderStyle(RenderStyle& style, const RenderStyle& par
         if ((element->hasTagName(SVGNames::foreignObjectTag) || element->hasTagName(SVGNames::textTag)) && style.isDisplayInlineType())
             style.setDisplay(BLOCK);
     }
-    
+
     // If the inherited value of justify-items includes the 'legacy' keyword,
     // 'auto' computes to the the inherited value. Otherwise, 'auto' computes to
     // 'normal'.
@@ -1057,7 +1057,7 @@ void StyleResolver::adjustRenderStyle(RenderStyle& style, const RenderStyle& par
             style.setJustifyItems(parentBoxStyle->justifyItems());
     }
 }
-    
+
 bool StyleResolver::checkRegionStyle(const Element* regionElement)
 {
     unsigned rulesSize = m_ruleSets.authorStyle().regionSelectorsAndRuleSets().size();
@@ -1133,7 +1133,7 @@ Vector<RefPtr<StyleRule>> StyleResolver::pseudoStyleRulesForElement(const Elemen
     if (rulesToInclude & UAAndUserCSSRules) {
         // First we match rules from the user agent sheet.
         collector.matchUARules();
-        
+
         // Now we check user sheet rules.
         if (m_matchAuthorAndUserStyles)
             collector.matchUserRules(rulesToInclude & EmptyCSSRules);
@@ -1375,13 +1375,13 @@ void StyleResolver::applyMatchedProperties(const MatchResult& matchResult, const
 
         applyCascadedProperties(cascade, CSSPropertyWebkitRubyPosition, CSSPropertyWebkitRubyPosition, &matchResult);
         adjustStyleForInterCharacterRuby();
-    
+
         // Resolve custom variables first.
         applyCascadedProperties(cascade, CSSPropertyCustom, CSSPropertyCustom, &matchResult);
 
         // Start by applying properties that other properties may depend on.
         applyCascadedProperties(cascade, firstCSSProperty, lastHighPriorityProperty, &matchResult);
-    
+
         updateFont();
         applyCascadedProperties(cascade, firstLowPriorityProperty, lastCSSProperty, &matchResult);
 
@@ -1393,12 +1393,12 @@ void StyleResolver::applyMatchedProperties(const MatchResult& matchResult, const
     cascade.addImportantMatches(matchResult, matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule, applyInheritedOnly);
     cascade.addImportantMatches(matchResult, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, applyInheritedOnly);
     cascade.addImportantMatches(matchResult, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly);
-    
+
     // Resolve custom properties first.
     applyCascadedProperties(cascade, CSSPropertyCustom, CSSPropertyCustom, &matchResult);
 
     applyCascadedProperties(cascade, CSSPropertyWebkitRubyPosition, CSSPropertyWebkitRubyPosition, &matchResult);
-    
+
     // Adjust the font size to be smaller if ruby-position is inter-character.
     adjustStyleForInterCharacterRuby();
 
@@ -1425,7 +1425,7 @@ void StyleResolver::applyMatchedProperties(const MatchResult& matchResult, const
     cascade.applyDeferredProperties(*this, &matchResult);
 
     ASSERT(!state.fontDirty());
-    
+
     if (cacheItem || !cacheHash)
         return;
     if (!isCacheableInMatchedPropertiesCache(*state.element(), state.style(), state.parentStyle()))
@@ -1556,7 +1556,7 @@ bool StyleResolver::useSVGZoomRulesForLength()
 StyleResolver::CascadedProperties* StyleResolver::cascadedPropertiesForRollback(const MatchResult& matchResult)
 {
     ASSERT(cascadeLevel() != UserAgentLevel);
-    
+
     TextDirection direction;
     WritingMode writingMode;
     extractDirectionAndWritingMode(*state().style(), matchResult, direction, writingMode);
@@ -1565,34 +1565,34 @@ StyleResolver::CascadedProperties* StyleResolver::cascadedPropertiesForRollback(
         CascadedProperties* authorRollback = state().authorRollback();
         if (authorRollback)
             return authorRollback;
-        
+
         auto newAuthorRollback(std::make_unique<CascadedProperties>(direction, writingMode));
-        
+
         // This special rollback cascade contains UA rules and user rules but no author rules.
         newAuthorRollback->addNormalMatches(matchResult, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, false);
         newAuthorRollback->addNormalMatches(matchResult, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, false);
         newAuthorRollback->addImportantMatches(matchResult, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, false);
         newAuthorRollback->addImportantMatches(matchResult, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, false);
-    
+
         state().setAuthorRollback(newAuthorRollback);
         return state().authorRollback();
     }
-    
+
     if (cascadeLevel() == UserLevel) {
         CascadedProperties* userRollback = state().userRollback();
         if (userRollback)
             return userRollback;
-        
+
         auto newUserRollback(std::make_unique<CascadedProperties>(direction, writingMode));
-        
+
         // This special rollback cascade contains only UA rules.
         newUserRollback->addNormalMatches(matchResult, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, false);
         newUserRollback->addImportantMatches(matchResult, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, false);
-    
+
         state().setUserRollback(newUserRollback);
         return state().userRollback();
     }
-    
+
     return nullptr;
 }
 
@@ -1601,7 +1601,7 @@ void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value, SelectorChe
     ASSERT_WITH_MESSAGE(!isShorthandCSSProperty(id), "Shorthand property id = %d wasn't expanded at parsing time", id);
 
     State& state = m_state;
-    
+
     RefPtr<CSSValue> valueToApply = value;
     if (value->hasVariableReferences()) {
         valueToApply = resolvedVariableValue(id, *value);
@@ -1618,11 +1618,11 @@ void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value, SelectorChe
         ASSERT(newId != id);
         return applyProperty(newId, valueToApply.get(), linkMatchMask, matchResult);
     }
-    
+
     CSSValue* valueToCheckForInheritInitial = valueToApply.get();
     CSSCustomPropertyValue* customPropertyValue = nullptr;
     CSSValueID customPropertyValueID = CSSValueInvalid;
-    
+
     if (id == CSSPropertyCustom) {
         customPropertyValue = &downcast<CSSCustomPropertyValue>(*valueToApply);
         customPropertyValueID = customPropertyValue->valueID();
@@ -1630,7 +1630,7 @@ void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value, SelectorChe
 
     bool isInherit = state.parentStyle() ? valueToCheckForInheritInitial->isInheritedValue() || customPropertyValueID == CSSValueInherit : false;
     bool isInitial = valueToCheckForInheritInitial->isInitialValue() || customPropertyValueID == CSSValueInitial || (!state.parentStyle() && (valueToCheckForInheritInitial->isInheritedValue() || customPropertyValueID == CSSValueInherit));
-    
+
     bool isUnset = valueToCheckForInheritInitial->isUnsetValue() || customPropertyValueID == CSSValueUnset;
     bool isRevert = valueToCheckForInheritInitial->isRevertValue() || customPropertyValueID == CSSValueRevert;
 
@@ -1659,11 +1659,11 @@ void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value, SelectorChe
                     applyProperty(property.id, property.cssValue[linkMatchMask], linkMatchMask, matchResult);
                 return;
             }
-        
+
             isUnset = true;
         }
     }
-    
+
     if (isUnset) {
         if (CSSProperty::isInheritedProperty(id))
             isInherit = true;
@@ -1680,7 +1680,7 @@ void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value, SelectorChe
 
     if (isInherit && !CSSProperty::isInheritedProperty(id))
         state.style()->setHasExplicitlyInheritedProperties();
-    
+
     if (customPropertyValue) {
         auto& name = customPropertyValue->name();
         auto* value = isInitial ? nullptr : isInherit ? state.parentStyle()->customProperties().get(name) : customPropertyValue;
@@ -1736,7 +1736,7 @@ void StyleResolver::checkForZoomChange(RenderStyle* style, const RenderStyle* pa
 {
     if (!parentStyle)
         return;
-    
+
     if (style->effectiveZoom() == parentStyle->effectiveZoom() && style->textZoom() == parentStyle->textZoom())
         return;
 
@@ -1899,13 +1899,13 @@ bool StyleResolver::createFilterOperations(const CSSValue& inValue, FilterOperat
 {
     State& state = m_state;
     ASSERT(outOperations.isEmpty());
-    
+
     if (is<CSSPrimitiveValue>(inValue)) {
         auto& primitiveValue = downcast<CSSPrimitiveValue>(inValue);
         if (primitiveValue.valueID() == CSSValueNone)
             return true;
     }
-    
+
     if (!is<CSSValueList>(inValue))
         return false;
 
@@ -2099,7 +2099,7 @@ void StyleResolver::CascadedProperties::set(CSSPropertyID id, CSSValue& cssValue
         }
         return;
     }
-    
+
     if (!m_propertyIsPresent[id])
         memset(property.cssValue, 0, sizeof(property.cssValue));
     m_propertyIsPresent.set(id);
@@ -2269,7 +2269,7 @@ void StyleResolver::applyCascadedProperties(CascadedProperties& cascade, int fir
         ASSERT(!shouldApplyPropertyInParseOrder(propertyID));
         property.apply(*this, matchResult);
     }
-    
+
     if (firstProperty == CSSPropertyCustom)
         m_state.style()->checkVariablesInCustomProperties();
 }
index c92cc4c..9fc6000 100644 (file)
@@ -39,10 +39,10 @@ namespace WebCore {
 
 namespace Style {
 
-enum MinimumFontSizeRule {
-    DoNotApplyMinimumFontSize,
-    DoNotUseSmartMinimumForFontSize,
-    UseSmartMinimumForFontFize
+enum class MinimumFontSizeRule {
+    None,
+    Absolute,
+    AbsoluteAndRelative
 };
 
 static float computedFontSizeFromSpecifiedSize(float specifiedSize, bool isAbsoluteSize, float zoomFactor, MinimumFontSizeRule minimumSizeRule, const Settings& settings)
@@ -64,7 +64,7 @@ static float computedFontSizeFromSpecifiedSize(float specifiedSize, bool isAbsol
     // However we always allow the page to set an explicit pixel size that is smaller,
     // since sites will mis-render otherwise (e.g., http://www.gamespot.com with a 9px minimum).
 
-    if (minimumSizeRule == DoNotApplyMinimumFontSize)
+    if (minimumSizeRule == MinimumFontSizeRule::None)
         return specifiedSize;
 
     int minSize = settings.minimumFontSize();
@@ -72,18 +72,17 @@ static float computedFontSizeFromSpecifiedSize(float specifiedSize, bool isAbsol
     float zoomedSize = specifiedSize * zoomFactor;
 
     // Apply the hard minimum first. We only apply the hard minimum if after zooming we're still too small.
-    if (zoomedSize < minSize)
-        zoomedSize = minSize;
+    zoomedSize = std::max(zoomedSize, static_cast<float>(minSize));
 
-    // Now apply the "smart minimum." This minimum is also only applied if we're still too small
+    // Now apply the smart minimum. This minimum is also only applied if we're still too small
     // after zooming. The font size must either be relative to the user default or the original size
     // must have been acceptable. In other words, we only apply the smart minimum whenever we're positive
     // doing so won't disrupt the layout.
-    if (minimumSizeRule ==  UseSmartMinimumForFontFize && zoomedSize < minLogicalSize && (specifiedSize >= minLogicalSize || !isAbsoluteSize))
-        zoomedSize = minLogicalSize;
+    if (minimumSizeRule == MinimumFontSizeRule::AbsoluteAndRelative && (specifiedSize >= minLogicalSize || !isAbsoluteSize))
+        zoomedSize = std::max(zoomedSize, static_cast<float>(minLogicalSize));
 
     // Also clamp to a reasonable maximum to prevent insane font sizes from causing crashes on various
-    // platforms (I'm looking at you, Windows.)
+    // platforms. (I'm looking at you, Windows.)
     return std::min(maximumAllowedFontSize, zoomedSize);
 }
 
@@ -96,12 +95,12 @@ float computedFontSizeFromSpecifiedSize(float specifiedSize, bool isAbsoluteSize
         if (frame && style->textZoom() != TextZoomReset)
             zoomFactor *= frame->textZoomFactor();
     }
-    return computedFontSizeFromSpecifiedSize(specifiedSize, isAbsoluteSize, zoomFactor, useSVGZoomRules ? DoNotApplyMinimumFontSize : UseSmartMinimumForFontFize, document.settings());
+    return computedFontSizeFromSpecifiedSize(specifiedSize, isAbsoluteSize, zoomFactor, useSVGZoomRules ? MinimumFontSizeRule::None : MinimumFontSizeRule::AbsoluteAndRelative, document.settings());
 }
 
 float computedFontSizeFromSpecifiedSizeForSVGInlineText(float specifiedSize, bool isAbsoluteSize, float zoomFactor, const Document& document)
 {
-    return computedFontSizeFromSpecifiedSize(specifiedSize, isAbsoluteSize, zoomFactor, DoNotUseSmartMinimumForFontSize, document.settings());
+    return computedFontSizeFromSpecifiedSize(specifiedSize, isAbsoluteSize, zoomFactor, MinimumFontSizeRule::Absolute, document.settings());
 }
 
 const int fontSizeTableMax = 16;