Update HTMLOListElement.start to behavior from latest HTML specification
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Oct 2017 00:11:50 +0000 (00:11 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Oct 2017 00:11:50 +0000 (00:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178057

Reviewed by Chris Dumez.

LayoutTests/imported/w3c:

* web-platform-tests/html/semantics/grouping-content/the-ol-element/grouping-ol-expected.txt:
* web-platform-tests/html/semantics/grouping-content/the-ol-element/ol.start-reflection-2-expected.txt:
Updated to expect more tests to pass.

Source/WebCore:

* html/HTMLOListElement.cpp:
(optionalValue): Added. Helper function that we can put into Expected.h later
if we like; makes it easier to turn Expected into std::optional.
(WebCore::HTMLOListElement::HTMLOListElement): Moved data member initialization
into class definition so it doesn't have to be done here.
(WebCore::HTMLOListElement::parseAttribute): Simplified using the new
optionalValue function. Moved the call to update values in here since it's
a trivial one-liner (albeit done twice).
(WebCore::HTMLOListElement::updateItemValues): Deleted. Moved this into the
parseAttribute function.
(WebCore::HTMLOListElement::itemCount): Updated to use std::optional instead
of a separate m_shouldRecalculateItemCount flag. Also inlined the
recalculateItemCount function since it's a trivial one-liner.
(WebCore::HTMLOListElement::itemCountAfterLayout): Deleted. The only use of
this was to implement the now-obsolete behavior of the start attribute.
(WebCore::HTMLOListElement::recalculateItemCount): Deleted. Moved this into
the itemCount function.

* html/HTMLOListElement.h: Changed startForBindings to return 1 when start
is not specified; this what the HTML specification now calls for. Updated
for the changes above. Merged m_itemCount and m_shouldRecalculateItemCount
into a single optional m_itemCount, and made it mutable so it can be
computed as a side effect of calling the const member function start.

LayoutTests:

* fast/lists/ol-reversed-simple-expected.txt:
* fast/lists/ol-reversed-simple.html:
* fast/lists/ol-reversed-simple.xhtml:
Updated test and results to expect the new behavior.

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

LayoutTests/ChangeLog
LayoutTests/fast/lists/ol-reversed-simple-expected.txt
LayoutTests/fast/lists/ol-reversed-simple.html
LayoutTests/fast/lists/ol-reversed-simple.xhtml
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/semantics/grouping-content/the-ol-element/grouping-ol-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/semantics/grouping-content/the-ol-element/ol.start-reflection-2-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLOListElement.cpp
Source/WebCore/html/HTMLOListElement.h

index dc3c656..625b608 100644 (file)
@@ -1,5 +1,17 @@
 2017-10-08  Darin Adler  <darin@apple.com>
 
+        Update HTMLOListElement.start to behavior from latest HTML specification
+        https://bugs.webkit.org/show_bug.cgi?id=178057
+
+        Reviewed by Chris Dumez.
+
+        * fast/lists/ol-reversed-simple-expected.txt:
+        * fast/lists/ol-reversed-simple.html:
+        * fast/lists/ol-reversed-simple.xhtml:
+        Updated test and results to expect the new behavior.
+
+2017-10-08  Darin Adler  <darin@apple.com>
+
         Fix bugs related to setting reflected floating point DOM attributes
         https://bugs.webkit.org/show_bug.cgi?id=178061
 
index b7f9e6e..08eafd2 100644 (file)
@@ -41,6 +41,6 @@ This tests that reversed lists with a negative start value render properly.
 -8 Minus Eight
 -9 Minus Nine
 
-This tests that reversed lists have the start attribute equals the number of list items when no start value is specified.
+This tests that reversed lists have a start attribute of 1 when no start value is specified.
 
-Value of start attribute of the list is : 5
+Value of start attribute of the list is : 1
index 070335c..393569a 100644 (file)
@@ -89,7 +89,7 @@
         </ol>
         <p id="console5"></p>
 
-        <p>This tests that reversed lists have the start attribute equals the number of list items when no start value is specified.</p>
+        <p>This tests that reversed lists have a start attribute of 1 when no start value is specified.</p>
         <ol id="list6" reversed>
             <li>Five</li>
             <li>Four</li>
index a942bbe..ab48ab0 100644 (file)
@@ -90,7 +90,7 @@
     </ol>
     <p id="console5"></p>
 
-    <p>This tests that reversed lists have the start attribute equals the number of list items when no start value is specified.</p>
+    <p>This tests that reversed lists have a start attribute of 1 when no start value is specified.</p>
     <ol id="list6" reversed="reversed">
         <li>Five</li>
         <li>Four</li>
index c2d2f75..1a040da 100644 (file)
@@ -1,3 +1,14 @@
+2017-10-07  Darin Adler  <darin@apple.com>
+
+        Update HTMLOListElement.start to behavior from latest HTML specification
+        https://bugs.webkit.org/show_bug.cgi?id=178057
+
+        Reviewed by Chris Dumez.
+
+        * web-platform-tests/html/semantics/grouping-content/the-ol-element/grouping-ol-expected.txt:
+        * web-platform-tests/html/semantics/grouping-content/the-ol-element/ol.start-reflection-2-expected.txt:
+        Updated to expect more tests to pass.
+
 2017-10-08  Darin Adler  <darin@apple.com>
 
         Fix bugs related to setting reflected floating point DOM attributes
index 2e02da7..9138fef 100644 (file)
@@ -16,11 +16,11 @@ PASS Changing IDL 'reversed' property changes list's reversed property.
 PASS Default start value for non-reversed list should be 1 
 PASS IDL and content attribute parse start of '.5' correctly. 
 PASS IDL and content attribute parse start of 'A' correctly. 
-FAIL Default start value (if none provided) for reversed list = 1. assert_equals: expected 1 but got 3
-FAIL Default start value (if failed to parse) for reversed list = 1. assert_equals: expected 1 but got 3
-FAIL Default start value for reversed list = 1 (even with tons of other child elements). assert_equals: expected 1 but got 3
-FAIL Adding child element to reversed list does not change start value assert_equals: expected 1 but got 4
-FAIL Deleting child element from reversed list does not change start value assert_equals: expected 1 but got 2
+PASS Default start value (if none provided) for reversed list = 1. 
+PASS Default start value (if failed to parse) for reversed list = 1. 
+PASS Default start value for reversed list = 1 (even with tons of other child elements). 
+PASS Adding child element to reversed list does not change start value 
+PASS Deleting child element from reversed list does not change start value 
 PASS IDL and content attribute parse start of '2' correctly. 
 PASS IDL and content attribute parse start of '-10' correctly. 
 PASS IDL and content attribute parse start of '4.03' correctly. 
index f99e3bc..50d3446 100644 (file)
@@ -1,5 +1,36 @@
 2017-10-08  Darin Adler  <darin@apple.com>
 
+        Update HTMLOListElement.start to behavior from latest HTML specification
+        https://bugs.webkit.org/show_bug.cgi?id=178057
+
+        Reviewed by Chris Dumez.
+
+        * html/HTMLOListElement.cpp:
+        (optionalValue): Added. Helper function that we can put into Expected.h later
+        if we like; makes it easier to turn Expected into std::optional.
+        (WebCore::HTMLOListElement::HTMLOListElement): Moved data member initialization
+        into class definition so it doesn't have to be done here.
+        (WebCore::HTMLOListElement::parseAttribute): Simplified using the new
+        optionalValue function. Moved the call to update values in here since it's
+        a trivial one-liner (albeit done twice).
+        (WebCore::HTMLOListElement::updateItemValues): Deleted. Moved this into the
+        parseAttribute function.
+        (WebCore::HTMLOListElement::itemCount): Updated to use std::optional instead
+        of a separate m_shouldRecalculateItemCount flag. Also inlined the
+        recalculateItemCount function since it's a trivial one-liner.
+        (WebCore::HTMLOListElement::itemCountAfterLayout): Deleted. The only use of
+        this was to implement the now-obsolete behavior of the start attribute.
+        (WebCore::HTMLOListElement::recalculateItemCount): Deleted. Moved this into
+        the itemCount function.
+
+        * html/HTMLOListElement.h: Changed startForBindings to return 1 when start
+        is not specified; this what the HTML specification now calls for. Updated
+        for the changes above. Merged m_itemCount and m_shouldRecalculateItemCount
+        into a single optional m_itemCount, and made it mutable so it can be
+        computed as a side effect of calling the const member function start.
+
+2017-10-08  Darin Adler  <darin@apple.com>
+
         Fix bugs related to setting reflected floating point DOM attributes
         https://bugs.webkit.org/show_bug.cgi?id=178061
 
index a4405fc..7c96e27 100644 (file)
 #include "HTMLParserIdioms.h"
 #include "RenderListItem.h"
 
+// FIXME: There should be a standard way to turn a std::expected into a std::optional.
+// Maybe we should put this into the header file for Expected and give it a better name.
+template<typename T, typename E> inline std::optional<T> optionalValue(Expected<T, E>&& expected)
+{
+    return expected ? std::optional<T>(WTFMove(expected.value())) : std::nullopt;
+}
+
 namespace WebCore {
 
 using namespace HTMLNames;
 
-HTMLOListElement::HTMLOListElement(const QualifiedName& tagName, Document& document)
+inline HTMLOListElement::HTMLOListElement(const QualifiedName& tagName, Document& document)
     : HTMLElement(tagName, document)
-    , m_itemCount(0)
-    , m_isReversed(false)
-    , m_shouldRecalculateItemCount(false)
 {
     ASSERT(hasTagName(olTag));
 }
@@ -80,17 +84,16 @@ void HTMLOListElement::parseAttribute(const QualifiedName& name, const AtomicStr
 {
     if (name == startAttr) {
         int oldStart = start();
-        auto optionalStart = parseHTMLInteger(value);
-        m_start = optionalStart ? std::optional<int>(optionalStart.value()) : std::nullopt;
+        m_start = optionalValue(parseHTMLInteger(value));
         if (oldStart == start())
             return;
-        updateItemValues();
+        RenderListItem::updateItemValuesForOrderedList(*this);
     } else if (name == reversedAttr) {
         bool reversed = !value.isNull();
         if (reversed == m_isReversed)
             return;
         m_isReversed = reversed;
-        updateItemValues();
+        RenderListItem::updateItemValuesForOrderedList(*this);
     } else
         HTMLElement::parseAttribute(name, value);
 }
@@ -100,29 +103,11 @@ void HTMLOListElement::setStartForBindings(int start)
     setIntegralAttribute(startAttr, start);
 }
 
-void HTMLOListElement::updateItemValues()
-{
-    RenderListItem::updateItemValuesForOrderedList(*this);
-}
-
 unsigned HTMLOListElement::itemCount() const
 {
-    if (m_shouldRecalculateItemCount)
-        const_cast<HTMLOListElement*>(this)->recalculateItemCount();
-    return m_itemCount;
-}
-
-unsigned HTMLOListElement::itemCountAfterLayout() const
-{
-    document().updateLayoutIgnorePendingStylesheets();
-
-    return itemCount();
-}
-
-void HTMLOListElement::recalculateItemCount()
-{
-    m_itemCount = RenderListItem::itemCountForOrderedList(*this);
-    m_shouldRecalculateItemCount = false;
+    if (!m_itemCount)
+        m_itemCount = RenderListItem::itemCountForOrderedList(*this);
+    return m_itemCount.value();
 }
 
 }
index c3fe246..9cc381c 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
- * Copyright (C) 2010 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -23,7 +23,6 @@
 #pragma once
 
 #include "HTMLElement.h"
-#include <wtf/Optional.h>
 
 namespace WebCore {
 
@@ -32,37 +31,29 @@ public:
     static Ref<HTMLOListElement> create(Document&);
     static Ref<HTMLOListElement> create(const QualifiedName&, Document&);
 
-    // FIXME: The reason we have this start() function which does not trigger layout is because it is called
-    // from rendering code and this is unfortunately one of the few cases where the render tree is mutated
-    // while in layout.
-    int start() const { return m_start ? m_start.value() : (m_isReversed ? itemCount() : 1); }
-    int startForBindings() const { return m_start ? m_start.value() : (m_isReversed ? itemCountAfterLayout() : 1); }
-
+    int startForBindings() const { return m_start.value_or(1); }
     WEBCORE_EXPORT void setStartForBindings(int);
 
-    bool isReversed() const { return m_isReversed; }
+    // FIXME: The reason start() does not trigger layout is because it is called
+    // from rendering code. This is unfortunately one of the few cases where the
+    // render tree is mutated during layout.
 
-    void itemCountChanged() { m_shouldRecalculateItemCount = true; }
+    int start() const { return m_start ? m_start.value() : (m_isReversed ? itemCount() : 1); }
+    bool isReversed() const { return m_isReversed; }
+    void itemCountChanged() { m_itemCount = std::nullopt; }
 
 private:
     HTMLOListElement(const QualifiedName&, Document&);
         
-    void updateItemValues();
-
-    WEBCORE_EXPORT unsigned itemCountAfterLayout() const;
     WEBCORE_EXPORT unsigned itemCount() const;
 
-    WEBCORE_EXPORT void recalculateItemCount();
-
     void parseAttribute(const QualifiedName&, const AtomicString&) final;
     bool isPresentationAttribute(const QualifiedName&) const final;
     void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) final;
 
     std::optional<int> m_start;
-    unsigned m_itemCount;
-
-    bool m_isReversed : 1;
-    bool m_shouldRecalculateItemCount : 1;
+    mutable std::optional<unsigned> m_itemCount;
+    bool m_isReversed { false };
 };
 
 } //namespace