- Revision
- 200265
- Author
- [email protected]
- Date
- 2016-04-29 14:48:43 -0700 (Fri, 29 Apr 2016)
Log Message
<select multiple> padding should react when scrolling
Source/WebCore:
https://bugs.webkit.org/show_bug.cgi?id=156590
https://bugs.webkit.org/show_bug.cgi?id=156591
Reviewed by Reviewed by Darin Adler.
Tests: fast/forms/listbox-respects-padding-bottom.html
fast/forms/listbox-top-padding-do-not-clip-items.html
Non-dropdown listboxes have support to padding-{top,bottom} implemented similarly
to the border model: the padding area does not move when the listbox' content gets scrolled,
but instead it clips out its content.
This is not consistent with other browsers and is not consistent with the CSS box model.
This in practice, if a <select> has padding-top set, the padding-top area will clip out listbox'
content as one scrolls upwards.
It also means that if padding-bottom is set, when one scrolls all the way to the bottom
of the listbox content, padding-bottom is not respected.
In order to fix these two problems, and make WebKit match Blink with respect to the the way
padding-{top,bottom} are handled, patch adds two class member variables that control the number
of list items (i.e. <option>s) that can be painted over the current listbox' padding area.
In short, depending on the scroll position and the amount of space available in the padding top/bottom
areas, items are painted or not on top of it, mimic'ing the CSS box model behavior of other browsers.
Note that this is specific solution is worth it to pursue on the short/mid term, but a long-term solution
to this problem and many other listbox discrepancies on WebKit's implementation, would be to reimplement
RenderListBox class in terms of RenderLayer. This will be a follow up work.
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::updateFromElement):
(WebCore::RenderListBox::numVisibleItems):
(WebCore::RenderListBox::paintObject):
(WebCore::RenderListBox::scrollToRevealElementAtListIndex):
(WebCore::RenderListBox::listIndexIsVisible):
(WebCore::RenderListBox::maximumNumberOfItemsThatFitInPaddingBottomArea):
(WebCore::RenderListBox::numberOfVisibleItemsInPaddingTop):
(WebCore::RenderListBox::numberOfVisibleItemsInPaddingBottom):
(WebCore::RenderListBox::computeFirstIndexesVisibleInPaddingTopBottomAreas):
(WebCore::RenderListBox::scrollTo):
* rendering/RenderListBox.h:
LayoutTests:
https://bugs.webkit.org/show_bug.cgi?id=156590
https://bugs.webkit.org/show_bug.cgi?id=156591
Reviewed by Reviewed by Darin Adler.
* fast/forms/listbox-respects-padding-bottom-expected.txt: Added.
* fast/forms/listbox-respects-padding-bottom.html: Added.
* fast/forms/listbox-top-padding-do-not-clip-items-expected.txt: Added.
* fast/forms/listbox-top-padding-do-not-clip-items.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (200264 => 200265)
--- trunk/LayoutTests/ChangeLog 2016-04-29 21:29:01 UTC (rev 200264)
+++ trunk/LayoutTests/ChangeLog 2016-04-29 21:48:43 UTC (rev 200265)
@@ -1,3 +1,16 @@
+2016-04-29 Antonio Gomes <[email protected]>
+
+ <select multiple> padding should react when scrolling
+ https://bugs.webkit.org/show_bug.cgi?id=156590
+ https://bugs.webkit.org/show_bug.cgi?id=156591
+
+ Reviewed by Reviewed by Darin Adler.
+
+ * fast/forms/listbox-respects-padding-bottom-expected.txt: Added.
+ * fast/forms/listbox-respects-padding-bottom.html: Added.
+ * fast/forms/listbox-top-padding-do-not-clip-items-expected.txt: Added.
+ * fast/forms/listbox-top-padding-do-not-clip-items.html: Added.
+
2016-04-29 Eric Carlson <[email protected]>
[Mac] AirPlay fails if target is set before AVPlayer has been created
Added: trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt (0 => 200265)
--- trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom-expected.txt 2016-04-29 21:48:43 UTC (rev 200265)
@@ -0,0 +1,10 @@
+Tests that padding-bottom is respected in non-dropdown listbox'es. Once a listbox is scrolled to its end, clicking on the padding-bottom area should hit an item only after the listbox is scrolled upwards some steps.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS i > 5 && i < maxAttempts is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom.html (0 => 200265)
--- trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom.html (rev 0)
+++ trunk/LayoutTests/fast/forms/listbox-respects-padding-bottom.html 2016-04-29 21:48:43 UTC (rev 200265)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+ <script src=""
+ <script>
+ description("Tests that padding-bottom is respected in non-dropdown listbox'es. Once a listbox is scrolled to its end, clicking on the padding-bottom area should hit an item only after the listbox is scrolled upwards some steps.");
+ window.jsTestIsAsync = true;
+
+ var i = 0;
+ var maxAttempts = 10;
+ function runTest()
+ {
+ var scrollAmount = 0;
+ var select = document.getElementById("sl");
+ select.scrollTop = select.scrollHeight;
+ var maxScrollOffset = select.scrollTop;
+
+ var x = select.offsetLeft + (select.offsetLeft + select.offsetWidth) / 2;
+ var y = select.offsetTop + select.offsetHeight - 15;
+
+ for ( ; i < maxAttempts; i++) {
+ var el = document.elementFromPoint(x, y);
+ if (el instanceof HTMLOptionElement)
+ break;
+
+ scrollAmount += 10;
+ select.scrollTop = maxScrollOffset - scrollAmount;
+ }
+
+ shouldBeTrue("i > 5 && i < maxAttempts");
+ finishJSTest();
+ }
+ </script>
+ <body _onload_="runTest()">
+ <select id="sl" multiple="multiple" style="padding-bottom: 100px; font-size: 15px;">
+ <option>January (0)</option>
+ <option>February (1)</option>
+ <option>March (2)</option>
+ <option>April (3)</option>
+ <option>May (4)</option>
+ <option>June (5) </option>
+ <option>July (6)</option>
+ <option>August (7)</option>
+ <option>September (8)</option>
+ <option>October (9)</option>
+ <option>November (10)</option>
+ <option>December (11)</option>
+ </select>
+ </body>
+ <script src=""
+</html>
Added: trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items-expected.txt (0 => 200265)
--- trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items-expected.txt 2016-04-29 21:48:43 UTC (rev 200265)
@@ -0,0 +1,10 @@
+Tests that padding-top does not clip out listbox' items once it scrolled upwards.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS i > 0 && i < maxAttempts is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items.html (0 => 200265)
--- trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items.html (rev 0)
+++ trunk/LayoutTests/fast/forms/listbox-top-padding-do-not-clip-items.html 2016-04-29 21:48:43 UTC (rev 200265)
@@ -0,0 +1,44 @@
+<!DOCTYPE html>
+<html>
+ <script src=""
+ <script>
+ description("Tests that padding-top does not clip out listbox' items once it scrolled upwards.");
+ window.jsTestIsAsync = true;
+
+ var i = 0;
+ var maxAttempts = 10;
+ function runTest()
+ {
+ var scrollAmount = 0;
+ for ( ; i < maxAttempts; i++) {
+ var select = document.getElementById("sl");
+ var x = select.offsetLeft + (select.offsetLeft + select.offsetWidth) / 2;
+ var y = select.offsetTop + 15;
+ var el = document.elementFromPoint(x, y);
+ if (el instanceof HTMLOptionElement)
+ break;
+ scrollAmount += 10;
+ select.scrollTop = scrollAmount;
+ }
+ shouldBeTrue("i > 0 && i < maxAttempts");
+ finishJSTest();
+ }
+ </script>
+ <body _onload_="runTest()">
+ <select id="sl" multiple="multiple" style="padding: 50px; font-size: 15px;">
+ <option>January (0)</option>
+ <option>February (1)</option>
+ <option>March (2)</option>
+ <option>April (3)</option>
+ <option>May (4)</option>
+ <option>June (5) </option>
+ <option>July (6)</option>
+ <option>August (7)</option>
+ <option>September (8)</option>
+ <option>October (9)</option>
+ <option>November (10)</option>
+ <option>December (11)</option>
+ </select>
+ </body>
+ <script src=""
+</html>
Modified: trunk/LayoutTests/platform/ios-simulator/TestExpectations (200264 => 200265)
--- trunk/LayoutTests/platform/ios-simulator/TestExpectations 2016-04-29 21:29:01 UTC (rev 200264)
+++ trunk/LayoutTests/platform/ios-simulator/TestExpectations 2016-04-29 21:48:43 UTC (rev 200265)
@@ -3001,6 +3001,8 @@
fast/forms/listbox-padding-clip.html [ ImageOnlyFailure ]
fast/forms/listbox-padding-clip-overlay.html [ ImageOnlyFailure ]
fast/forms/listbox-padding-clip-selected.html [ ImageOnlyFailure ]
+fast/forms/listbox-respects-padding-bottom.html [ ImageOnlyFailure ]
+fast/forms/listbox-top-padding-do-not-clip-items.html [ ImageOnlyFailure ]
# MSE not enabled.
media/media-source/media-source-airplay.html [ Skip ]
Modified: trunk/Source/WebCore/ChangeLog (200264 => 200265)
--- trunk/Source/WebCore/ChangeLog 2016-04-29 21:29:01 UTC (rev 200264)
+++ trunk/Source/WebCore/ChangeLog 2016-04-29 21:48:43 UTC (rev 200265)
@@ -1,3 +1,49 @@
+2016-04-29 Antonio Gomes <[email protected]>
+
+ <select multiple> padding should react when scrolling
+
+ https://bugs.webkit.org/show_bug.cgi?id=156590
+ https://bugs.webkit.org/show_bug.cgi?id=156591
+
+ Reviewed by Reviewed by Darin Adler.
+
+ Tests: fast/forms/listbox-respects-padding-bottom.html
+ fast/forms/listbox-top-padding-do-not-clip-items.html
+
+ Non-dropdown listboxes have support to padding-{top,bottom} implemented similarly
+ to the border model: the padding area does not move when the listbox' content gets scrolled,
+ but instead it clips out its content.
+ This is not consistent with other browsers and is not consistent with the CSS box model.
+
+ This in practice, if a <select> has padding-top set, the padding-top area will clip out listbox'
+ content as one scrolls upwards.
+ It also means that if padding-bottom is set, when one scrolls all the way to the bottom
+ of the listbox content, padding-bottom is not respected.
+
+ In order to fix these two problems, and make WebKit match Blink with respect to the the way
+ padding-{top,bottom} are handled, patch adds two class member variables that control the number
+ of list items (i.e. <option>s) that can be painted over the current listbox' padding area.
+
+ In short, depending on the scroll position and the amount of space available in the padding top/bottom
+ areas, items are painted or not on top of it, mimic'ing the CSS box model behavior of other browsers.
+
+ Note that this is specific solution is worth it to pursue on the short/mid term, but a long-term solution
+ to this problem and many other listbox discrepancies on WebKit's implementation, would be to reimplement
+ RenderListBox class in terms of RenderLayer. This will be a follow up work.
+
+ * rendering/RenderListBox.cpp:
+ (WebCore::RenderListBox::updateFromElement):
+ (WebCore::RenderListBox::numVisibleItems):
+ (WebCore::RenderListBox::paintObject):
+ (WebCore::RenderListBox::scrollToRevealElementAtListIndex):
+ (WebCore::RenderListBox::listIndexIsVisible):
+ (WebCore::RenderListBox::maximumNumberOfItemsThatFitInPaddingBottomArea):
+ (WebCore::RenderListBox::numberOfVisibleItemsInPaddingTop):
+ (WebCore::RenderListBox::numberOfVisibleItemsInPaddingBottom):
+ (WebCore::RenderListBox::computeFirstIndexesVisibleInPaddingTopBottomAreas):
+ (WebCore::RenderListBox::scrollTo):
+ * rendering/RenderListBox.h:
+
2016-04-29 Eric Carlson <[email protected]>
[Mac] AirPlay fails if target is set before AVPlayer has been created
Modified: trunk/Source/WebCore/rendering/RenderListBox.cpp (200264 => 200265)
--- trunk/Source/WebCore/rendering/RenderListBox.cpp 2016-04-29 21:29:01 UTC (rev 200264)
+++ trunk/Source/WebCore/rendering/RenderListBox.cpp 2016-04-29 21:48:43 UTC (rev 200265)
@@ -139,6 +139,8 @@
setHasVerticalScrollbar(true);
+ computeFirstIndexesVisibleInPaddingTopBottomAreas();
+
setNeedsLayoutAndPrefWidthsRecalc();
}
}
@@ -236,10 +238,14 @@
return defaultSize;
}
-int RenderListBox::numVisibleItems() const
+int RenderListBox::numVisibleItems(ConsiderPadding considerPadding) const
{
// Only count fully visible rows. But don't return 0 even if only part of a row shows.
- return std::max<int>(1, (contentHeight() + paddingBottom() + rowSpacing) / itemHeight());
+ int visibleItemsExcludingPadding = std::max<int>(1, (contentHeight() + rowSpacing) / itemHeight());
+ if (considerPadding == ConsiderPadding::No)
+ return visibleItemsExcludingPadding;
+
+ return numberOfVisibleItemsInPaddingTop() + visibleItemsExcludingPadding + numberOfVisibleItemsInPaddingBottom();
}
int RenderListBox::numItems() const
@@ -275,8 +281,9 @@
void RenderListBox::paintItem(PaintInfo& paintInfo, const LayoutPoint& paintOffset, PaintFunction paintFunction)
{
int listItemsSize = numItems();
- int endIndex = m_indexOffset + numVisibleItems();
- for (int i = m_indexOffset; i < listItemsSize && i <= endIndex; ++i)
+ int firstVisibleItem = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset);
+ int endIndex = firstVisibleItem + numVisibleItems(ConsiderPadding::Yes);
+ for (int i = firstVisibleItem; i < listItemsSize && i < endIndex; ++i)
paintFunction(paintInfo, paintOffset, i);
}
@@ -589,8 +596,13 @@
}
bool RenderListBox::listIndexIsVisible(int index)
-{
- return index >= m_indexOffset && index < m_indexOffset + numVisibleItems();
+{
+ int firstIndex = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset);
+ int endIndex = m_indexOfFirstVisibleItemInsidePaddingBottomArea
+ ? m_indexOfFirstVisibleItemInsidePaddingBottomArea.value() + numberOfVisibleItemsInPaddingBottom()
+ : m_indexOffset + numVisibleItems();
+
+ return index >= firstIndex && index < endIndex;
}
bool RenderListBox::scroll(ScrollDirection direction, ScrollGranularity granularity, float multiplier, Element**, RenderBox*, const IntPoint&)
@@ -634,12 +646,53 @@
scrollTo(offset.y());
}
+int RenderListBox::maximumNumberOfItemsThatFitInPaddingBottomArea() const
+{
+ return paddingBottom() / itemHeight();
+}
+
+int RenderListBox::numberOfVisibleItemsInPaddingTop() const
+{
+ if (!m_indexOfFirstVisibleItemInsidePaddingTopArea)
+ return 0;
+
+ return m_indexOffset - m_indexOfFirstVisibleItemInsidePaddingTopArea.value();
+}
+
+int RenderListBox::numberOfVisibleItemsInPaddingBottom() const
+{
+ if (!m_indexOfFirstVisibleItemInsidePaddingBottomArea)
+ return 0;
+
+ return std::min(maximumNumberOfItemsThatFitInPaddingBottomArea(), numItems() - m_indexOffset - numVisibleItems());
+}
+
+void RenderListBox::computeFirstIndexesVisibleInPaddingTopBottomAreas()
+{
+ m_indexOfFirstVisibleItemInsidePaddingTopArea = Nullopt;
+ m_indexOfFirstVisibleItemInsidePaddingBottomArea = Nullopt;
+
+ int maximumNumberOfItemsThatFitInPaddingTopArea = paddingTop() / itemHeight();
+ if (maximumNumberOfItemsThatFitInPaddingTopArea) {
+ if (m_indexOffset)
+ m_indexOfFirstVisibleItemInsidePaddingTopArea = std::max(0, m_indexOffset - maximumNumberOfItemsThatFitInPaddingTopArea);
+ }
+
+ if (maximumNumberOfItemsThatFitInPaddingBottomArea()) {
+ if (numItems() > (m_indexOffset + numVisibleItems()))
+ m_indexOfFirstVisibleItemInsidePaddingBottomArea = m_indexOffset + numVisibleItems();
+ }
+}
+
void RenderListBox::scrollTo(int newOffset)
{
if (newOffset == m_indexOffset)
return;
m_indexOffset = newOffset;
+
+ computeFirstIndexesVisibleInPaddingTopBottomAreas();
+
repaint();
document().eventQueue().enqueueOrDispatchScrollEvent(selectElement());
}
Modified: trunk/Source/WebCore/rendering/RenderListBox.h (200264 => 200265)
--- trunk/Source/WebCore/rendering/RenderListBox.h 2016-04-29 21:29:01 UTC (rev 200264)
+++ trunk/Source/WebCore/rendering/RenderListBox.h 2016-04-29 21:48:43 UTC (rev 200265)
@@ -34,6 +34,8 @@
#include "RenderBlockFlow.h"
#include "ScrollableArea.h"
+#include <wtf/Optional.h>
+
namespace WebCore {
class HTMLSelectElement;
@@ -149,9 +151,18 @@
PassRefPtr<Scrollbar> createScrollbar();
void destroyScrollbar();
+ int maximumNumberOfItemsThatFitInPaddingBottomArea() const;
+
+ int numberOfVisibleItemsInPaddingTop() const;
+ int numberOfVisibleItemsInPaddingBottom() const;
+
+ void computeFirstIndexesVisibleInPaddingTopBottomAreas();
+
LayoutUnit itemHeight() const;
void valueChanged(unsigned listIndex);
- int numVisibleItems() const;
+
+ enum class ConsiderPadding { Yes, No };
+ int numVisibleItems(ConsiderPadding = ConsiderPadding::No) const;
int numItems() const;
LayoutUnit listHeight() const;
void paintScrollbar(PaintInfo&, const LayoutPoint&);
@@ -167,6 +178,9 @@
int m_optionsWidth;
int m_indexOffset;
+ Optional<int> m_indexOfFirstVisibleItemInsidePaddingTopArea;
+ Optional<int> m_indexOfFirstVisibleItemInsidePaddingBottomArea;
+
RefPtr<Scrollbar> m_vBar;
};
