Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
306923003:
Use webkit-margin-end for details element to get same margin for both ltr and rtl (Closed)
|
Created:
6 years, 6 months ago by Habib Virji Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUse webkit-margin-end for details element to get same margin for both ltr and rtl
RenderDetailMarker needs both left and right margin to get rendered with same margin. Updated with webkit-margin-end to get the same margin result for both ltr and rtl
R=eseidel, leviw, tkent
BUG=335075
TEST=Existing details-writing-mode covers test for showing text in RTL box, with the patch rtl box now shows margin which is currently missing.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176016
Patch Set 1 #
Total comments: 3
Patch Set 2 : RenderDetailsMarker marker box to include margin right and left #
Total comments: 1
Patch Set 3 : Replaced margin-right with webkit-margin-end and removed horizontal check in RenderDetailsMarker #Patch Set 4 : Added test in TextExpectation and RenderDetailsMarker to include marginStart for placing a marker b… #Patch Set 5 : Removed changes from RenderDetailsMarker #Messages
Total messages: 26 (0 generated)
This sounds very reasonable. I presume the new behavior matches other browsers?
https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) I don't understand why details markers are special in this code? This feels like the wrong layer to know about that kind of thing.
Thanks Eric for having a look. Cannot compare with other browsers because FF and IE still does not support it. Reason for omitting other list elements was it was breaking current inline list tests. It breaks the start of the bullets margin but does not effect its text position. The reason of difference is probably details element arrow, which is placed differently in comparison to bullets. That's the reason for differentiating list element inside and details element.
https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) @eseidel: RenderListItem::updateMarkerLocation (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) already calculates offset hence in placeBoxRangeInInlineDirection, I am omitting list item and just calculating for details marker. If marginRight gets added it leaves the list marker at wrong location. Do you reckon to have something similar as updateMarkerLocation for details marker too?
@leviw any comment on my previous comment. Thanks
https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) I'd either expect the list and details code to be shared or the RenderDetailsMaker code to handle this itself. I'm with Eric: This isn't the right layer for this.
On 2014/06/05 22:25:43, leviw wrote: > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > File Source/core/rendering/InlineFlowBox.cpp (right): > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) > I'd either expect the list and details code to be shared or the > RenderDetailsMaker code to handle this itself. I'm with Eric: This isn't the > right layer for this. Thanks for reviewing levi/eseidel. Updated code not to change InlineFlowBox, but instead DetailsMarker to include margin right and left. The image shows with the change horizontal rtl matching margin with horizontal ltr: https://chromium.googlecode.com/issues/attachment?aid=3350750002000&name=deta.... Previous horizontal rtl behavior can be found here: https://chromium.googlecode.com/issues/attachment?aid=3350750002001&name=deta....
On 2014/06/05 22:25:43, leviw wrote: > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > File Source/core/rendering/InlineFlowBox.cpp (right): > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) > I'd either expect the list and details code to be shared or the > RenderDetailsMaker code to handle this itself. I'm with Eric: This isn't the > right layer for this. Thanks for reviewing levi/eseidel. Updated code not to change InlineFlowBox, but instead DetailsMarker to include margin right and left. The image shows with the change horizontal rtl matching margin with horizontal ltr: https://chromium.googlecode.com/issues/attachment?aid=3350750002000&name=deta.... Previous horizontal rtl behavior can be found here: https://chromium.googlecode.com/issues/attachment?aid=3350750002001&name=deta....
On 2014/06/10 at 16:15:33, habib.virji wrote: > On 2014/06/05 22:25:43, leviw wrote: > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > > File Source/core/rendering/InlineFlowBox.cpp (right): > > > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > > Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) > > I'd either expect the list and details code to be shared or the > > RenderDetailsMaker code to handle this itself. I'm with Eric: This isn't the > > right layer for this. > > Thanks for reviewing levi/eseidel. > > Updated code not to change InlineFlowBox, but instead DetailsMarker to include margin right and left. > > The image shows with the change horizontal rtl matching margin with horizontal ltr: https://chromium.googlecode.com/issues/attachment?aid=3350750002000&name=deta.... Previous horizontal rtl behavior can be found here: https://chromium.googlecode.com/issues/attachment?aid=3350750002001&name=deta.... I get 400's trying to look at the images. Can you at least upload a version with results for the platform you're running on?
https://codereview.chromium.org/306923003/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderDetailsMarker.cpp (right): https://codereview.chromium.org/306923003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderDetailsMarker.cpp:138: boxOrigin.move(borderLeft() + paddingLeft(), borderTop() + paddingTop()); This seems wrong. Why do you only consider margin in the horizontal case?
On 2014/06/10 19:06:31, leviw wrote: > On 2014/06/10 at 16:15:33, habib.virji wrote: > > On 2014/06/05 22:25:43, leviw wrote: > > > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > > > File Source/core/rendering/InlineFlowBox.cpp (right): > > > > > > > https://codereview.chromium.org/306923003/diff/1/Source/core/rendering/Inline... > > > Source/core/rendering/InlineFlowBox.cpp:426: if (isDetailsMarker) > > > I'd either expect the list and details code to be shared or the > > > RenderDetailsMaker code to handle this itself. I'm with Eric: This isn't the > > > right layer for this. > > > > Thanks for reviewing levi/eseidel. > > > > Updated code not to change InlineFlowBox, but instead DetailsMarker to include > margin right and left. > > > > The image shows with the change horizontal rtl matching margin with horizontal > ltr: > https://chromium.googlecode.com/issues/attachment?aid=3350750002000&name=deta.... > Previous horizontal rtl behavior can be found here: > https://chromium.googlecode.com/issues/attachment?aid=3350750002001&name=deta.... > > I get 400's trying to look at the images. Can you at least upload a version with > results for the platform you're running on? Sorry. The images are at the crbug.com/335075. Could not upload images via git cl.
On 2014/06/10 19:06:58, leviw wrote: > https://codereview.chromium.org/306923003/diff/40001/Source/core/rendering/Re... > File Source/core/rendering/RenderDetailsMarker.cpp (right): > > https://codereview.chromium.org/306923003/diff/40001/Source/core/rendering/Re... > Source/core/rendering/RenderDetailsMarker.cpp:138: boxOrigin.move(borderLeft() + > paddingLeft(), borderTop() + paddingTop()); > This seems wrong. Why do you only consider margin in the horizontal case? Reason being it was already not done for vertical ltr. You can see at crbug.com/335075 image thats why did not update vertical rtl case. Was going to ask should do for vertical ltr and rtl scenario.
Can we just replace margin-right with -webkit-margin-end in the UA stylesheet of summary::-webkit-details-marker?
On 2014/06/10 23:08:43, tkent wrote: > Can we just replace margin-right with -webkit-margin-end in the UA stylesheet of > summary::-webkit-details-marker? Thanks tkent. Updated with webkit-margin-end corrected for both vertical and right margin and almost all scenarios. Still kept rightMargin/leftMargin addition as horizontal top to bottom rtl scenario was not working.
Would you show the test result image with the html.css change without the RenderDetailsMarker.cpp change please? Adding margin in RenderDetailsMarker is really weird.
On 2014/06/11 08:36:01, tkent wrote: > Would you show the test result image with the html.css change without the > RenderDetailsMarker.cpp change please? Adding margin in RenderDetailsMarker is > really weird. @tkent: I have upload images at crbug.com/335075 and it includes diff image showing what is the diff between actual and expected image. Horizontal top to bottom rtl, is not updated.
On 2014/06/11 08:52:07, Habib Virji wrote: > @tkent: I have upload images at crbug.com/335075 and it includes diff image > showing what is the diff between actual and expected image. Horizontal top to > bottom rtl, is not updated. Thanks. I understand the situation. Clicking the marker of the horizontal-tb rtl case fixes the position of the marker. I guess there is a bug not in RenderDetailsMarker.
On 2014/06/11 09:11:37, tkent wrote: > On 2014/06/11 08:52:07, Habib Virji wrote: > > @tkent: I have upload images at crbug.com/335075 and it includes diff image > > showing what is the diff between actual and expected image. Horizontal top to > > bottom rtl, is not updated. > > Thanks. I understand the situation. > > Clicking the marker of the horizontal-tb rtl case fixes the position of the > marker. I guess there is a bug not in RenderDetailsMarker. Thanks, did try debugging further. - It appears it works correct without changes in RenderDetailsMarker if detail element is not in table. - Specifically it is in second row, horizontal-tb goes wrong. If ltr detail element is placed too, it goes wrong. In firstRow same rtl works fine. (Could not understand this peculiarity) - marginStart fixes the offset correctly. Could not find anything specific to horizontal-tb, as all scenarios considers horizontal-tb and horizontal-bt together.
On 2014/06/11 16:26:27, Habib Virji wrote: > Thanks, did try debugging further. > - It appears it works correct without changes in RenderDetailsMarker if detail > element is not in table. > - Specifically it is in second row, horizontal-tb goes wrong. If ltr detail > element is placed too, it goes wrong. In firstRow same rtl works fine. (Could > not understand this peculiarity) > - marginStart fixes the offset correctly. Could not find anything specific to > horizontal-tb, as all scenarios considers horizontal-tb and horizontal-bt > together. Thank your for the investigation. It seems we have a bug in table layout. I think you may proceed this CL with only html.css + TestExpectation changes first, then work on the table issue later.
Isn't the table issue related to https://code.google.com/p/chromium/issues/detail?id=76858 ? Just a guess
On 2014/06/12 00:42:32, tkent wrote: > On 2014/06/11 16:26:27, Habib Virji wrote: > > Thanks, did try debugging further. > > - It appears it works correct without changes in RenderDetailsMarker if detail > > element is not in table. > > - Specifically it is in second row, horizontal-tb goes wrong. If ltr detail > > element is placed too, it goes wrong. In firstRow same rtl works fine. (Could > > not understand this peculiarity) > > - marginStart fixes the offset correctly. Could not find anything specific to > > horizontal-tb, as all scenarios considers horizontal-tb and horizontal-bt > > together. > > Thank your for the investigation. It seems we have a bug in table layout. > > I think you may proceed this CL with only html.css + TestExpectation changes > first, then work on the table issue later. Thanks. Have uploaded without changes to RenderDetailsMarker and will start looking into 76858/278351 and fix table issue.
On 2014/06/12 07:03:06, ebraminio wrote: > Isn't the table issue related to > https://code.google.com/p/chromium/issues/detail?id=76858 ? Just a guess Yes ebraminio,probably yes and also 278351 looks like a close match with the issue I saw with the bug.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/306923003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
Message was sent while issue was closed.
Change committed as 176016 |
