Changeset 200265 in webkit


Ignore:
Timestamp:
Apr 29, 2016 2:48:43 PM (8 years ago)
Author:
tonikitoo@webkit.org
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.
Location:
trunk
Files:
4 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r200264 r200265  
     12016-04-29  Antonio Gomes  <tonikitoo@webkit.org>
     2
     3        <select multiple> padding should react when scrolling
     4        https://bugs.webkit.org/show_bug.cgi?id=156590
     5        https://bugs.webkit.org/show_bug.cgi?id=156591
     6
     7        Reviewed by Reviewed by Darin Adler.
     8
     9        * fast/forms/listbox-respects-padding-bottom-expected.txt: Added.
     10        * fast/forms/listbox-respects-padding-bottom.html: Added.
     11        * fast/forms/listbox-top-padding-do-not-clip-items-expected.txt: Added.
     12        * fast/forms/listbox-top-padding-do-not-clip-items.html: Added.
     13
    1142016-04-29  Eric Carlson  <eric.carlson@apple.com>
    215

  • trunk/LayoutTests/platform/ios-simulator/TestExpectations

    r200264 r200265  
    30023002fast/forms/listbox-padding-clip-overlay.html [ ImageOnlyFailure ]
    30033003fast/forms/listbox-padding-clip-selected.html [ ImageOnlyFailure ]
     3004fast/forms/listbox-respects-padding-bottom.html [ ImageOnlyFailure ]
     3005fast/forms/listbox-top-padding-do-not-clip-items.html [ ImageOnlyFailure ]
    30043006
    30053007# MSE not enabled.

  • trunk/Source/WebCore/ChangeLog

    r200264 r200265  
     12016-04-29  Antonio Gomes  <tonikitoo@webkit.org>
     2
     3        <select multiple> padding should react when scrolling
     4
     5        https://bugs.webkit.org/show_bug.cgi?id=156590
     6        https://bugs.webkit.org/show_bug.cgi?id=156591
     7
     8        Reviewed by Reviewed by Darin Adler.
     9
     10        Tests: fast/forms/listbox-respects-padding-bottom.html
     11               fast/forms/listbox-top-padding-do-not-clip-items.html
     12
     13        Non-dropdown listboxes have support to padding-{top,bottom} implemented similarly
     14        to the border model: the padding area does not move when the listbox' content gets scrolled,
     15        but instead it clips out its content.
     16        This is not consistent with other browsers and is not consistent with the CSS box model.
     17
     18        This in practice, if a <select> has padding-top set, the padding-top area will clip out listbox'
     19        content as one scrolls upwards.
     20        It also means that if padding-bottom is set, when one scrolls all the way to the bottom
     21        of the listbox content, padding-bottom is not respected.
     22
     23        In order to fix these two problems, and make WebKit match Blink with respect to the the way
     24        padding-{top,bottom} are handled, patch adds two class member variables that control the number
     25        of list items (i.e. <option>s) that can be painted over the current listbox' padding area.
     26
     27        In short, depending on the scroll position and the amount of space available in the padding top/bottom
     28        areas, items are painted or not on top of it, mimic'ing the CSS box model behavior of other browsers.
     29
     30        Note that this is specific solution is worth it to pursue on the short/mid term, but a long-term solution
     31        to this problem and many other listbox discrepancies on WebKit's implementation, would be to reimplement
     32        RenderListBox class in terms of RenderLayer. This will be a follow up work.
     33
     34        * rendering/RenderListBox.cpp:
     35        (WebCore::RenderListBox::updateFromElement):
     36        (WebCore::RenderListBox::numVisibleItems):
     37        (WebCore::RenderListBox::paintObject):
     38        (WebCore::RenderListBox::scrollToRevealElementAtListIndex):
     39        (WebCore::RenderListBox::listIndexIsVisible):
     40        (WebCore::RenderListBox::maximumNumberOfItemsThatFitInPaddingBottomArea):
     41        (WebCore::RenderListBox::numberOfVisibleItemsInPaddingTop):
     42        (WebCore::RenderListBox::numberOfVisibleItemsInPaddingBottom):
     43        (WebCore::RenderListBox::computeFirstIndexesVisibleInPaddingTopBottomAreas):
     44        (WebCore::RenderListBox::scrollTo):
     45        * rendering/RenderListBox.h:
     46
    1472016-04-29  Eric Carlson  <eric.carlson@apple.com>
    248

  • trunk/Source/WebCore/rendering/RenderListBox.cpp

    r200190 r200265  
    140140        setHasVerticalScrollbar(true);
    141141
     142        computeFirstIndexesVisibleInPaddingTopBottomAreas();
     143
    142144        setNeedsLayoutAndPrefWidthsRecalc();
    143145    }
    … …  
    237239}
    238240
    239 int RenderListBox::numVisibleItems() const
     241int RenderListBox::numVisibleItems(ConsiderPadding considerPadding) const
    240242{
    241243    // Only count fully visible rows. But don't return 0 even if only part of a row shows.
    242     return std::max<int>(1, (contentHeight() + paddingBottom() + rowSpacing) / itemHeight());
     244    int visibleItemsExcludingPadding = std::max<int>(1, (contentHeight() + rowSpacing) / itemHeight());
     245    if (considerPadding == ConsiderPadding::No)
     246        return visibleItemsExcludingPadding;
     247
     248    return numberOfVisibleItemsInPaddingTop() + visibleItemsExcludingPadding + numberOfVisibleItemsInPaddingBottom();
    243249}
    244250
    … …  
    276282{
    277283    int listItemsSize = numItems();
    278     int endIndex = m_indexOffset + numVisibleItems();
    279     for (int i = m_indexOffset; i < listItemsSize && i <= endIndex; ++i)
     284    int firstVisibleItem = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset);
     285    int endIndex = firstVisibleItem + numVisibleItems(ConsiderPadding::Yes);
     286    for (int i = firstVisibleItem; i < listItemsSize && i < endIndex; ++i)
    280287        paintFunction(paintInfo, paintOffset, i);
    281288}
    … …  
    590597
    591598bool RenderListBox::listIndexIsVisible(int index)
    592 {   
    593     return index >= m_indexOffset && index < m_indexOffset + numVisibleItems();
     599{
     600    int firstIndex = m_indexOfFirstVisibleItemInsidePaddingTopArea.valueOr(m_indexOffset);
     601    int endIndex = m_indexOfFirstVisibleItemInsidePaddingBottomArea
     602        ? m_indexOfFirstVisibleItemInsidePaddingBottomArea.value() + numberOfVisibleItemsInPaddingBottom()
     603        : m_indexOffset + numVisibleItems();
     604
     605    return index >= firstIndex && index < endIndex;
    594606}
    595607
    … …  
    635647}
    636648
     649int RenderListBox::maximumNumberOfItemsThatFitInPaddingBottomArea() const
     650{
     651    return paddingBottom() / itemHeight();
     652}
     653
     654int RenderListBox::numberOfVisibleItemsInPaddingTop() const
     655{
     656    if (!m_indexOfFirstVisibleItemInsidePaddingTopArea)
     657        return 0;
     658
     659    return m_indexOffset - m_indexOfFirstVisibleItemInsidePaddingTopArea.value();
     660}
     661
     662int RenderListBox::numberOfVisibleItemsInPaddingBottom() const
     663{
     664    if (!m_indexOfFirstVisibleItemInsidePaddingBottomArea)
     665        return 0;
     666
     667    return std::min(maximumNumberOfItemsThatFitInPaddingBottomArea(), numItems() - m_indexOffset - numVisibleItems());
     668}
     669
     670void RenderListBox::computeFirstIndexesVisibleInPaddingTopBottomAreas()
     671{
     672    m_indexOfFirstVisibleItemInsidePaddingTopArea = Nullopt;
     673    m_indexOfFirstVisibleItemInsidePaddingBottomArea = Nullopt;
     674
     675    int maximumNumberOfItemsThatFitInPaddingTopArea = paddingTop() / itemHeight();
     676    if (maximumNumberOfItemsThatFitInPaddingTopArea) {
     677        if (m_indexOffset)
     678            m_indexOfFirstVisibleItemInsidePaddingTopArea = std::max(0, m_indexOffset - maximumNumberOfItemsThatFitInPaddingTopArea);
     679    }
     680
     681    if (maximumNumberOfItemsThatFitInPaddingBottomArea()) {
     682        if (numItems() > (m_indexOffset + numVisibleItems()))
     683            m_indexOfFirstVisibleItemInsidePaddingBottomArea = m_indexOffset + numVisibleItems();
     684    }
     685}
     686
    637687void RenderListBox::scrollTo(int newOffset)
    638688{
    … …  
    641691
    642692    m_indexOffset = newOffset;
     693
     694    computeFirstIndexesVisibleInPaddingTopBottomAreas();
     695
    643696    repaint();
    644697    document().eventQueue().enqueueOrDispatchScrollEvent(selectElement());

  • trunk/Source/WebCore/rendering/RenderListBox.h

    r200230 r200265  
    3434#include "RenderBlockFlow.h"
    3535#include "ScrollableArea.h"
     36
     37#include <wtf/Optional.h>
    3638
    3739namespace WebCore {
    … …  
    150152    void destroyScrollbar();
    151153   
     154    int maximumNumberOfItemsThatFitInPaddingBottomArea() const;
     155
     156    int numberOfVisibleItemsInPaddingTop() const;
     157    int numberOfVisibleItemsInPaddingBottom() const;
     158
     159    void computeFirstIndexesVisibleInPaddingTopBottomAreas();
     160
    152161    LayoutUnit itemHeight() const;
    153162    void valueChanged(unsigned listIndex);
    154     int numVisibleItems() const;
     163
     164    enum class ConsiderPadding { Yes, No };
     165    int numVisibleItems(ConsiderPadding = ConsiderPadding::No) const;
    155166    int numItems() const;
    156167    LayoutUnit listHeight() const;
    … …  
    168179    int m_indexOffset;
    169180
     181    Optional<int> m_indexOfFirstVisibleItemInsidePaddingTopArea;
     182    Optional<int> m_indexOfFirstVisibleItemInsidePaddingBottomArea;
     183
    170184    RefPtr<Scrollbar> m_vBar;
    171185};
Note: See TracChangeset for help on using the changeset viewer.