Textarea resize-able only to larger; min-height and min-width properly set

Source/core/rendering/RenderLayerScrollableArea.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@web.de>
  *   Randall Jesup <rjesup@wgate.com>
@@ skipping 698 matching lines @@
     // RenderView shouldn't provide scrollbars on its own.
     if (m_box->isRenderView())
         return;
 
     if (!m_scrollDimensionsDirty)
         updateScrollableAreaSet(hasScrollableHorizontalOverflow() || hasScrollableVerticalOverflow());
 
     EOverflow overflowX = m_box->style()->overflowX();
     EOverflow overflowY = m_box->style()->overflowY();
 
+    Node* node = m_box->node();
+    if (node && node->isElementNode()) {
+        Element* element = toElement(node);
+        LayoutUnit minimumWidth = valueForLength(m_box->style()->logicalMinWidth(), m_box->logicalWidth());
+        LayoutUnit minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->logicalHeight());
+        if (minimumWidth == 0)
+            minimumWidth = LayoutUnit(15);
+        if (minimumHeight == 0)
+            minimumHeight = LayoutUnit(15);

[Julien - ping for review, 2014/04/21 17:46:46]

I don't think we should only check for 0. We should just do a min between
min-height/min-width and our internal minimal size.

Please also: NO MAGIC CONSTANT. Add a meaningful variable to hold them along
with a comment on why you chose these values as you have a great explanation
behind them.

[harpreet.sk, 2014/04/24 15:28:27]

Now added a check doing a max between the internal min-height/min-width and set
min-height/min-width. 

Added a variable along with comment for storing internal minimum size.

+        element->setMinimumSizeForResizing(LayoutSize(minimumWidth, minimumHeight));

[Julien - ping for review, 2014/04/21 17:46:46]

The fact that we do a round-trip through the DOM is just plain wrong (this was
pointed out in the original bug ->
https://bugs.webkit.org/show_bug.cgi?id=12798).

We don't have to fix it here but I think we should just store the minimum size
either in RenderLayerScrollableArea or directly ScrollableArea. The second
choice is more appealing to me if we can prove that other classes deriving from
ScrollableArea (RenderListBox comes to my mind) are impacted by this bug too
(which I would expect). Would you care to file a bug about that (extra points if
you want to fix it too)?

[harpreet.sk, 2014/04/22 13:56:36]

The classes deriving from ScrollableArea other than RenderLayerScrollableArea
are:

1. RenderListBox: The current bug will not impact this class. It is showing
behaviour similar to firefox. If we only set min-width/min-height and do not set
width/height then min-width/min-height of listbox will be max of intrinsic
calculated min-width/min-height and style min-width/min-height.  If width is
also been set in style then maximum between style width and style min-width will
be taken and set as the min-width of listbox. Same behaviour is shown by
firefox. So i don't think so we need to store the minimum size for listbox.

2. PinchViewport

3. ScrollView

4 ScrollbarGroup

I dont think that above classes will be impacted by the bug so i think it will
better to store the minimumSizeForResizing in RenderLayerScrollableArea other
than ScrollableArea. 

Currently i think only RenderLayerScrollableArea is using
minimumSizeForResizing() of Element.cpp and no other derive class of
ScrollableArea is using it. So it would be better to include it in
RenderLayerScrollableArea. 

Please correct me if i am wrong.

[Julien - ping for review, 2014/04/22 22:35:12]

Your analysis is fine by me.

+    }
+
     // To avoid doing a relayout in updateScrollbarsAfterLayout, we try to keep any automatic scrollbar that was already present.
     bool needsHorizontalScrollbar = (hasHorizontalScrollbar() && overflowDefinesAutomaticScrollbar(overflowX)) || overflowRequiresScrollbar(overflowX);
     bool needsVerticalScrollbar = (hasVerticalScrollbar() && overflowDefinesAutomaticScrollbar(overflowY)) || overflowRequiresScrollbar(overflowY);
     setHasHorizontalScrollbar(needsHorizontalScrollbar);
     setHasVerticalScrollbar(needsVerticalScrollbar);
 
     // With overflow: scroll, scrollbars are always visible but may be disabled.
     // When switching to another value, we need to re-enable them (see bug 11985).
     if (needsHorizontalScrollbar && oldStyle && oldStyle->overflowX() == OSCROLL && overflowX != OSCROLL) {
         ASSERT(hasHorizontalScrollbar());
@@ skipping 622 matching lines @@
         ASSERT_NOT_REACHED();
     }
 
     float zoomFactor = m_box->style()->effectiveZoom();
 
     LayoutSize newOffset = offsetFromResizeCorner(document.view()->windowToContents(pos));
     newOffset.setWidth(newOffset.width() / zoomFactor);
     newOffset.setHeight(newOffset.height() / zoomFactor);
 
     LayoutSize currentSize = LayoutSize(m_box->width() / zoomFactor, m_box->height() / zoomFactor);
-    LayoutSize minimumSize = element->minimumSizeForResizing().shrunkTo(currentSize);

[Julien - ping for review, 2014/04/21 17:46:46]

Don't we need the shrunkTo call ias zoomFactor can be as big as you want (it's
determined by 'factor' AFAICT)?

[harpreet.sk, 2014/04/24 15:28:27]

We do not need shrunkTo() as over here m_box->width()/m_box->height() value is
equal to (zoomFactor * currentWidth)/(zoomFactor * currentHeigt) so after
reducing m_box->width() by factor of zoom we will never get value smaller than
minimum size.

-    element->setMinimumSizeForResizing(minimumSize);
+    LayoutSize minimumSize = element->minimumSizeForResizing();
 
     LayoutSize adjustedOldOffset = LayoutSize(oldOffset.width() / zoomFactor, oldOffset.height() / zoomFactor);
     if (m_box->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft()) {
         newOffset.setWidth(-newOffset.width());
         adjustedOldOffset.setWidth(-adjustedOldOffset.width());
     }
 
     LayoutSize difference = (currentSize + newOffset - adjustedOldOffset).expandedTo(minimumSize) - currentSize;
 
     bool isBoxSizingBorder = m_box->style()->boxSizing() == BORDER_BOX;
@@ skipping 155 matching lines @@
 void RenderLayerScrollableArea::setForceNeedsCompositedScrolling(ForceNeedsCompositedScrollingMode mode)
 {
     if (m_forceNeedsCompositedScrolling == mode)
         return;
 
     m_forceNeedsCompositedScrolling = mode;
     layer()->didUpdateNeedsCompositedScrolling();
 }
 
 } // Namespace WebCore