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

Source/core/rendering/RenderLayerScrollableArea.cpp

Index: Source/core/rendering/RenderLayerScrollableArea.cpp
diff --git a/Source/core/rendering/RenderLayerScrollableArea.cpp b/Source/core/rendering/RenderLayerScrollableArea.cpp
index ab57b5b14b762ce471fa304300f8bc9603bf28f7..a603721764a4ca86138289944e0ea9596d66a7db 100644
--- a/Source/core/rendering/RenderLayerScrollableArea.cpp
+++ b/Source/core/rendering/RenderLayerScrollableArea.cpp
@@ -73,6 +73,11 @@ 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)
@@ -81,6 +86,7 @@ RenderLayerScrollableArea::RenderLayerScrollableArea(RenderBox* box)
     , m_inOverflowRelayout(false)
     , m_needsCompositedScrolling(false)
     , m_forceNeedsCompositedScrolling(DoNotForceCompositedScrolling)
+    , m_minimumSizeForResizing(defaultMinimumSizeForResizing)
     , m_scrollCorner(0)
     , m_resizer(0)
 {
@@ -716,6 +722,18 @@ void RenderLayerScrollableArea::updateAfterStyleChange(const RenderStyle* oldSty
     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);
@@ -1358,8 +1376,7 @@ void RenderLayerScrollableArea::resize(const PlatformEvent& evt, const LayoutSiz
     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()) {