Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239983004:
Textarea resize-able only to larger; min-height and min-width properly set (Closed)
|
Created:
6 years, 8 months ago by harpreet.sk Modified:
6 years, 7 months ago CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, blink-layers+watch_chromium.org, jchaffraix+rendering, pdr., rune+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionTextarea resize-able only to larger; min-height and min-width properly set
Textarea does not respect the min-height and min-width property when they are
set by user. When textarea min-height and min-width is set by the user and
if we resize textarea it will only resize to value larger than the width and
heght of textarea and not with value smaller than it's width and height. It
assumes width and height as min-width and min height and does not take into
account the actual min-width and min-height set by user. This patch removes
this bug by initializing the setMinimumSizeForResizing value with min-width
and min-height.
BUG=94583
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173269
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressing the changes asked in comments in patch set 1 #
Total comments: 15
Patch Set 3 : Addresses the changes asked in patch set 2 #
Total comments: 21
Patch Set 4 : Addresses the changes asked in patch set 3 #
Total comments: 9
Patch Set 5 : Addressing the changes asked in patch set 4 #
Total comments: 10
Patch Set 6 : Addresses the changes asked in patch set 5 #
Total comments: 1
Patch Set 7 : Rebase and added shouldBeEqualToString() instead of shouldBe() in LayoutTest #Messages
Total messages: 28 (0 generated)
Please have a look.
Julien would be the best person to review this change.
I expect the 2 previous bugs to be covered by a test in the follow-up change. https://codereview.chromium.org/239983004/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderLayerScrollableArea.cpp:97: element->setMinimumSizeForResizing(LayoutSize(LayoutUnit(m_box->style()->logicalMinWidth().value()), LayoutUnit(m_box->style()->logicalMinHeight().value()))); This is wrong for several reasons: 1. it's not dynamic, which means that if you update min-width / min-height, you won't update your limit and that seems wrong to me. 2. more importantly this will convert min-width: 50% into min-width: 50px which is is just wrong. To solve them: 1. You need to put the logic into updateAfterStyleChange. 2. You don't convert a Length into a LayoutUnit by calling Length.value(), that's because some Length need some computations to be converted into absolute pixels. That's handled by valueForLength().
This allows resizing the textarea to 0, we don't support that on purpose since once you do it there's no way to undo it as there's no handle to grab. You need to clamp the minimum value to at least the some minimum size, probably 16x16 at the smallest, but we might want it to be larger so the user knows what the thing is.
https://codereview.chromium.org/239983004/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderLayerScrollableArea.cpp:97: element->setMinimumSizeForResizing(LayoutSize(LayoutUnit(m_box->style()->logicalMinWidth().value()), LayoutUnit(m_box->style()->logicalMinHeight().value()))); On 2014/04/18 17:04:08, Julien Chaffraix - PST wrote: > This is wrong for several reasons: > 1. it's not dynamic, which means that if you update min-width / min-height, you > won't update your limit and that seems wrong to me. > 2. more importantly this will convert min-width: 50% into min-width: 50px which > is is just wrong. > > To solve them: > 1. You need to put the logic into updateAfterStyleChange. > 2. You don't convert a Length into a LayoutUnit by calling Length.value(), > that's because some Length need some computations to be converted into absolute > pixels. That's handled by valueForLength(). I have added the logic into updateAfterStyleChange. Now using valueForLength() for converting Length to LayoutUnit. As per esprehn comment i also added check to see if min-height or min-width is 0 or not. If it is 0 i had set the min-value to 15px. I set it to 15px as Firefox also set it to 15px.
On 2014/04/18 17:04:08, Julien Chaffraix - PST wrote: > I expect the 2 previous bugs to be covered by a test in the follow-up change. I added the case of updating the min-width and min-height and setting min-width value in terms of % in existing LayoutTest.
On 2014/04/18 17:09:26, esprehn wrote: > This allows resizing the textarea to 0, we don't support that on purpose since > once you do it there's no way to undo it as there's no handle to grab. > > You need to clamp the minimum value to at least the some minimum size, probably > 16x16 at the smallest, but we might want it to be larger so the user knows what > the thing is. I cross-checked min-height and min-width in Firefox. Firefox clamp the minimum size to be at 15x15 so in the given patch i set the minimum size to be 15px if the min-height or min-width is 0. Please let me know if you want to change it.
https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-min-width-min-height.html (right): https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:2: "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> We prefer to use the HTML5 doctype (easier to read): <!DOCTYPE html> https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:11: function updateSize() I am not a huge fan of bundling several tests into one. This is because your test is now fragile (can break for several reasons) and unfocused (more difficult to maintain / follow). Also this test has the following shortcomings: - you don't test the 15x15 minimum. - you don't test other values for <length> (<percentage> is only one of the possibility that would have been broken, viewport relative length would have been as broken, https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:22: testRunner.waitUntilDone(); You don't need to make the test asynchronous as we wait until the end of the 'load' listener to stop the test and dump the result. https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:26: log("testRunner is not available"); That's not an helpful and actionable message. Somebody opening the test wouldn't understand what this means. Here is a better message: log("This test needs to window.testRunner and window.eventSender to work. To manually test it, drag the text area above, it shouldn't go below the min-width and min-height."); https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (left): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1361: LayoutSize minimumSize = element->minimumSizeForResizing().shrunkTo(currentSize); Don't we need the shrunkTo call ias zoomFactor can be as big as you want (it's determined by 'factor' AFAICT)? https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:727: minimumHeight = LayoutUnit(15); 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. https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:728: element->setMinimumSizeForResizing(LayoutSize(minimumWidth, minimumHeight)); 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)?
On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > ... > https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... > Source/core/rendering/RenderLayerScrollableArea.cpp:728: > element->setMinimumSizeForResizing(LayoutSize(minimumWidth, minimumHeight)); > 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)? The only reason to put it in the DOM is to deal with reattach (that's why the scroll offset is there too). This doesn't seem like it needs to survive a reattach though, so this should probably be inside ScrollableArea.
https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:728: element->setMinimumSizeForResizing(LayoutSize(minimumWidth, minimumHeight)); On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > 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)? 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.
https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:728: element->setMinimumSizeForResizing(LayoutSize(minimumWidth, minimumHeight)); On 2014/04/22 13:56:36, harpreet.sk wrote: > On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > > > 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)? > > > > 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. Your analysis is fine by me.
https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-min-width-min-height.html (right): https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:2: "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > We prefer to use the HTML5 doctype (easier to read): > > <!DOCTYPE html> Changed to HTML5 doctype. https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:11: function updateSize() On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > I am not a huge fan of bundling several tests into one. This is because your > test is now fragile (can break for several reasons) and unfocused (more > difficult to maintain / follow). > > Also this test has the following shortcomings: > - you don't test the 15x15 minimum. > - you don't test other values for <length> (<percentage> is only one of the > possibility that would have been broken, viewport relative length would have > been as broken, This layout test is removed and 3 new layout test added with different test cases as given below: Case 1: Test for resizing below intrinsic minmum size ( 15x15 case ) Case 2: Test for resizing below minimum size set by user. Case 3 Test for resizing above minimum size set by user and below initial size. Each layout test also checks for following types of length: 1. Length of fixed type (px) 2. Length of percentage type (%) 3. Length in terms of viewport-length (vw or vh) https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:22: testRunner.waitUntilDone(); On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > You don't need to make the test asynchronous as we wait until the end of the > 'load' listener to stop the test and dump the result. Done. https://codereview.chromium.org/239983004/diff/20001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-min-width-min-height.html:26: log("testRunner is not available"); On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > That's not an helpful and actionable message. Somebody opening the test wouldn't > understand what this means. > > Here is a better message: > log("This test needs to window.testRunner and window.eventSender to work. To > manually test it, drag the text area above, it shouldn't go below the min-width > and min-height."); Action message is modified. https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (left): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1361: LayoutSize minimumSize = element->minimumSizeForResizing().shrunkTo(currentSize); On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > Don't we need the shrunkTo call ias zoomFactor can be as big as you want (it's > determined by 'factor' AFAICT)? 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. https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:727: minimumHeight = LayoutUnit(15); On 2014/04/21 17:46:46, Julien Chaffraix - PST wrote: > 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. 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. https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-intrinsic-size.html (right): https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-intrinsic-size.html:32: if ((draggable.style.width == "9px") && (draggable.style.height == "9px")) The condition check here for 9px and not 15px as min-width/min-height includes padding and border size. Border and padding size together sum upto 6px for width/height ( 2px padding all side and 1px border all side). So width/height returns 9px. Is it a bug that min-width includes padding and border and width is not including border and padding? I thinks it happens only when we include html5 doctype. https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:53: if ((draggable.style.width == "194px") && (draggable.style.height == "194px")) The condition check here for 194px and not 200px as min-width/min-height includes padding and border size so subtracted 6px from width/height. https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:58: if ((draggable.style.width == "114px") && (draggable.style.height == "84px")) Ideally for this case (viewport-length) the width should be 189px and height should be 59px as i checked in the browser by manually dragging it but when i run it in test runner it returns width and height as given in condition. I don't know why it is returning such value. Can you tell me why test runner is returning this width and height? https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:63: if ((draggable.style.width == "74px") && (draggable.style.height == "74px")) min-width/min-height includes padding and border size so subtracted 6px from width/height.
This is starting to look good. Some more comments. https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:53: if ((draggable.style.width == "194px") && (draggable.style.height == "194px")) On 2014/04/24 15:28:27, harpreet.sk wrote: > The condition check here for 194px and not 200px as min-width/min-height > includes padding and border size so subtracted 6px from width/height. I expect that we need the borders to delimit the text-area so you should put these comments into the code. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:76: // Defines the default minimum size while resizing. I think the variable name conveys this idea pretty well without this comment. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:78: // minimum size used by other browsers. 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. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:79: const LayoutSize defaultMinimumSizeForResizing(LayoutUnit(15), LayoutUnit(15)); It should have static linkage too. I also think having 2 variables should make the code cleaner. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:728: if (m_box->parent()) { This is wrong, you want to use your containingBlock() here, not your parent. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:730: minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->parentBox()->logicalHeight()); 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. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:734: minimumHeight = std::max(minimumHeight, defaultMinimumSizeForResizing.height()); How about rolling this code into an helper function? https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1379: LayoutSize minimumSize = m_minimumSizeForResizing; No need for this variable anymore.
https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/30001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:53: if ((draggable.style.width == "194px") && (draggable.style.height == "194px")) On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > On 2014/04/24 15:28:27, harpreet.sk wrote: > > The condition check here for 194px and not 200px as min-width/min-height > > includes padding and border size so subtracted 6px from width/height. > > I expect that we need the borders to delimit the text-area so you should put > these comments into the code. Comment added. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:76: // Defines the default minimum size while resizing. On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > I think the variable name conveys this idea pretty well without this comment. This line of comment removed https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:78: // minimum size used by other browsers. On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > 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. Removed other browsers and added firefox https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:79: const LayoutSize defaultMinimumSizeForResizing(LayoutUnit(15), LayoutUnit(15)); On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > It should have static linkage too. > > I also think having 2 variables should make the code cleaner. Introduce a new variable defaultMinimumValueForResizing to hold value of 15 https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:728: if (m_box->parent()) { On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > This is wrong, you want to use your containingBlock() here, not your parent. Replaced with containingBlock() https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:730: minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->parentBox()->logicalHeight()); On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > 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. 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. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:734: minimumHeight = std::max(minimumHeight, defaultMinimumSizeForResizing.height()); On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > How about rolling this code into an helper function? Introduce a helper function updateMinimumSizeForResizing() and code is rolled into it. https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1379: LayoutSize minimumSize = m_minimumSizeForResizing; On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > No need for this variable anymore. Variable removed.
https://codereview.chromium.org/239983004/diff/40001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/40001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:61: if ((draggable.style.width == "114px") && (draggable.style.height == "84px")) Ideally for this case (viewport-length) the width should be 189px and height should be 59px as i checked in the browser by manually dragging it but when i run it in test runner it returns width and height as given in condition. I don't know why it is returning such value. Can you tell me why test runner is returning this width and height?
https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/30001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:730: minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->parentBox()->logicalHeight()); On 2014/04/25 13:33:48, harpreet.sk wrote: > On 2014/04/24 21:49:05, Julien Chaffraix - PST wrote: > > 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. > > > 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. 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. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:80: static const LayoutSize defaultMinimumSizeForResizing(defaultMinimumValueForResizing, defaultMinimumValueForResizing); What I meant by 2 variables is minWidth and minHeight, instead of bundling the 2 together. We can keep the internal logic to deal with LayoutSize though. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:731: minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->containingBlock()->logicalHeight()); This should be in physical coordinates as m_minimumSizeForResizing is. FWIW the current logic is confused as it uses LayoutSize (fractional pixel unit) and not physical pixels. I think we should just make this logic consistent and use integer / IntSize, not LayoutUnit / LayoutSize. To ensure the mark, we should also add an orthogonal flow test. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:735: minimumHeight = std::max(minimumHeight, defaultMinimumSizeForResizing.height()); I think this code would be better if you initialized |minimumWidth| and |minimumHeight| to these minimum and then do the std::max if you have a containing block (when do we not have a containing block btw?) https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1386: LayoutSize difference = (currentSize + newOffset - adjustedOldOffset).expandedTo(m_minimumSizeForResizing) - currentSize; We should probably not cache the value as resize() is a rare operation compared to style changes. That's why I would just change updateMinimumSizeForResizing() to minimumSizeForResizing() and make it return the IntSize.
Added the orthogonal flow test and made the changes asked in previous patch. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:80: static const LayoutSize defaultMinimumSizeForResizing(defaultMinimumValueForResizing, defaultMinimumValueForResizing); On 2014/04/28 17:07:29, Julien Chaffraix - PST wrote: > What I meant by 2 variables is minWidth and minHeight, instead of bundling the 2 > together. > > We can keep the internal logic to deal with LayoutSize though. Added the variables defaultMinimumWidthForResizing and defaultMinimumHeightForResizing. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:731: minimumHeight = valueForLength(m_box->style()->logicalMinHeight(), m_box->containingBlock()->logicalHeight()); On 2014/04/28 17:07:29, Julien Chaffraix - PST wrote: > This should be in physical coordinates as m_minimumSizeForResizing is. FWIW the > current logic is confused as it uses LayoutSize (fractional pixel unit) and not > physical pixels. I think we should just make this logic consistent and use > integer / IntSize, not LayoutUnit / LayoutSize. > > To ensure the mark, we should also add an orthogonal flow test. Now using int for minimumWidth and minimumHeight and the api will return IntSize. Also the orthogonal flow test has been added. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:735: minimumHeight = std::max(minimumHeight, defaultMinimumSizeForResizing.height()); On 2014/04/28 17:07:30, Julien Chaffraix - PST wrote: > I think this code would be better if you initialized |minimumWidth| and > |minimumHeight| to these minimum and then do the std::max if you have a > containing block (when do we not have a containing block btw?) Actually i used containing block check earlier because browser was getting crashed just on launch maybe because for some element containing block does not exist. Now i had remove this containing block check as now the api minimumSizeForResizing will be called in the resize api where we want to use minimum size for resizing and not in the updateStyleChange api. It works fine for all cases. https://codereview.chromium.org/239983004/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1386: LayoutSize difference = (currentSize + newOffset - adjustedOldOffset).expandedTo(m_minimumSizeForResizing) - currentSize; On 2014/04/28 17:07:30, Julien Chaffraix - PST wrote: > We should probably not cache the value as resize() is a rare operation compared > to style changes. > > That's why I would just change updateMinimumSizeForResizing() to > minimumSizeForResizing() and make it return the IntSize. The api is changed to minimumSizeForResizing() and it return IntSize.
lgtm The comments on the last test apply to all the tests. I would like to see the updated change before it lands. https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html (right): https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html:46: <textarea style="width:400px; height:400px; min-width:10%; min-height:10%" id="textInputID"> Writing mode is inherited so the textarea and the div share the same writing mode. If you want an orthogonal flow, you have to override the textarea's writing mode. https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:69: log("FAIL textAreaElement width is " + draggable.style.width + " and height is " + draggable.style.height); FYI we have a framework that does exactly what you do here. It's the JS file LayoutTests/resources/js-test.js and it uses unit test inspired names like shouldBe(A, B)... I would rather use it as it would avoid a lot of this boilerplate code. https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:76: <div style="width:800px; height:800px"> It's missing a description for what the test is testing and the expected result (3 PASS). https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:725: int minimumHeight; No need for C89 style declaration. It's better to just declare and initialize them at the same time below. https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1380: LayoutSize difference = (currentSize + newOffset - adjustedOldOffset).expandedTo(LayoutSize(minimumSizeForResizing())) - currentSize; Do we really need this explicit conversion? If we really do it, it seems better to just change minimumSizeForResizing() to return a LayoutSize (with a FIXME that it's not right but this code is confused).
https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html (right): https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html:46: <textarea style="width:400px; height:400px; min-width:10%; min-height:10%" id="textInputID"> On 2014/04/29 22:13:54, Julien Chaffraix - PST wrote: > Writing mode is inherited so the textarea and the div share the same writing > mode. If you want an orthogonal flow, you have to override the textarea's > writing mode. Done. https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/textarea-resize-below-min-size-set.html (right): https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:69: log("FAIL textAreaElement width is " + draggable.style.width + " and height is " + draggable.style.height); On 2014/04/29 22:13:54, Julien Chaffraix - PST wrote: > FYI we have a framework that does exactly what you do here. It's the JS file > LayoutTests/resources/js-test.js and it uses unit test inspired names like > shouldBe(A, B)... > > I would rather use it as it would avoid a lot of this boilerplate code. Done. https://codereview.chromium.org/239983004/diff/60001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/textarea-resize-below-min-size-set.html:76: <div style="width:800px; height:800px"> On 2014/04/29 22:13:54, Julien Chaffraix - PST wrote: > It's missing a description for what the test is testing and the expected result > (3 PASS). Description and expected result added in all layout test. https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:725: int minimumHeight; On 2014/04/29 22:13:54, Julien Chaffraix - PST wrote: > No need for C89 style declaration. It's better to just declare and initialize > them at the same time below. Done. https://codereview.chromium.org/239983004/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderLayerScrollableArea.cpp:1380: LayoutSize difference = (currentSize + newOffset - adjustedOldOffset).expandedTo(LayoutSize(minimumSizeForResizing())) - currentSize; On 2014/04/29 22:13:54, Julien Chaffraix - PST wrote: > Do we really need this explicit conversion? If we really do it, it seems better > to just change minimumSizeForResizing() to return a LayoutSize (with a FIXME > that it's not right but this code is confused). I cross checked that expandTo() takes LayoutSize as argument so i thought of converting it but it's working fine even if we pass IntSize so i had rmeove this explicit conversion.
lgtm https://codereview.chromium.org/239983004/diff/80001/LayoutTests/fast/forms/t... File LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html (right): https://codereview.chromium.org/239983004/diff/80001/LayoutTests/fast/forms/t... LayoutTests/fast/forms/text-area-resize-orthogonal-containingBlock.html:48: shouldBe('document.getElementById("textInputID").style.height;', '"74px"'); FYI for next change, there is a shouldBeEqualToString which removes the double quotes (it took me a long time to discover it as it's hidden in js-test.js).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/239983004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/rendering/RenderLayerScrollableArea.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/rendering/RenderLayerScrollableArea.cpp
Hunk #1 FAILED at 74.
Hunk #2 succeeded at 722 (offset 8 lines).
Hunk #3 FAILED at 1362.
Hunk #4 succeeded at 1399 with fuzz 2 (offset 28 lines).
2 out of 4 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderLayerScrollableArea.cpp.rej
Patch: 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
d341b15c297482c8479e09e707bee5f24d4a2da6..017b25bfedcb9ef91a1eef5af65f080a9f6a8523
100644
--- a/Source/core/rendering/RenderLayerScrollableArea.cpp
+++ b/Source/core/rendering/RenderLayerScrollableArea.cpp
@@ -74,6 +74,11 @@ namespace WebCore {
const int ResizerControlExpandRatioForTouch = 2;
+// Default value is set to 15 as the default
+// minimum size used by firefox is 15x15.
+static const int defaultMinimumWidthForResizing = 15;
+static const int defaultMinimumHeightForResizing = 15;
+
RenderLayerScrollableArea::RenderLayerScrollableArea(RenderBox& box)
: m_box(box)
, m_inResizeMode(false)
@@ -714,6 +719,16 @@ static bool overflowDefinesAutomaticScrollbar(EOverflow
overflow)
return overflow == OAUTO || overflow == OOVERLAY;
}
+IntSize RenderLayerScrollableArea::minimumSizeForResizing()
+{
+ int minimumWidth = intValueForLength(m_box.style()->logicalMinWidth(),
m_box.containingBlock()->logicalWidth());
+ int minimumHeight = intValueForLength(m_box.style()->logicalMinHeight(),
m_box.containingBlock()->logicalHeight());
+
+ minimumWidth = std::max(minimumWidth, defaultMinimumWidthForResizing);
+ minimumHeight = std::max(minimumHeight, defaultMinimumHeightForResizing);
+ return IntSize(minimumWidth, minimumHeight);
+}
+
void RenderLayerScrollableArea::updateAfterStyleChange(const RenderStyle*
oldStyle)
{
// List box parts handle the scrollbars by themselves so we have nothing to
do.
@@ -1352,8 +1367,6 @@ 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 adjustedOldOffset = LayoutSize(oldOffset.width() / zoomFactor,
oldOffset.height() / zoomFactor);
if (m_box.style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft()) {
@@ -1361,7 +1374,7 @@ void RenderLayerScrollableArea::resize(const
PlatformEvent& evt, const LayoutSiz
adjustedOldOffset.setWidth(-adjustedOldOffset.width());
}
- LayoutSize difference = (currentSize + newOffset -
adjustedOldOffset).expandedTo(minimumSize) - currentSize;
+ LayoutSize difference = (currentSize + newOffset -
adjustedOldOffset).expandedTo(minimumSizeForResizing()) - currentSize;
bool isBoxSizingBorder = m_box.style()->boxSizing() == BORDER_BOX;
The CQ bit was checked by harpreet.sk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/239983004/100001
Message was sent while issue was closed.
Change committed as 173269
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/336143002/ by tkent@chromium.org. The reason for reverting is: Regression for RTL textareas. BUG=94583,371743 . |
