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 55 matching lines @@
 #include "platform/graphics/GraphicsContextStateSaver.h"
 #include "platform/graphics/GraphicsLayer.h"
 #include "platform/scroll/ScrollAnimator.h"
 #include "platform/scroll/ScrollbarTheme.h"
 #include "public/platform/Platform.h"
 
 namespace WebCore {
 
 const int ResizerControlExpandRatioForTouch = 2;
 
+// Defines the default minimum size while resizing.

[Julien - ping for review, 2014/04/24 21:49:05]

I think the variable name conveys this idea pretty well without this comment.

[harpreet.sk, 2014/04/25 13:33:48]

This line of comment removed

+// Default size is set to 15x15 as it is the default
+// minimum size used by other browsers.

[Julien - ping for review, 2014/04/24 21:49:05]

You only mentioned testing on Firefox so this seems like a broad statement to
say "other browsers". I would rather stick to the facts here.

[harpreet.sk, 2014/04/25 13:33:48]

Removed other browsers and added firefox

+const LayoutSize defaultMinimumSizeForResizing(LayoutUnit(15), LayoutUnit(15));

[Julien - ping for review, 2014/04/24 21:49:05]

It should have static linkage too.

I also think having 2 variables should make the code cleaner.

[harpreet.sk, 2014/04/25 13:33:48]

Introduce a new variable defaultMinimumValueForResizing to hold value of 15

+
 RenderLayerScrollableArea::RenderLayerScrollableArea(RenderBox* box)
     : m_box(box)
     , m_inResizeMode(false)
     , m_scrollsOverflow(false)
     , m_scrollDimensionsDirty(true)
     , m_inOverflowRelayout(false)
     , m_needsCompositedScrolling(false)
     , m_forceNeedsCompositedScrolling(DoNotForceCompositedScrolling)
+    , m_minimumSizeForResizing(defaultMinimumSizeForResizing)
     , m_scrollCorner(0)
     , m_resizer(0)
 {
     ScrollableArea::setConstrainsScrollingToContentEdge(false);
 
     Node* node = m_box->node();
     if (node && node->isElementNode()) {
         // We save and restore only the scrollOffset as the other scroll values are recalculated.
         Element* element = toElement(node);
         m_scrollOffset = element->savedLayerScrollOffset();
@@ skipping 615 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();
 
+    LayoutUnit minimumWidth;
+    LayoutUnit minimumHeight;
+
+    if (m_box->parent()) {

[Julien - ping for review, 2014/04/24 21:49:05]

This is wrong, you want to use your containingBlock() here, not your parent.

[harpreet.sk, 2014/04/25 13:33:48]

Replaced with containingBlock()

+        minimumWidth = valueForLength(m_box->style()->logicalMinWidth(), m_box->parentBox()->logicalWidth());
+        minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->parentBox()->logicalHeight());

[Julien - ping for review, 2014/04/24 21:49:05]

I think this could not return what is expected by CSS if your containing block's
'height' is auto (ie it depends on this element's content) or if you have an
orthogonal writing-mode. I would use constrainLogicalHeightByMinMax (same for
the logical width) and let it do the magic.

[harpreet.sk, 2014/04/25 13:33:48]

I thought of keeping it same as m_box->containingBlock()->logicalWidth() ( same
for height as well) as i checked the behaviour of test case mention above
when containing block's height is auto and find the behaviour similar to
firefox. 

Case:  
<div style="width:auto; height:auto">
<textarea style="width:400px; height:400px; min-width:10%; min-height:10%">
Some text
</textarea>
</div>

Behviour in firefox and in chromium with my patch:  

The text area height will resize upto internal minimum height ( 15px) as
containing block height is changing ( as it depends on textarea content ) and so
do min-height as it depends on containing block.

And for case of orthogonal wirting mode i think logicalWidth() and
logicalHeight() checks for it before returning width or height so i think this
case will not be a problem.

[Julien - ping for review, 2014/04/28 17:07:29]

You're misunderstanding here: vertical writing mode is handled correctly as you
pointed out. However orthogonal writings modes (or orthogonal flow in the
specification http://dev.w3.org/csswg/css-writing-modes/#orthogonal-flows) are
not as you will be using the wrong side.

As a whole, it looks like this code is confused as it mixes logical (ie writing
mode aware) and physical coordinates.

+    }
+
+    minimumWidth = std::max(minimumWidth, defaultMinimumSizeForResizing.width());
+    minimumHeight = std::max(minimumHeight, defaultMinimumSizeForResizing.height());

[Julien - ping for review, 2014/04/24 21:49:05]

How about rolling this code into an helper function?

[harpreet.sk, 2014/04/25 13:33:48]

Introduce a helper function updateMinimumSizeForResizing() and code  is rolled
into it.

+    m_minimumSizeForResizing = LayoutSize(minimumWidth, minimumHeight);
+
     // 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);
-    element->setMinimumSizeForResizing(minimumSize);
+    LayoutSize minimumSize = m_minimumSizeForResizing;

[Julien - ping for review, 2014/04/24 21:49:05]

No need for this variable anymore.

[harpreet.sk, 2014/04/25 13:33:48]

Variable removed.

 
     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