Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
278603002:
Centering text inside a button set to display flex with justify-content: center is impossible (Closed)
|
Created:
6 years, 7 months ago by harpreet.sk Modified:
6 years, 7 months ago CC:
blink-reviews, blink-reviews-rendering, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionCentering text inside a button set to display flex with justify-content: center is impossible
When we set button display to flex and set justify-content: center
chrome does not render text in center.
This patch removes this bug by setting justify-content property with
value specified in style for the flexbox.
BUG=344733
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174761
Patch Set 1 #Patch Set 2 : New solution considering text-align property #Patch Set 3 : Fixup for Linux and Windows LayoutTests #Patch Set 4 : Fixup for Mac Layout tests #
Total comments: 4
Patch Set 5 : Modified solution that do not break existing LayoutTests #Patch Set 6 : New patchset based on comments #
Total comments: 4
Patch Set 7 : Addresses the changes asked in patch set 6 #
Total comments: 8
Patch Set 8 : Addressing comments for LayoutTests and fix for layouttest that get failed #
Messages
Total messages: 31 (0 generated)
PTAL
On 2014/05/08 14:54:53, harpreet.sk wrote: > PTAL This will break the case when text-align: left is specified. http://jsfiddle.net/LdSS9/ The text should be aligned to the left but after your CL the text is centered.
On 2014/05/09 02:35:48, keishi1 wrote: > On 2014/05/08 14:54:53, harpreet.sk wrote: > > PTAL > > This will break the case when text-align: left is specified. > http://jsfiddle.net/LdSS9/ > The text should be aligned to the left but after your CL the text is centered. Thanks for your comment. I have modified the patch to set margins based on text-align property. Please have a look. I will upload the fix of layout test that are getting fail because of my patch behavior once you are okay with my approach.
I think RenderButton.cpp looks good. Please make this pass the layout tests.
https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderButton.cpp (left): https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:92: // RenderBlock::createAnonymousBlock creates a new RenderStyle, so this is You need to keep this comment. https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:94: innerStyle->setFlexGrow(1.0f); Why remove this line?
https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderButton.cpp (left): https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:92: // RenderBlock::createAnonymousBlock creates a new RenderStyle, so this is On 2014/05/13 07:58:27, cbiesinger wrote: > You need to keep this comment. I will keep this comment in next patch. https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:94: innerStyle->setFlexGrow(1.0f); On 2014/05/13 07:58:27, cbiesinger wrote: > Why remove this line? Actually i removed this line as this line will disable the effects of setting left or right margins to be auto. I think we don't require this line in case of setting auto margins for left or right.
On 2014/05/13 09:30:44, harpreet.sk wrote: > https://codereview.chromium.org/278603002/diff/60001/Source/core/rendering/Re... > Source/core/rendering/RenderButton.cpp:94: innerStyle->setFlexGrow(1.0f); > On 2014/05/13 07:58:27, cbiesinger wrote: > > Why remove this line? > > Actually i removed this line as this line will disable the effects of setting > left or right margins to be auto. I think we don't require this line in case of > setting auto margins for left or right. My concern is that this will change the behavior in the default case. For example, I see that you are marking a lot of tests as requiring rebaselining -- why is the new behavior more correct? (and why manual rebaseline?)
On 2014/05/14 12:49:19, cbiesinger wrote: > My concern is that this will change the behavior in the default case. For > example, I see that you are marking a lot of tests as requiring rebaselining -- > why is the new behavior more correct? (and why manual rebaseline?) Actually the property of text-align is not getting applied in the case when display:flex with current behavior. For eg if you set text-align:right and set dispaly:flex it will still show text to be align on left. In my solution i introduced auto margins so that for the case of flex it works fine. Even it's working fine in default case. Actually i had done the rebaseline because there is little pixels difference coming with my solution from the expected solution.
On 2014/05/14 13:33:30, harpreet.sk wrote: > On 2014/05/14 12:49:19, cbiesinger wrote: > > > My concern is that this will change the behavior in the default case. For > > example, I see that you are marking a lot of tests as requiring rebaselining > -- > > why is the new behavior more correct? (and why manual rebaseline?) > > > Actually the property of text-align is not getting applied in the case when > display:flex with current behavior. For eg if you set text-align:right and set > dispaly:flex it will still show text to be align on left. In my solution i > introduced auto margins so that for the case of flex it works fine. Even it's > working fine in default case. It changes the behavior of the default case. > Actually i had done the rebaseline because there is little pixels difference > coming with my solution from the expected solution. I understand that, but why is there a pixel difference? There shouldn't be one. If there is one, why is the new layout correct?
I have uploaded a new patch that set the margins to be auto only in case of display:flex. For default case it do what it does earlier. The new solution does not break the existing layout test. Please have a look.
PING
Sorry, I was on a plane. I will take a look today.
So, I previously misunderstood what this bug was about. The text-align code in RenderButton misled me -- in fact the bug report was just asking for justify-contents to work on <button style="display:flex">, which is very reasonable. Now that I understand it correctly, here's a few comments: - The testcase is written incorrectly - as written it tests that justify-content has *no* effect on a <button> (!) - It seems that the right way to fix the original bug is to add a call to innerStyle->setJustifyContents, just like it currently calls setFlexDirection, and probably the same for setFlexWrap, setAlignItems and setAlignContents. - It is true that text-align doesn't work on a button with display:flex. This doesn't work in Firefox either, and neither does it work on a non-button flexbox. It is not clear to me now that it should work. I'm sorry that I keep making you go in circles :( But I really appreciate that you're fixing flexbox bugs! It needs it :)
On 2014/05/17 00:04:23, cbiesinger wrote: > So, I previously misunderstood what this bug was about. The text-align code in > RenderButton misled me -- in fact the bug report was just asking for > justify-contents to work on <button style="display:flex">, which is very > reasonable. The bug not only ask for justify-content property to work on but also it ask for centering of text in the case when display:flex and justify-content property is not applied. See the second case in the following link in chrome. It shows text on left for second and third case. http://jsfiddle.net/UyXR9/1/ By setting innerStyle->setJustifyContent(style()->justifyContent()) will resolve the third case but not the second case. > > Now that I understand it correctly, here's a few comments: > - The testcase is written incorrectly - as written it tests that justify-content > has *no* effect on a <button> (!) > - It seems that the right way to fix the original bug is to add a call to > innerStyle->setJustifyContents, just like it currently calls setFlexDirection, > and probably the same for setFlexWrap, setAlignItems and setAlignContents. > - It is true that text-align doesn't work on a button with display:flex. This > doesn't work in Firefox either, and neither does it work on a non-button > flexbox. It is not clear to me now that it should work. In the link mention http://jsfiddle.net/UyXR9/1/ i tried to set text-align:right in button css and run it in firefox. It align the text to right in all the 3 cases. I think firefox obey the text-align property in the case of flexbox too. What's your opinion about it ? I think setting innerStyle->setJustifyContent(style()->justifyContent()) will not be sufficient as we need something else to make text center for the second case mention in the link above. Therefore i thought of using auto-margins based on text-align property.
PING
On 2014/05/19 08:34:35, harpreet.sk wrote: > PING (I don't usually work on weekends.) justify-content and text-align seem different enough that they should get a separate patch. Certainly, they both should get a testcase. > In the link mention http://jsfiddle.net/UyXR9/1/ i tried to set text-align:right > in button css and run it in firefox. It align the text to right in all the 3 > cases. I think firefox obey the text-align property in the case of flexbox too. > What's your opinion about it ? Huh, that is interesting because I tried this testcase: http://jsfiddle.net/Nw49s/1/ The text is not aligned in this case (in Firefox/Chrome). I will have to study the Firefox code. I'm happy to review a patch in the meantime that handles the justify-content case.
On 2014/05/19 17:11:43, cbiesinger wrote: > ... > > Huh, that is interesting because I tried this testcase: > http://jsfiddle.net/Nw49s/1/ http://jsfiddle.net/Nw49s/2/ That's because text nodes inside a flex box are wrapped in anonymous blocks that become flex items. The items are collapsed to the side since they're not flexing which means you can't align to the right of the flex box.
On 2014/05/19 21:09:44, esprehn wrote: > On 2014/05/19 17:11:43, cbiesinger wrote: > > ... > > > > Huh, that is interesting because I tried this testcase: > > http://jsfiddle.net/Nw49s/1/ > > http://jsfiddle.net/Nw49s/2/ > > That's because text nodes inside a flex box are wrapped in anonymous blocks that > become flex items. The items are collapsed to the side since they're not flexing > which means you can't align to the right of the flex box. Yes, but why does Firefox allow text-align:right to work on a button with display:flex? (what I was trying to get at) http://jsfiddle.net/Nw49s/3/
On 2014/05/19 21:15:33, cbiesinger wrote: > On 2014/05/19 21:09:44, esprehn wrote: > > On 2014/05/19 17:11:43, cbiesinger wrote: > > > ... > > > > > > Huh, that is interesting because I tried this testcase: > > > http://jsfiddle.net/Nw49s/1/ > > > > http://jsfiddle.net/Nw49s/2/ > > > > That's because text nodes inside a flex box are wrapped in anonymous blocks > that > > become flex items. The items are collapsed to the side since they're not > flexing > > which means you can't align to the right of the flex box. > > Yes, but why does Firefox allow text-align:right to work on a button with > display:flex? (what I was trying to get at) > http://jsfiddle.net/Nw49s/3/ Because Firefox doesn't respect the display: flex, you get a special renderer. http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstruct... http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsHTMLButtonContro...
On 2014/05/19 21:27:51, esprehn wrote: > On 2014/05/19 21:15:33, cbiesinger wrote: > > On 2014/05/19 21:09:44, esprehn wrote: > > > On 2014/05/19 17:11:43, cbiesinger wrote: > > > > ... > > > > > > > > Huh, that is interesting because I tried this testcase: > > > > http://jsfiddle.net/Nw49s/1/ > > > > > > http://jsfiddle.net/Nw49s/2/ > > > > > > That's because text nodes inside a flex box are wrapped in anonymous blocks > > that > > > become flex items. The items are collapsed to the side since they're not > > flexing > > > which means you can't align to the right of the flex box. > > > > Yes, but why does Firefox allow text-align:right to work on a button with > > display:flex? (what I was trying to get at) > > http://jsfiddle.net/Nw49s/3/ > > Because Firefox doesn't respect the display: flex, you get a special renderer. > > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstruct... > > http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsHTMLButtonContro... Oh, heh. OK! So the text-align part of this patch is not a bug and should not be touched. However the justify-content part should be fixed.
I have uploaded a new patch fixing justify-content. Please have a look.
https://codereview.chromium.org/278603002/diff/90001/LayoutTests/fast/forms/b... File LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html (right): https://codereview.chromium.org/278603002/diff/90001/LayoutTests/fast/forms/b... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:5: button { I think you could make this a reftest, if you style the button right. Try: <button style="border: 0; background-color: white; padding: 0; font-family: sans-serif; font-size: initial;">safd</button> And then in a reftest, the same except use a <div> and additionally display: inline-flex. https://codereview.chromium.org/278603002/diff/90001/Source/core/rendering/Re... File Source/core/rendering/RenderButton.cpp (right): https://codereview.chromium.org/278603002/diff/90001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:100: innerStyle->setJustifyContent(style()->justifyContent()); Can you add these properties as well: setFlexWrap, setAlignItems and setAlignContents.
Please have another look. https://codereview.chromium.org/278603002/diff/90001/LayoutTests/fast/forms/b... File LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html (right): https://codereview.chromium.org/278603002/diff/90001/LayoutTests/fast/forms/b... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:5: button { On 2014/05/21 00:18:10, cbiesinger wrote: > I think you could make this a reftest, if you style the button right. Try: > <button style="border: 0; background-color: white; padding: 0; font-family: > sans-serif; font-size: initial;">safd</button> > > And then in a reftest, the same except use a <div> and additionally display: > inline-flex. Done. https://codereview.chromium.org/278603002/diff/90001/Source/core/rendering/Re... File Source/core/rendering/RenderButton.cpp (right): https://codereview.chromium.org/278603002/diff/90001/Source/core/rendering/Re... Source/core/rendering/RenderButton.cpp:100: innerStyle->setJustifyContent(style()->justifyContent()); On 2014/05/21 00:18:10, cbiesinger wrote: > Can you add these properties as well: > > setFlexWrap, setAlignItems and setAlignContents. Done.
lgtm thanks! an owner will have to give the official lgtm - ojan?
lgtm Feel free to commit after addressing my test nits. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html (right): https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:1: <!doctype html> Nit: We usually use <!DOCTYPE html>. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:3: <head> Nit: we usually leave out the html, head and body elements if the test doesn't explicitly require them. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:24: <h4>Test for BUG=344733: Centering text inside a button set to display flex and justify-content: center is impossible</h4> Nit: If you're going to include a bug, use a URL crbug.com/344733. It makes it more friendly to people who are working on this. That said, I'd just leave out the bug information. That's contained in the history (i.e. it's in the change description for this patch, which people can easily get to by looking at the log for this file). https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:25: <button class="flex flex-center">text with justify-content: center</button> Nit: I'd drop the classnames entirely. You can merge all the styles into the single button rule. It makes it so that there's slightly less going on in the test case for someone reading it to figure out.
https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html (right): https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:1: <!doctype html> On 2014/05/23 19:52:39, ojan wrote: > Nit: We usually use <!DOCTYPE html>. Done. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:3: <head> On 2014/05/23 19:52:39, ojan wrote: > Nit: we usually leave out the html, head and body elements if the test doesn't > explicitly require them. Done. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:24: <h4>Test for BUG=344733: Centering text inside a button set to display flex and justify-content: center is impossible</h4> On 2014/05/23 19:52:39, ojan wrote: > Nit: If you're going to include a bug, use a URL crbug.com/344733. It makes it > more friendly to people who are working on this. > > That said, I'd just leave out the bug information. That's contained in the > history (i.e. it's in the change description for this patch, which people can > easily get to by looking at the log for this file). Done. https://codereview.chromium.org/278603002/diff/110001/LayoutTests/fast/forms/... LayoutTests/fast/forms/button-set-display-flex-justifyContent-center.html:25: <button class="flex flex-center">text with justify-content: center</button> On 2014/05/23 19:52:39, ojan wrote: > Nit: I'd drop the classnames entirely. You can merge all the styles into the > single button rule. It makes it so that there's slightly less going on in the > test case for someone reading it to figure out. Done.
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/278603002/120001
Message was sent while issue was closed.
Change committed as 174761 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
