Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14146029:
Fix textarea font autosizing oscillation when the number of lines exceeds 4. The proposed fix is no… (Closed)
|
Created:
7 years, 8 months ago by timvolodine Modified:
7 years, 7 months ago CC:
blink-reviews, jchaffraix+rendering Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix textarea font autosizing oscillation when the number of lines exceeds 4. The proposed fix is not to autosize textareas.
BUG=165481
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149645
Patch Set 1 #Patch Set 2 : fix diff #Patch Set 3 : fix include #
Total comments: 4
Patch Set 4 : autosize textareas by default #
Total comments: 10
Patch Set 5 : fixed John's comments. #Patch Set 6 : fixed John's comments #
Total comments: 2
Patch Set 7 : added expected font size to the tests #
Total comments: 1
Patch Set 8 : rebased #
Messages
Total messages: 19 (0 generated)
Hi John, Potential fix for the text-area boosting bug. Pls have a look. Tim
Code seems good (minor nits). I was originally thinking it might make more sense to always autosize them (rather than never autosizing them). Aurimas, what do you think would be best here? Essentially, how well does Clank cope with really wide textareas? If we never autosize textareas, then there could be textareas where once you've zoomed in such that the text you're typing is legible, you can't see the full width of the textarea and you have to constantly pan horizontally, which sounds annoying. If instead we always autosized textareas, then once you'd zoomed to fit the textarea to the screen, the contents of the textarea would be legible and you wouldn't need to pan horizontally. The text in the textarea would probably be larger than text in other form elements though. Either way though, being consistent is much better than the current state, so even if we decide to always autosize them it probably makes sense to land this patch as an interim fix. https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... File LayoutTests/fast/text-autosizing/textarea-fontsize-change.html (right): https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:12: .largersize{font-size: 1.1em} Unused? https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:27: <textarea rows=8 cols=85> Nit: it's more standard to always put quotes around attributes.
On 2013/04/26 12:24:57, johnme wrote: > Code seems good (minor nits). I was originally thinking it might make more sense > to always autosize them (rather than never autosizing them). > > Aurimas, what do you think would be best here? Essentially, how well does Clank > cope with really wide textareas? If we never autosize textareas, then there > could be textareas where once you've zoomed in such that the text you're typing > is legible, you can't see the full width of the textarea and you have to > constantly pan horizontally, which sounds annoying. > > If instead we always autosized textareas, then once you'd zoomed to fit the > textarea to the screen, the contents of the textarea would be legible and you > wouldn't need to pan horizontally. The text in the textarea would probably be > larger than text in other form elements though. > > Either way though, being consistent is much better than the current state, so > even if we decide to always autosize them it probably makes sense to land this > patch as an interim fix. > > https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... > File LayoutTests/fast/text-autosizing/textarea-fontsize-change.html (right): > > https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... > LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:12: > .largersize{font-size: 1.1em} > Unused? > > https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... > LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:27: <textarea > rows=8 cols=85> > Nit: it's more standard to always put quotes around attributes. My reasoning was mainly based on the crbug in question where the autosized textarea text looks really too big when more than 4 lines are entered. So initially it is not autosized and this code will prevent it from being autosized regardless of the number of lines of text entered.
On 2013/04/26 12:35:29, timvolodine wrote: > My reasoning was mainly based on the crbug in question where the autosized > textarea text looks really too big when more than 4 lines are entered. So > initially it is not autosized and this code will prevent it from being autosized > regardless of the number of lines of text entered. Are you talking about the video in http://go/chrome-androidlogs/224494 ? If so bear in mind that only part of the textarea is currently onscreen; it would be preferable for the full width of the textarea to be visible at the same time, by being slightly more zoomed out, which would make the text look smaller. It's true however that on some sites narrow textareas might be being autosized based on their much wider enclosing cluster; but we can fix that by making textareas into their own cluster so only the width of the textarea itself is taken into account.
On 2013/04/26 12:45:14, johnme wrote: > On 2013/04/26 12:35:29, timvolodine wrote: > > My reasoning was mainly based on the crbug in question where the autosized > > textarea text looks really too big when more than 4 lines are entered. So > > initially it is not autosized and this code will prevent it from being > autosized > > regardless of the number of lines of text entered. > > Are you talking about the video in http://go/chrome-androidlogs/224494 ? If so > bear in mind that only part of the textarea is currently onscreen; it would be > preferable for the full width of the textarea to be visible at the same time, by > being slightly more zoomed out, which would make the text look smaller. > > It's true however that on some sites narrow textareas might be being autosized > based on their much wider enclosing cluster; but we can fix that by making > textareas into their own cluster so only the width of the textarea itself is > taken into account. I meant crbug.com/165481, in particular the crbug.com comment field, the effect there seems to be even worse than in the video..
Basically, make sure it works well on http://jsbin.com/100percent-textarea/1 (and by works well I mean you don't have to keep panning horizontally). I'll check out crbug in a moment, but I suspect the problem there is that the textarea is narrower than the cluster, the possibility I mentioned in my previous comment.
https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... File LayoutTests/fast/text-autosizing/textarea-fontsize-change.html (right): https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:12: .largersize{font-size: 1.1em} On 2013/04/26 12:24:57, johnme wrote: > Unused? Done. https://codereview.chromium.org/14146029/diff/3005/LayoutTests/fast/text-auto... LayoutTests/fast/text-autosizing/textarea-fontsize-change.html:27: <textarea rows=8 cols=85> On 2013/04/26 12:24:57, johnme wrote: > Nit: it's more standard to always put quotes around attributes. Done.
On 2013/04/26 13:30:14, johnme wrote: > Basically, make sure it works well on http://jsbin.com/100percent-textarea/1 > (and by works well I mean you don't have to keep panning horizontally). > > I'll check out crbug in a moment, but I suspect the problem there is that the > textarea is narrower than the cluster, the possibility I mentioned in my > previous comment. Yes, good point, with the "not autosize"-fix you still have to pan horizontally. It doesn't look too bad, but maybe the autosize option is better as you will not need to pan that much (there may still be some small panning needed). I have uploaded a new patch, taking into account your previous comments about making textareas a cluster and autosizing them by default. It does work for both crbug and the jsbin examples and looks pretty consistent.
On 2013/04/26 12:24:57, johnme wrote: > Code seems good (minor nits). I was originally thinking it might make more sense > to always autosize them (rather than never autosizing them). > > Aurimas, what do you think would be best here? Essentially, how well does Clank > cope with really wide textareas? If we never autosize textareas, then there > could be textareas where once you've zoomed in such that the text you're typing > is legible, you can't see the full width of the textarea and you have to > constantly pan horizontally, which sounds annoying. > > If instead we always autosized textareas, then once you'd zoomed to fit the > textarea to the screen, the contents of the textarea would be legible and you > wouldn't need to pan horizontally. The text in the textarea would probably be > larger than text in other form elements though. > > Either way though, being consistent is much better than the current state, so > even if we decide to always autosize them it probably makes sense to land this > patch as an interim fix. Chrome for Android is pretty difficult to use when editing large <textarea> elements. What is the current (before this patch) logic on autosizing textareas?
On 2013/04/26 18:23:08, aurimas wrote: > On 2013/04/26 12:24:57, johnme wrote: > > Code seems good (minor nits). I was originally thinking it might make more > sense > > to always autosize them (rather than never autosizing them). > > > > Aurimas, what do you think would be best here? Essentially, how well does > Clank > > cope with really wide textareas? If we never autosize textareas, then there > > could be textareas where once you've zoomed in such that the text you're > typing > > is legible, you can't see the full width of the textarea and you have to > > constantly pan horizontally, which sounds annoying. > > > > If instead we always autosized textareas, then once you'd zoomed to fit the > > textarea to the screen, the contents of the textarea would be legible and you > > wouldn't need to pan horizontally. The text in the textarea would probably be > > larger than text in other form elements though. > > > > Either way though, being consistent is much better than the current state, so > > even if we decide to always autosize them it probably makes sense to land this > > patch as an interim fix. > > Chrome for Android is pretty difficult to use when editing large <textarea> > elements. What is the current (before this patch) logic on autosizing textareas? The main problem with the current approach is that it can oscillate when a lot of text is entered (more than 4 lines), which is really annoying. This patch fixes that, although it is not directly clear which approach is better, either keep textareas not autosized or autosize them always. The latter seems to be more consistent and behaves without panning on the jsbin website John mentioned earlier.
Cool, I like the new patch. https://codereview.chromium.org/14146029/diff/13001/LayoutTests/fast/text-aut... File LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html (right): https://codereview.chromium.org/14146029/diff/13001/LayoutTests/fast/text-aut... LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html:17: <textarea rows="12" style="width: 100%; font-size: 2.05rem"> This seems surprisingly small. I was expecting a value closer to 800/320 = 2.5 (though it probably won't be exactly 2.5 due to the margins/borders of the textarea). Do you know how this got calculated? https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:368: || containerIsTextArea(renderer); Please add this as a condition in isIndependentDescendant instead of here. This is just a convenience method; isIndependentDescendant has all the logic for determining what is a cluster, except for the width-based checks which are done in the other two methods. https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:442: bool TextAutosizer::containerIsTextArea(const RenderBlock* container) Nit: This method is so simple, that you might as well just inline it in the two places it gets used. https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:488: // lines of text). This is to ensure that the text does not suddenly gets Nit: s/gets/get/ https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:497: if (containerIsTextArea(clusterInfos[i].root)) Nit: would be nice to move this check to the start of the for loop, so that when it is a textarea we early-out rather than doing the expensive call to measureDescendantTextWidth.
uploaded new patch https://codereview.chromium.org/14146029/diff/13001/LayoutTests/fast/text-aut... File LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html (right): https://codereview.chromium.org/14146029/diff/13001/LayoutTests/fast/text-aut... LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html:17: <textarea rows="12" style="width: 100%; font-size: 2.05rem"> On 2013/04/29 11:11:26, johnme wrote: > This seems surprisingly small. I was expecting a value closer to 800/320 = 2.5 > (though it probably won't be exactly 2.5 due to the margins/borders of the > textarea). Do you know how this got calculated? It seems that the default font-size for the text area is smaller that that of body text. If I put an explicit font-size: 16px, then problem disappears and the factor becomes 2.5... https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:368: || containerIsTextArea(renderer); On 2013/04/29 11:11:26, johnme wrote: > Please add this as a condition in isIndependentDescendant instead of here. This > is just a convenience method; isIndependentDescendant has all the logic for > determining what is a cluster, except for the width-based checks which are done > in the other two methods. Done. https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:442: bool TextAutosizer::containerIsTextArea(const RenderBlock* container) On 2013/04/29 11:11:26, johnme wrote: > Nit: This method is so simple, that you might as well just inline it in the two > places it gets used. Done. https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:488: // lines of text). This is to ensure that the text does not suddenly gets On 2013/04/29 11:11:26, johnme wrote: > Nit: s/gets/get/ Done. https://codereview.chromium.org/14146029/diff/13001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:497: if (containerIsTextArea(clusterInfos[i].root)) On 2013/04/29 11:11:26, johnme wrote: > Nit: would be nice to move this check to the start of the for loop, so that when > it is a textarea we early-out rather than doing the expensive call to > measureDescendantTextWidth. Done.
LGTM (with nit). https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... File LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html (right): https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html:18: The text inside this textarea should be autosized. Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Nit: like the other tests, it would be nice to explicitly say the expected size, e.g.: "The text inside this textarea should be autosized to 40px computed font-size (16 * 800/320)."
https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... File LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html (right): https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html:18: The text inside this textarea should be autosized. Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. On 2013/05/01 11:34:39, johnme wrote: > Nit: like the other tests, it would be nice to explicitly say the expected size, > e.g.: > > "The text inside this textarea should be autosized to 40px computed font-size > (16 * 800/320)." Done.
On 2013/05/02 11:12:12, timvolodine wrote: > https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... > File LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html > (right): > > https://codereview.chromium.org/14146029/diff/25001/LayoutTests/fast/text-aut... > LayoutTests/fast/text-autosizing/textarea-fontsize-change-expected.html:18: The > text inside this textarea should be autosized. Lorem ipsum dolor sit amet, > consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et > dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco > laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in > reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. > Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia > deserunt mollit anim id est laborum. > On 2013/05/01 11:34:39, johnme wrote: > > Nit: like the other tests, it would be nice to explicitly say the expected > size, > > e.g.: > > > > "The text inside this textarea should be autosized to 40px computed font-size > > (16 * 800/320)." > > Done. also ran the comparison script for ~2000 top websites, there were no significant impacts on these sites, in fact most of them were identical.
https://codereview.chromium.org/14146029/diff/31001/Source/core/rendering/Tex... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/14146029/diff/31001/Source/core/rendering/Tex... Source/core/rendering/TextAutosizer.cpp:356: || renderer->isTextArea(); I think you should also check for elements with contenteditable="true" too in this case. AFAICT checking renderer->style()->userModify() != READ_ONLY should do the trick.
LGTM, you can add the missing check in a follow-up change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14146029/36001
Message was sent while issue was closed.
Change committed as 149645 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
