|
Description
cathiechen
2018-12-21 04:05:58 PST
Created attachment 357937 [details]
Patch
Comment on attachment 357937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357937&action=review > Source/WebCore/rendering/RenderListItem.h:65 > + bool needBlockDirectionAlign() { return m_needBlockDirectionAlign; } This should be const. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:79 > + return; You probably want to check this before cloning. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:170 > + else The indenting is weird here, does check-webkit-style say anything? Comment on attachment 357937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357937&action=review >> Source/WebCore/rendering/RenderListItem.h:65 >> + bool needBlockDirectionAlign() { return m_needBlockDirectionAlign; } > > This should be const. done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:79 >> + return; > > You probably want to check this before cloning. done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:170 >> + else > > The indenting is weird here, does check-webkit-style say anything? Nope, it made me delete the one-line braces at first. Created attachment 357938 [details]
Patch
Attachment 357938 [details] did not pass style-queue:
ERROR: LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
Total errors found: 12 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 357939 [details]
Patch
Created attachment 357940 [details]
Patch
Comment on attachment 357940 [details] Patch Attachment 357940 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10506751 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/block/float/float-not-removed-crash.html imported/blink/fast/lists/list-inside-columns-crash.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 357942 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 357940 [details] Patch Attachment 357940 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10506865 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/block/float/float-not-removed-crash.html imported/blink/fast/lists/list-inside-columns-crash.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 357943 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 357940 [details] Patch Attachment 357940 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10506940 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/block/float/float-not-removed-crash.html imported/blink/fast/lists/list-inside-columns-crash.html fast/multicol/multicol-li-crash.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 357944 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 357940 [details] Patch Attachment 357940 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10506911 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/block/float/float-not-removed-crash.html imported/blink/fast/lists/list-inside-columns-crash.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 357945 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 357940 [details] Patch Attachment 357940 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10506912 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/forms/form-hides-table.html fast/block/float/float-not-removed-crash.html imported/blink/fast/lists/list-inside-columns-crash.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 357946 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 357940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357940&action=review Thanks for the patch, I did an initial review with lots of comments. I didn't have time to look into the tests yet, but it'd be nice to use WPT instead to share them with Chromium and Firefox. This was fixed in Chromium first, could you link the Chromium bug in "See also" field? Thanks. > Source/WebCore/ChangeLog:9 > + is marker has been added as the child of this overflow block. And the Nit: "is that marker has been added" > Source/WebCore/ChangeLog:11 > + sibilng of a block child. To fix these two kinds of issue: Is there a way to divide these two fixes in two separated patches, or the code is the same for both things? I have the feeling that we're fixing two things in this patch one releated to line breaks and another related to overflow: hidden. > Source/WebCore/ChangeLog:12 > + 1. Create a zero-height anonymouse parent for marker(to eleminate the Typo: s/anonymouse/anonymous/ Nit: Missing space before "(" > Source/WebCore/ChangeLog:15 > + has been layout (marker has to adjust itselt to the content of li) Typo: s/has/have/ > Source/WebCore/ChangeLog:30 > + (WebCore::RenderListItem::alignMarkerInBlockDirection): As this method is quite complex, it'd be nice to have a summary of what it does here. > Source/WebCore/ChangeLog:36 > + (WebCore::RenderTreeBuilder::List::updateItemMarker): And the same in this case. > Source/WebCore/rendering/RenderListItem.cpp:445 > +// Align marker_inline_box in block direction according to line_box_root's baseline. Nit: s/marker_inline_box/markerInlineBox/ Also I guess s/line_box_root/RootInlineBox/ ? > Source/WebCore/rendering/RenderListItem.cpp:460 > + } Nit: This variable could be initialized like: backToOriginalBaseline = !baselineProvider || baselineProvider->isWritingModeRoot(); If this is true couldn't we just return and avoid doing anything in this method? Aren't we keeping the old behavior? Or are we doing something else? > Source/WebCore/rendering/RenderListItem.cpp:463 > + RootInlineBox& markerRoot = markerInlineBox->root(); Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at least? > Source/WebCore/rendering/RenderListItem.cpp:465 > + // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker. Nit: "marker_" I guess that name might come from Blink and it should be "m_marker" here. > Source/WebCore/rendering/RenderListItem.cpp:466 > + if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot) We don't need two ifs, we can combine both in just one if. > Source/WebCore/rendering/RenderListItem.cpp:473 > + offset = box->firstLineBaseline().value(); Wouldn't we need "valueOr()" to be safe here? Nit: Couldn't we write this like: offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value(); > Source/WebCore/rendering/RenderListItem.cpp:477 > + baselineProvider = m_marker->containingBlock(); Again I guess we're sure containingBlock() is never null here. > Source/WebCore/rendering/RenderListItem.cpp:482 > + if (offset != -1) { I'd change this for something like: if (offset == -1) return; > Source/WebCore/rendering/RenderListItem.cpp:489 > + // BaselinePosition is workable when marker is an image. What do you mean here by "is workable"? > Source/WebCore/rendering/RenderListItem.cpp:491 > + // So use markerFontMetrics.Ascent when marker is text. Nit: s/Ascent/ascent/ > Source/WebCore/rendering/RenderListItem.cpp:492 > + if (m_marker->isImage()) I see that RenderListMarker has a method baselinePosition() why we cannot use that? I guess it provides wrong information, then why we don't modify it? > Source/WebCore/rendering/RenderListItem.cpp:496 > + const FontMetrics& markerFontMetrics = m_marker->style().fontMetrics(); > + offset -= markerFontMetrics.ascent(markerRoot.baselineType()); Nit: I'd write this as: offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType()); > Source/WebCore/rendering/RenderListItem.h:64 > + void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; } Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/ > Source/WebCore/rendering/RenderListItem.h:65 > + bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign; } Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/ > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52 > - if (!is<RenderBlock>(child) || is<RenderTable>(child) || is<RenderRubyAsBlock>(child)) > - break; > + if (current.hasOverflowClip()) > + return ¤t; > + > + if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot()))) > + return &downcast<RenderBlock>(child); I don't understand this change very well. For the case of isWritingModeRoot() it was breaking before (so it returned nullptr) and now it's returning the child... Is this the expected bahvavior? Doesn't it affect other cases? > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114 > + // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox. Typo: "Make sure it doesn't share parent" > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120 > + // Prepare for block direction align Nit: Comments should finish in a "dot". > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126 > + // So add IsInside() here. Nit: s/IsInside/isInisde/ > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130 > + forceLogicalHeight(*currentParent, Length(style.logicalHeight())); Isn't enough just calling setLogicalHeigth()? > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133 > + // We need to remove marker here.E.g: <li><span><div>text<div><span></li> Mmmm, what's the expected behavior for this example? > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139 > + forceLogicalHeight(*currentParent, Length(0, Fixed)); Same question than before regarding setLogicalHeight(). > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:152 > + forceLogicalHeight(*markerContainer, Length(0, Fixed)); Ditto. > LayoutTests/ChangeLog:10 > + marker in li + block overflow:hiddlen child, and testing if the acting s/acting/behavior/ > LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1 > +<head></head><body><ul> Tests need to start with <!DOCTYPE html> unless you're explicitly testing quirks mode (which I don't think is the case). Created attachment 358035 [details]
Patch
Comment on attachment 357940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357940&action=review Hi Rego, Thanks very much for the thorough review:) And sorry for the typos. >> Source/WebCore/ChangeLog:9 >> + is marker has been added as the child of this overflow block. And the > > Nit: "is that marker has been added" Done >> Source/WebCore/ChangeLog:11 >> + sibilng of a block child. To fix these two kinds of issue: > > Is there a way to divide these two fixes in two separated patches, or the code is the same for both things? > I have the feeling that we're fixing two things in this patch one releated to line breaks and another related to overflow: hidden. Nope, it couldn't be separated. Logically, this design would fit for all conditions. In this patch, we let li + overflow, flex, etc. use it. >> Source/WebCore/ChangeLog:12 >> + 1. Create a zero-height anonymouse parent for marker(to eleminate the > > Typo: s/anonymouse/anonymous/ > Nit: Missing space before "(" Done >> Source/WebCore/ChangeLog:15 >> + has been layout (marker has to adjust itselt to the content of li) > > Typo: s/has/have/ Done >> Source/WebCore/ChangeLog:30 >> + (WebCore::RenderListItem::alignMarkerInBlockDirection): > > As this method is quite complex, it'd be nice to have a summary of what it does here. Done >> Source/WebCore/ChangeLog:36 >> + (WebCore::RenderTreeBuilder::List::updateItemMarker): > > And the same in this case. Done >> Source/WebCore/rendering/RenderListItem.cpp:445 >> +// Align marker_inline_box in block direction according to line_box_root's baseline. > > Nit: s/marker_inline_box/markerInlineBox/ > Also I guess s/line_box_root/RootInlineBox/ ? Done >> Source/WebCore/rendering/RenderListItem.cpp:460 >> + } > > Nit: This variable could be initialized like: > backToOriginalBaseline = !baselineProvider || baselineProvider->isWritingModeRoot(); > > If this is true couldn't we just return and avoid doing anything in this method? > Aren't we keeping the old behavior? Or are we doing something else? Done. In this case, because marker might be moved to other position at last layout pass. So we have to make sure it back to its original baseline, couldn't just return. >> Source/WebCore/rendering/RenderListItem.cpp:463 >> + RootInlineBox& markerRoot = markerInlineBox->root(); > > Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at least? Done >> Source/WebCore/rendering/RenderListItem.cpp:465 >> + // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker. > > Nit: "marker_" I guess that name might come from Blink and it should be "m_marker" here. Done >> Source/WebCore/rendering/RenderListItem.cpp:466 >> + if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot) > > We don't need two ifs, we can combine both in just one if. Done >> Source/WebCore/rendering/RenderListItem.cpp:473 >> + offset = box->firstLineBaseline().value(); > > Wouldn't we need "valueOr()" to be safe here? > > Nit: Couldn't we write this like: > offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value(); Yes, using value() directly would cause a crash issue. I use Optional in the new patch. RenderBox::firstLineBaseline() is public. RenderBlock::firstLineBaseline() is protected. In order to call it, I have to cast baslineProvider from RenderBlock to RenderBox. RenderBox is the parent class, so it isn't downcast... >> Source/WebCore/rendering/RenderListItem.cpp:477 >> + baselineProvider = m_marker->containingBlock(); > > Again I guess we're sure containingBlock() is never null here. Done >> Source/WebCore/rendering/RenderListItem.cpp:482 >> + if (offset != -1) { > > I'd change this for something like: > if (offset == -1) > return; Use this format in the new patch. if (offset) LayoutUnit offsetValue = offset.value(); >> Source/WebCore/rendering/RenderListItem.cpp:489 >> + // BaselinePosition is workable when marker is an image. > > What do you mean here by "is workable"? I mean RenderListMarker::baselinePosition() return the right value when marker is image. >> Source/WebCore/rendering/RenderListItem.cpp:491 >> + // So use markerFontMetrics.Ascent when marker is text. > > Nit: s/Ascent/ascent/ Done >> Source/WebCore/rendering/RenderListItem.cpp:492 >> + if (m_marker->isImage()) > > I see that RenderListMarker has a method baselinePosition() why we cannot use that? > I guess it provides wrong information, then why we don't modify it? Yes, it return baselinePosition of list item when marker isn't image. This isn't right. Corrected baselinePosition() and lineheight() in the new patch >> Source/WebCore/rendering/RenderListItem.cpp:496 >> + offset -= markerFontMetrics.ascent(markerRoot.baselineType()); > > Nit: I'd write this as: > offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType()); Done >> Source/WebCore/rendering/RenderListItem.h:64 >> + void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; } > > Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/ Done >> Source/WebCore/rendering/RenderListItem.h:65 >> + bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign; } > > Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/ Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52 >> + return &downcast<RenderBlock>(child); > > I don't understand this change very well. > For the case of isWritingModeRoot() it was breaking before (so it returned nullptr) and now it's returning the child... > Is this the expected bahvavior? Doesn't it affect other cases? We use getParentOfFirstLineBox to determine if this list marker should use the new design. And use getParentOfFirstLineBox as the baselineprovider in alignMarkerInBlockDirection(). And yes, for list-style-position:inside cases, this might cause crash issues. The new patch would deal with it. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114 >> + // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox. > > Typo: "Make sure it doesn't share parent" Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120 >> + // Prepare for block direction align > > Nit: Comments should finish in a "dot". Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126 >> + // So add IsInside() here. > > Nit: s/IsInside/isInisde/ Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130 >> + forceLogicalHeight(*currentParent, Length(style.logicalHeight())); > > Isn't enough just calling setLogicalHeigth()? Not sure if I understand the question correctly. We need use setStyle to make sure it would trigger needsLayout(). And it calls three times, so create a function for them. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133 >> + // We need to remove marker here.E.g: <li><span><div>text<div><span></li> > > Mmmm, what's the expected behavior for this example? In this case, marker might occupy a anonymous block. <li> <anonymous><span>m_marker</anonymous> <div>text<div> <anonymous><span></anonymous> </li> We need detach m_marker from render tree, and insert it to newParent. So the render tree would like this: <li> <anonymous><span></anonymous> <div>m_marker text<div> <anonymous><span></anonymous> </li> Explained more about it in the new patch. >> LayoutTests/ChangeLog:10 >> + marker in li + block overflow:hiddlen child, and testing if the acting > > s/acting/behavior/ Done >> LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1 >> +<head></head><body><ul> > > Tests need to start with <!DOCTYPE html> unless you're explicitly testing quirks mode (which I don't think is the case). Done Comment on attachment 358035 [details] Patch Attachment 358035 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10534026 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/lists/list-and-margin-collapse.html fast/css-generated-content/table-with-before.html fast/css/only-child-pseudo-class.html fast/css/empty-pseudo-class.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/css/first-child-pseudo-class.html fast/forms/form-hides-table.html css2.1/20110323/height-applies-to-010a.htm fast/lists/list-marker-before-overflow-hidden.html fast/css/last-of-type-pseudo-class.html fast/css/last-child-pseudo-class.html fast/css/only-of-type-pseudo-class.html fast/css/first-of-type-pseudo-class.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358036 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358035 [details] Patch Attachment 358035 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10534039 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css/first-of-type-pseudo-class.html fast/css-generated-content/table-with-before.html fast/css/only-child-pseudo-class.html fast/css/empty-pseudo-class.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/css/first-child-pseudo-class.html fast/css/last-child-pseudo-class.html fast/forms/form-hides-table.html css2.1/20110323/height-applies-to-010a.htm fast/css/last-of-type-pseudo-class.html fast/lists/list-marker-before-overflow-hidden.html fast/css/only-of-type-pseudo-class.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358037 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358035 [details] Patch Attachment 358035 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10534085 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/lists/list-and-margin-collapse.html fast/css-generated-content/table-with-before.html fast/css/only-child-pseudo-class.html fast/css/empty-pseudo-class.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/css/first-child-pseudo-class.html fast/forms/form-hides-table.html css2.1/20110323/height-applies-to-010a.htm fast/lists/list-marker-before-overflow-hidden.html fast/css/last-of-type-pseudo-class.html fast/css/last-child-pseudo-class.html fast/css/only-of-type-pseudo-class.html fast/css/first-of-type-pseudo-class.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358038 [details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358035 [details] Patch Attachment 358035 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10534102 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css/first-of-type-pseudo-class.html fast/css-generated-content/table-with-before.html fast/css/only-child-pseudo-class.html fast/css/empty-pseudo-class.html fast/css-generated-content/table-row-with-before.html fast/css/first-child-pseudo-class.html editing/pasteboard/innerText-inline-table.html fast/css/last-child-pseudo-class.html fast/forms/form-hides-table.html css2.1/20110323/height-applies-to-010a.htm fast/css/last-of-type-pseudo-class.html fast/lists/list-marker-before-overflow-hidden.html fast/css/only-of-type-pseudo-class.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358039 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 358035 [details] Patch Attachment 358035 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10535130 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css/first-of-type-pseudo-class.html fast/css-generated-content/table-with-before.html fast/css/only-child-pseudo-class.html fast/css/empty-pseudo-class.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/css/first-child-pseudo-class.html fast/css/last-child-pseudo-class.html fast/forms/form-hides-table.html css2.1/20110323/height-applies-to-010a.htm fast/css/last-of-type-pseudo-class.html fast/lists/list-marker-before-overflow-hidden.html fast/css/only-of-type-pseudo-class.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358041 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 358063 [details]
Patch
Comment on attachment 358063 [details] Patch Attachment 358063 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10543823 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358065 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358063 [details] Patch Attachment 358063 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10543839 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358066 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358063 [details] Patch Attachment 358063 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10543972 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358067 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358063 [details] Patch Attachment 358063 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10544054 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/lists/list-marker-before-content-table.html fast/forms/form-hides-table.html webanimations/leak-document-with-web-animation.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358068 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 358063 [details] Patch Attachment 358063 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10543976 New failing tests: fast/css-generated-content/table-row-group-with-before.html fast/css-generated-content/table-with-before.html fast/css-generated-content/table-row-with-before.html fast/forms/form-hides-table.html fast/lists/list-marker-before-overflow-hidden.html fast/lists/list-and-margin-collapse.html fast/lists/ordered-list-with-no-ol-tag.html Created attachment 358069 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 358105 [details]
Patch
Comment on attachment 358105 [details] Patch Attachment 358105 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10558823 New failing tests: fast/forms/form-hides-table.html Created attachment 358106 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 358109 [details]
Patch
Created attachment 358114 [details]
Patch
Comment on attachment 358114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358114&action=review I have added a few extra comments on the patch, anyway I'd wait for WPT tests before merging this. The results in the rebaselined tests look much better. > Source/WebCore/ChangeLog:37 > + Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()). > Source/WebCore/rendering/RenderListItem.cpp:472 > + assert(baselineProvider); Why assert and not ASSERT? > Source/WebCore/rendering/RenderListItem.cpp:474 > + RenderBox* box = baselineProvider; > + offset = box->firstLineBaseline(); So this is done because RenderBlock::firstLineBaseline() is protected, wouldn't make sense to make it public? Specially if RenderBox::firstLineBaseline() is already public. Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(), if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()". > Source/WebCore/rendering/RenderListItem.cpp:496 > + for (RenderBox* o = m_marker->parentBox(); o != this; o = o->parentBox()) > + offsetValue -= o->logicalTop(); This is confusing, on a first sight I thought it was doing the same than the first loop. But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different. Could we put these two loops closer in the code and explain what they're doing we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37 > + bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false; Why we need a variable for this if it's only used in one place. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53 > + if (!inside) { > + if (current.hasOverflowClip()) > + return ¤t; > + if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot()))) > + return &downcast<RenderBlock>(child); Could you add a comment explaining this change? > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84 > + element.setStyle(WTFMove(style)); If this is only needed to trigger needs layout, why not doing: element.setLogicalHeight(WTFMove(height)); element.setNeedsLayout(); If this work I'd rename the method to something like: setLogicalHeightAndForceLayout() > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137 > + // E.g: <li><span><div>text<div><span></li> This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium. Check the following example: <ul> <li><span></span><div>text</div></li> <li><span>foo</span><div>text</div></li> </ul> Comment on attachment 358114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358114&action=review >> Source/WebCore/ChangeLog:37 >> + > > Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()). Done >> Source/WebCore/rendering/RenderListItem.cpp:472 >> + assert(baselineProvider); > > Why assert and not ASSERT? Done >> Source/WebCore/rendering/RenderListItem.cpp:474 >> + offset = box->firstLineBaseline(); > > So this is done because RenderBlock::firstLineBaseline() is protected, > wouldn't make sense to make it public? > Specially if RenderBox::firstLineBaseline() is already public. > > Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(), > if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()". Yes, this would call RenderBlock::firstLineBaseline(). Checked the patch that changed it to protect. https://chromium.googlesource.com/chromium/src/+/a5fd6dfebaee8cd6ed7f0c08e82306b4b44323e1 Done. >> Source/WebCore/rendering/RenderListItem.cpp:496 >> + offsetValue -= o->logicalTop(); > > This is confusing, on a first sight I thought it was doing the same than the first loop. > But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different. > Could we put these two loops closer in the code and explain what they're doing > we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens. Yes, they are different. Normally, baselineProvider is the child of list item which contains content. m_marker->parentBox() is the zero-height marker container. My intent is: offsetValue = (baseline of baselineProvider + offset from baselineProvider to listItem) - (baseline of marker container - offset from marker container to listItem). offsetValue is the distance marker need to move. Added comment to explain them. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37 >> + bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false; > > Why we need a variable for this if it's only used in one place. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53 >> + return &downcast<RenderBlock>(child); > > Could you add a comment explaining this change? Done. ===================== // For outside marker, return if we meet !overflow:visible, root writing-mode, // or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.). // The result would trigger setNeedsBlockDirectionAlign(true). And it will be the baselineProvider. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84 >> + element.setStyle(WTFMove(style)); > > If this is only needed to trigger needs layout, why not doing: > element.setLogicalHeight(WTFMove(height)); > element.setNeedsLayout(); > > If this work I'd rename the method to something like: > setLogicalHeightAndForceLayout() Looks like element.setLogicalHeight has been removed. And RenderObject just provide const RenderStyle. I think maybe this's reasonable in order to limit the change of style in setStyle. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137 >> + // E.g: <li><span><div>text<div><span></li> > > This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium. > Check the following example: > <ul> > <li><span></span><div>text</div></li> > <li><span>foo</span><div>text</div></li> > </ul> Sorry this example is causing misunderstands. I've added more explanation to it. =============== // If currentParent isn't the ancestor of newParent, marker might generate a new empty line. // We need to remove marker here. So it could be inserted to the proper position, and no extra line generated. // E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become: // RenderListItem // RenderBlock (anonymous, marker container) // RenderListMarker // RenderInline (span) // RenderBlock (anonymous) // RenderBlock (div) // RenderText // RenderBlock (anonymous) // RenderInline (span) // In this case, It would case an empty line because of marker. So we have to reattach marker Hi Rego, Thanks again for the review. :) I've add some comments, hope it would help to make thing clear. And the patch of moving test cases to wpt in chromium has been uploaded for code review. Created attachment 358171 [details]
Patch
*** Bug 13332 has been marked as a duplicate of this bug. *** Created attachment 358673 [details]
Patch
Created attachment 358674 [details]
Patch
This bug need a better title; I can't parse the current one. Created attachment 358789 [details]
Patch
Comment on attachment 358789 [details] Patch Attachment 358789 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10697319 New failing tests: imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html Created attachment 358793 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358789 [details] Patch Attachment 358789 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10697341 New failing tests: imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html Created attachment 358794 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358789 [details] Patch Attachment 358789 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10697464 New failing tests: imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html Created attachment 358797 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 358881 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #54) > This bug need a better title; I can't parse the current one. Done. Sorry for the typo. Comment on attachment 358881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358881&action=review > Source/WebCore/rendering/RenderBlock.cpp:519 > + if (logicalHeightLength.isFixed() && isAnonymous() && parent() && is<RenderListItem>(parent())) { is<> already checks for nullptr, so there is no need for the parent() clause. > Source/WebCore/rendering/RenderBlock.cpp:521 > + if (child && is<RenderListMarker>(child) && !child->nextSibling()) ditto > Source/WebCore/rendering/RenderBlock.h:-354 > - Optional<int> firstLineBaseline() const override; Why we need to change visibility of this function ? As far as I understood, m_baselineProvides is a RenderBlock, so protected visibility should be enough. > Source/WebCore/rendering/RenderListItem.cpp:461 > + if (baselineProvider && is<RenderBlockFlow>(baselineProvider) && downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot) No need to check for nullptr here if we already use the is<> template. > Source/WebCore/rendering/RenderListItem.cpp:465 > + if (!backToOriginalBaseline) Why not doing it this way instead ? Optional<int> offset = backToOriginalBaseline ? nullopt : baselineProvider->firstLineBaseline(); Then in the if below we can just have one clause: if (!offset) { > Source/WebCore/rendering/RenderListItem.cpp:474 > + if (offset) { Maybe we can early return here in case of !offset instead. Comment on attachment 358881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358881&action=review Hi Javier, Thanks a lot for the review:) >> Source/WebCore/rendering/RenderBlock.cpp:519 >> + if (logicalHeightLength.isFixed() && isAnonymous() && parent() && is<RenderListItem>(parent())) { > > is<> already checks for nullptr, so there is no need for the parent() clause. Done >> Source/WebCore/rendering/RenderBlock.cpp:521 >> + if (child && is<RenderListMarker>(child) && !child->nextSibling()) > > ditto Done >> Source/WebCore/rendering/RenderBlock.h:-354 >> - Optional<int> firstLineBaseline() const override; > > Why we need to change visibility of this function ? As far as I understood, m_baselineProvides is a RenderBlock, so protected visibility should be enough. The compiler complains. I think the reason is that for m_baselineProvides RenderListItem isn't the inside of own class. >> Source/WebCore/rendering/RenderListItem.cpp:461 >> + if (baselineProvider && is<RenderBlockFlow>(baselineProvider) && downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot) > > No need to check for nullptr here if we already use the is<> template. Done >> Source/WebCore/rendering/RenderListItem.cpp:465 >> + if (!backToOriginalBaseline) > > Why not doing it this way instead ? > > Optional<int> offset = backToOriginalBaseline ? nullopt : baselineProvider->firstLineBaseline(); > > Then in the if below we can just have one clause: > > if (!offset) { Done. Thanks! >> Source/WebCore/rendering/RenderListItem.cpp:474 >> + if (offset) { > > Maybe we can early return here in case of !offset instead. Done Created attachment 359016 [details]
Patch
Comment on attachment 359016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359016&action=review r=me. Thanks your all your work on this, I've several inline comments and nits please fix them before landing. > Source/WebCore/ChangeLog:11 > + sibilng of a block child. To fix these two kinds of issue: Typo: s/two kinds of issue/two kind of issues/ > Source/WebCore/ChangeLog:15 > + has been layout (marker have to adjust itself to the content of li) Typo: s/has been layout/have been layout/ Typo: s/marker have/marker has/ Nit: Missing dot at the end of sentence. > Source/WebCore/ChangeLog:17 > + Rebaseline test: I'd also mention the tests that are now passing from WPT (not as rebaseline tests but somewhere in the changelog. > Source/WebCore/ChangeLog:27 > + marker_container only have marker as its child, make it a self collapsing Typo: s/only have/only has/ > Source/WebCore/ChangeLog:40 > + whose m_needBlockDirectionAlign is true, insert marker to a zero height Typo: s/insert marker to/insert marker into/ > Source/WebCore/rendering/RenderListItem.cpp:452 > + // So if there's no line box in baselineProvider make sure it back to its original position. Typo: s/make sure it back/make sure it is back/ > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:49 > + // For outside marker, return if we meet !overflow:visible, root writing-mode, > + // or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.). This kind of comments are not very useful as they're like repeating the if condition in a comment. We'd need to either describe this with words or remove it. Also it doesn't say what we return... and it can be current or child. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:118 > + // Create a zero height parent for marker, so that marker could be displayed or no line break between marker and content. I'd reword this a little bit doing something like: * "marker could be displayed" => "marker would be visible with overflow: hidden" * "and there won't be a line break between marker and block content" Feel free to change it and write it properly. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:121 > + // If marker is the only child of markerContainer, set LogicalHeight of markerContainer to 0px; else restore it to LogicalHeight of <li>. I'd move this line 121 to around line 130, as there is where this is done. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:122 > + if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot())) Why we check "newParent->isWritingModeRoot()" here? > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:131 > + // So add isInside() here. Remove this line, the previous line is explaining what we're doing so we don't need this one (this is already in the if condition). > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139 > + // E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become: Please close the SPAN and DIV tags to make it clearer the example. And I'd remove the second SPAN as it's not needed. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:148 > + // RenderBlock (anonymous) > + // RenderInline (span) So we could remove these two lines if we remove the second SPAN. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:149 > + // In this case, It would case an empty line because of marker. So we have to reattach marker Typo: s/It would case/it would cause/ Maybe we need to reword this a little bit. It'd cause an empty line or a line break? I understand that the reason is because marker is in a different RenderBlock. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:169 > + if (originalMarker.get()) Isn't enough with using "if (originalMarker)" ? > LayoutTests/TestExpectations:2932 > +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker. Instead of adding a comment, I'd report this a separated bug and link it from here like: webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ] Comment on attachment 359016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359016&action=review > Source/WebCore/ChangeLog:12 > + 1. Create a zero-height anonymous parent for marker (to eleminate the eleminate -> eliminate > Source/WebCore/ChangeLog:14 > + 2. Reposition the position of marker after all list item's children "Reposition the position of marker after" -> "Reposition the marker to be after" > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:80 > +static void forceLogicalHeight(RenderElement& element, Length&& height) Don't call the argument 'element'. That makes it sound like an HTMLElement. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:125 > + // Prepare for block direction align. This comment doesn't add anything. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:127 > + // Deal with the situation of render tree changed. This comment doesn't add anything. > Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:187 > + m_builder.attach(*markerContainer, WTFMove(originalMarker), firstNonMarkerChild(*markerContainer)); > + else > + m_builder.attach(*markerContainer, WTFMove(newMarkerRenderer), firstNonMarkerChild(*markerContainer)); > + m_builder.attach(listItemRenderer, WTFMove(markerContainer), beforeChild); > + } else { > + if (markerRenderer->isInside()) { > + listItemRenderer.setNeedsBlockDirectionAlign(false); > + if (!newParent) > + newParent = &listItemRenderer; > + if (originalMarker.get()) > + m_builder.attach(*newParent, WTFMove(originalMarker), firstNonMarkerChild(*newParent)); > + else > + m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent)); > + > + } else if (originalMarker.get()) > + m_builder.attach(listItemRenderer, WTFMove(originalMarker), beforeChild); > + else > + m_builder.attach(listItemRenderer, WTFMove(newMarkerRenderer), beforeChild); All these repeated "m_builder.attach(listItemRenderer, <something>, beforeChild);" makes me think this code could be refactored to be much cleaner. Generally, if you have code that has multiple calls to a function with different parameters, you should factor it to have a single call. You could have a function or lambda that returns the appropriate marker (original or new), and just have one call to m_builder.attach(). Comment on attachment 359016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359016&action=review >> Source/WebCore/ChangeLog:11 >> + sibilng of a block child. To fix these two kinds of issue: > > Typo: s/two kinds of issue/two kind of issues/ Done >> Source/WebCore/ChangeLog:15 >> + has been layout (marker have to adjust itself to the content of li) > > Typo: s/has been layout/have been layout/ > Typo: s/marker have/marker has/ > Nit: Missing dot at the end of sentence. Done >> Source/WebCore/ChangeLog:17 >> + Rebaseline test: > > I'd also mention the tests that are now passing from WPT (not as rebaseline tests but somewhere in the changelog. Done >> Source/WebCore/ChangeLog:27 >> + marker_container only have marker as its child, make it a self collapsing > > Typo: s/only have/only has/ Done >> Source/WebCore/ChangeLog:40 >> + whose m_needBlockDirectionAlign is true, insert marker to a zero height > > Typo: s/insert marker to/insert marker into/ Done >> Source/WebCore/rendering/RenderListItem.cpp:452 >> + // So if there's no line box in baselineProvider make sure it back to its original position. > > Typo: s/make sure it back/make sure it is back/ Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:49 >> + // or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.). > > This kind of comments are not very useful as they're like repeating the if condition in a comment. > We'd need to either describe this with words or remove it. > Also it doesn't say what we return... and it can be current or child. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:118 >> + // Create a zero height parent for marker, so that marker could be displayed or no line break between marker and content. > > I'd reword this a little bit doing something like: > * "marker could be displayed" => "marker would be visible with overflow: hidden" > * "and there won't be a line break between marker and block content" > Feel free to change it and write it properly. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:121 >> + // If marker is the only child of markerContainer, set LogicalHeight of markerContainer to 0px; else restore it to LogicalHeight of <li>. > > I'd move this line 121 to around line 130, as there is where this is done. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:122 >> + if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot())) > > Why we check "newParent->isWritingModeRoot()" here? We'd make li + WritingModeRoot using the "zero-height approach", otherwise there is a line-break. This's according to getParentOfFirstLineBox(). Sorry, this's hard to read. Changed it a bit. It might be clear now. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:131 >> + // So add isInside() here. > > Remove this line, the previous line is explaining what we're doing so we don't need this one (this is already in the if condition). Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139 >> + // E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become: > > Please close the SPAN and DIV tags to make it clearer the example. > And I'd remove the second SPAN as it's not needed. Done and reword the example. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:148 >> + // RenderInline (span) > > So we could remove these two lines if we remove the second SPAN. Sorry... See the new explain of this example. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:149 >> + // In this case, It would case an empty line because of marker. So we have to reattach marker > > Typo: s/It would case/it would cause/ > Maybe we need to reword this a little bit. It'd cause an empty line or a line break? I understand that the reason is because marker is in a different RenderBlock. Done Yeah, it's line break. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:169 >> + if (originalMarker.get()) > > Isn't enough with using "if (originalMarker)" ? Done >> LayoutTests/TestExpectations:2932 >> +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker. > > Instead of adding a comment, I'd report this a separated bug and link it from here like: > webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ] Done (In reply to cathiechen from comment #69) > > >> LayoutTests/TestExpectations:2932 > >> +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker. > > > > Instead of adding a comment, I'd report this a separated bug and link it from here like: > > webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ] > > Done Thanks for fixing them all. Do you mind if I take a final look before you cq+ your updated patch? Thanks. Comment on attachment 359016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359016&action=review Hi Simon, Thanks for the review :) >> Source/WebCore/ChangeLog:12 >> + 1. Create a zero-height anonymous parent for marker (to eleminate the > > eleminate -> eliminate Done >> Source/WebCore/ChangeLog:14 >> + 2. Reposition the position of marker after all list item's children > > "Reposition the position of marker after" -> "Reposition the marker to be after" Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:80 >> +static void forceLogicalHeight(RenderElement& element, Length&& height) > > Don't call the argument 'element'. That makes it sound like an HTMLElement. Done. Changed it to renderer. >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:125 >> + // Prepare for block direction align. > > This comment doesn't add anything. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:127 >> + // Deal with the situation of render tree changed. > > This comment doesn't add anything. Done >> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:187 >> + m_builder.attach(listItemRenderer, WTFMove(newMarkerRenderer), beforeChild); > > All these repeated "m_builder.attach(listItemRenderer, <something>, beforeChild);" makes me think this code could be refactored to be much cleaner. Generally, if you have code that has multiple calls to a function with different parameters, you should factor it to have a single call. > > You could have a function or lambda that returns the appropriate marker (original or new), and just have one call to m_builder.attach(). Done. Thanks:) Created attachment 359606 [details]
Patch
(In reply to zalan from comment #70) > Thanks for fixing them all. Do you mind if I take a final look before you > cq+ your updated patch? Thanks. That is great! The new patch has been uploaded. Thanks:) (In reply to zalan from comment #70) > Thanks for fixing them all. Do you mind if I take a final look before you > cq+ your updated patch? Thanks. Pinging Zalan... :-) (In reply to Manuel Rego Casasnovas from comment #74) > (In reply to zalan from comment #70) > > Thanks for fixing them all. Do you mind if I take a final look before you > > cq+ your updated patch? Thanks. > > Pinging Zalan... :-) :( will review this over the weekend. Any reason why you didn't make this generic rule of _always_ wrapping the marker inside a dedicated anonymous block box? It would surely simplify some of the "new parent" logic here. (In reply to zalan from comment #76) > Any reason why you didn't make this generic rule of _always_ wrapping the > marker inside a dedicated anonymous block box? It would surely simplify some > of the "new parent" logic here. Hi zalan, Thanks for the reply:) The original thought of this patch is to fix the issues. And it would increase the cost for adding an additional anonymous block compared to the original algorithm. I do agree the logic would get simpler if we make it a generic rule. Maybe we could do it in the next patch? Hi zalan@, I filed a bug to make zero-height a common rule. What do you think? :) (In reply to cathiechen from comment #78) > Hi zalan@, > > I filed a bug to make zero-height a common rule. What do you think? :) https://bugs.webkit.org/show_bug.cgi?id=196489 Created attachment 366487 [details]
Patch
Comment on attachment 366487 [details]
Patch
Rebase
Rebase @Alan - Is this something needed based on IFC work or it is already fixed? If you want, I can try to rebase and see if it is easier to do and in case of any error, can ask for help? Thanks! (In reply to Ahmad Saleem from comment #83) > @Alan - Is this something needed based on IFC work or it is already fixed? > If you want, I can try to rebase and see if it is easier to do and in case > of any error, can ask for help? Thanks! Not yet fixed, but will certainly be addressed by LFC (the umbrella project). |