Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657483003:
Add a border radius property to inherit it from a parent in a fallback content (Closed)
|
Created:
4 years, 10 months ago by Miyoung Shin(c) Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a border radius property to inherit it from a parent in a fallback content
https://crrev.com/983693002 caused the regression that hit
testing is stopped in the parent inline, when it has border
radius style and locates out of a boundingBox of hitTest.
That patch made a early return in the parent inline.
We should do hit testing children first before doing the
parent's border radius.
Instead of that patch, we need to check the border radius
for LayoutBox in InlineFlowBox::nodeAtPoint.
Also the origin problem is that the fallback content of
image does not inherit the border radius property.
We should add the border radius property to inherit it from
the parent.
BUG=568896
R=pdr@chromium.org
TEST=fast/inline-block/hittest-child-of-inlineblock.html
fast/inline-block/hittest-inline-block-with-abspos.html
hittesting/border-hittest-with-image-fallback.html
Committed: https://crrev.com/58453820ceb5beada016cb6509d244d152776574
Cr-Commit-Position: refs/heads/master@{#385480}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : #
Total comments: 1
Messages
Total messages: 38 (10 generated)
PTAL
https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html (right): https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:1: <!DOCTYPE html> It looks like this test is intended to show the behavior of crbug.com/579741, can you update the change description to explain how the new patch avoids the issues in crbug.com/579741? https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:9: visibility: hidden; This seems like it would prevent this test from working. How does an invisible element affect hit testing? https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right): https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/line/InlineBox.cpp:229: bool inside = lineLayoutItem().hitTest(result, locationInContainer, childPoint); The other nodeAtPoint functions follow a pattern where, for non-rect hit testing, the HitTestResult is updated if and only if nodeAtPoint returns true. For example, in LayoutBlock::nodeAtPoint, all of the codepaths that return true will call updateHitTestResult (except for rect-based tests). In this patch, you're updating the result (through lineLayoutItem().hitTest) but returning false from InlineBox::nodeAtPoint... that doesn't seem right. Can you investigate why a different pattern is used here vs the other nodeAtPoints? It may also be useful to find what code is working when InlineBox::nodeAtPoint returns false but the HitTestResult has been updated. I suspect that code is wrong.
https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right): https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/line/InlineBox.cpp:229: bool inside = lineLayoutItem().hitTest(result, locationInContainer, childPoint); On 2016/01/31 21:32:36, pdr wrote: > The other nodeAtPoint functions follow a pattern where, for non-rect hit > testing, the HitTestResult is updated if and only if nodeAtPoint returns true. > For example, in LayoutBlock::nodeAtPoint, all of the codepaths that return true > will call updateHitTestResult (except for rect-based tests). In this patch, > you're updating the result (through lineLayoutItem().hitTest) but returning > false from InlineBox::nodeAtPoint... that doesn't seem right. Can you > investigate why a different pattern is used here vs the other nodeAtPoints? > > It may also be useful to find what code is working when InlineBox::nodeAtPoint > returns false but the HitTestResult has been updated. I suspect that code is > wrong. Your right. I found out we have only this issue with missing image. We create a new element for missing image, but don't inherit the style from the original IMG. If we apply the right style, I think we don't need this code. Thanks for letting me look deep into this. It's a National holiday until a next week. After I come back, I will upload the new patchset.
Description was changed from ========== [Reland] Fix-up a wrong hit testing when a parent inline has border radius. The problem is that hit testing is stopped in the parent inline. When it has border radius style and locates out of a boundingBox of hitTest. We should do hit testing children first before doing the parent's border radius. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content The problem is that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. We should do hit testing children first before doing the parent's border radius. https://crrev.com/983693002 caused this regression. That patch made a early return in the parent inline, but we do not need this patch because we already check a border radius area in InlineFlowBox::nodeAtPoint. The origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent, and add a float property not to be effect to origin <IMG> layout. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html ==========
Description was changed from ========== Add a border radius property to inherit it from a parent in a fallback content The problem is that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. We should do hit testing children first before doing the parent's border radius. https://crrev.com/983693002 caused this regression. That patch made a early return in the parent inline, but we do not need this patch because we already check a border radius area in InlineFlowBox::nodeAtPoint. The origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent, and add a float property not to be effect to origin <IMG> layout. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content The problem is that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. We should do hit testing children first before doing the parent's border radius. https://crrev.com/983693002 caused this regression. That patch made a early return in the parent inline, but we do not need this patch because we already check a border radius area in InlineFlowBox::nodeAtPoint. The origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent, and add a float property not to be effect to origin <IMG> layout. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html ==========
I've uploaded the new patch, PTAL. https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html (right): https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:1: <!DOCTYPE html> On 2016/01/31 21:32:36, pdr wrote: > It looks like this test is intended to show the behavior of crbug.com/579741, > can you update the change description to explain how the new patch avoids the > issues in crbug.com/579741? Done. I've updated the change description. https://codereview.chromium.org/1657483003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:9: visibility: hidden; On 2016/01/31 21:32:36, pdr wrote: > This seems like it would prevent this test from working. How does an invisible > element affect hit testing? I had changed the condition like below at http://crrev.com/369971. if (locationInContainer.intersects(border)) return true; And the invisible element was hit.
Sorry, please don't review it yet. Regarding to trybot fails, float property I added affects a several of dump-text results. I don't think it's a good way to update all of the expectation result. I will find out how to avoid it.
Description was changed from ========== Add a border radius property to inherit it from a parent in a fallback content The problem is that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. We should do hit testing children first before doing the parent's border radius. https://crrev.com/983693002 caused this regression. That patch made a early return in the parent inline, but we do not need this patch because we already check a border radius area in InlineFlowBox::nodeAtPoint. The origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent, and add a float property not to be effect to origin <IMG> layout. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ==========
Description was changed from ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ==========
pdr@, PTAL. Regarding Trybot fails of linux_blink_dbg, it doesn't seem to be caused by my patch. When I test it manually, it pass and other patches also show fail results.
pdr@, could you take a look ?
https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); How does the code in InlineFlowBox::nodeAtPoint relate to this change? Are these separate changes or are they both required? https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1036: if (lineLayoutItem().style()->hasBorderRadius()) { I agree that the code in InlineBox::nodeAtPoint is wrong, but I don't understand why the code here is correct. Why can't we change includeLogicalLeftEdge to look like: bool includeLogicalLeftEdge() const { return lineLayoutItem().isBox() ? m_includeLogicalLeftEdge : false; } I guess it's just not clear why this isBox check is correct, since I don't see similar calls elsewhere.
https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); On 2016/03/07 21:15:50, pdr wrote: > How does the code in InlineFlowBox::nodeAtPoint relate to this change? Are these > separate changes or are they both required? Yes, we need both of them, because container(DIV) will be hit if we don't inherit this property. https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1036: if (lineLayoutItem().style()->hasBorderRadius()) { On 2016/03/07 21:15:51, pdr wrote: > I agree that the code in InlineBox::nodeAtPoint is wrong, but I don't understand > why the code here is correct. > > Why can't we change includeLogicalLeftEdge to look like: > bool includeLogicalLeftEdge() const { return lineLayoutItem().isBox() ? > m_includeLogicalLeftEdge : false; } > > I guess it's just not clear why this isBox check is correct, since I don't see > similar calls elsewhere. <img> has a <div> child with inline-block for a fallback image, and <img> inlinebox is a rootInlineBox. Then, we will set the position in RootInlineBox::alignBoxesInBlockDirection and the position of <img> is updated by the child's maxAscent. So, <img>, rootInlineBox(inlineflowbox), has the different position from LayoutBox's position. If <img> doesn't have any child or the child's size is none, <img> position is not updated. As I know, in general <img> doesn't have chidren. I've seen the only case that <img> has a child when shadow dom is created for a fallback image. Considering this case, I thought we should check borderRadius with <img> LayoutBox's position. If I'm wrong, could you please correct me ?
Sorry it took a while to get to this. https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); On 2016/03/09 at 12:28:07, Miyoung Shin(c) wrote: > On 2016/03/07 21:15:50, pdr wrote: > > How does the code in InlineFlowBox::nodeAtPoint relate to this change? Are these > > separate changes or are they both required? > > Yes, we need both of them, because container(DIV) will be hit if we don't inherit this property. Does a test depend on this? both border-hittest.html and the new test (border-hittest-with-image-fallback.html) pass without this patch, and without this change. https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1036: if (lineLayoutItem().style()->hasBorderRadius()) { On 2016/03/09 at 12:28:07, Miyoung Shin(c) wrote: > Considering this case, I thought we should check borderRadius with <img> LayoutBox's position. I think you're getting stuck on the <img> case when the issue here is unrelated to images. I would recommend getting everything working for the common case of divs, then see if images need special handling. Hit testing and painting are very similar problems so one way to approach this is to look at how rounded rects are painted, and use the same functions here. Another approach might be to directly call LayoutBox::hitTestClippedOutByRoundedBorder.
I've uploaded the new patch. PTAL. https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); On 2016/03/21 05:26:10, pdr wrote: > On 2016/03/09 at 12:28:07, Miyoung Shin(c) wrote: > > On 2016/03/07 21:15:50, pdr wrote: > > > How does the code in InlineFlowBox::nodeAtPoint relate to this change? Are > these > > > separate changes or are they both required? > > > > Yes, we need both of them, because container(DIV) will be hit if we don't > inherit this property. > > Does a test depend on this? both border-hittest.html and the new test > (border-hittest-with-image-fallback.html) pass without this patch, and without > this change. The new test(border-hittest-with-image-fallback.html will be failed without this patch if we revert the patch at https://crrev.com/983693002 that made the early return in InlineBox.cpp. https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1657483003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1036: if (lineLayoutItem().style()->hasBorderRadius()) { On 2016/03/21 05:26:10, pdr wrote: > On 2016/03/09 at 12:28:07, Miyoung Shin(c) wrote: > > Considering this case, I thought we should check borderRadius with <img> > LayoutBox's position. > > I think you're getting stuck on the <img> case when the issue here is unrelated > to images. I would recommend getting everything working for the common case of > divs, then see if images need special handling. > > Hit testing and painting are very similar problems so one way to approach this > is to look at how rounded rects are painted, and use the same functions here. > Another approach might be to directly call > LayoutBox::hitTestClippedOutByRoundedBorder. As you suggested, I've looked at how rounded rects are painted. In case of IMG, we use the overflow rectangle applied with lineTop and lineBottom to paint the rounded border, but in case of a general inlineflowbox such as text, we use the frameRect moved by the paint offset in InlineFlowBoxPainter::paintBoxDecorationBackground. Currently in this function, we check hitTest with overflowRect but for borderRadius we use frameRect. So I think it is inconsistent, and LayoutBox::hitTestClippedOutByRoundedBorder will be reasonable to use here.
Using hitTestClippedOutByRoundedBorder looks much more reasonable but the rest of the patch does not. When I apply this patch, the testcase in the bug still fails. Did you mean to revert the change from InlineBox::nodeAtPoint or is it still needed? https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-child-of-inlineblock.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-child-of-inlineblock.html:1: <!DOCTYPE html> Can you move these test under hittesting? i.e.: hittesting/child-of-inlineblock.html hittesting/inline-block-with-abspos.html https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:9: visibility: hidden; What is this test testing? Which c++ line from this patch can we remove to make this test fail? https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:19: <div id="abs"><div id="abs-inner"></div> </div> Nit: trailing whitespace https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/hittesting/border-hittest-with-image-fallback.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/hittesting/border-hittest-with-image-fallback.html:1: <!doctype html> In your last reply you said "The new test(border-hittest-with-image-fallback.html will be failed without this patch if we revert the patch at https://crrev.com/983693002 that made the early return in InlineBox.cpp." On my machine, this test passes in the following scenarios: * Without any patch * With https://crrev.com/983693002 reverted * With this patch and https://crrev.com/983693002 reverted * With this patch In what case does this test fail? https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); I still don't see why this is needed. Which test fails without this change?
To make this review a little faster, please make sure that the tests fail without the C++ changes, and pass with them.
I'm travelling on business until 8 Apr to China. A network could be unstable in Chana so I may not get your review right away. https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-child-of-inlineblock.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-child-of-inlineblock.html:1: <!DOCTYPE html> On 2016/03/31 04:20:11, pdr wrote: > Can you move these test under hittesting? > > i.e.: > hittesting/child-of-inlineblock.html > hittesting/inline-block-with-abspos.html Done. https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:9: visibility: hidden; On 2016/03/31 04:20:11, pdr wrote: > What is this test testing? Which c++ line from this patch can we remove to make > this test fail? This is to prevent the regression that I made at https://crrev.com/1592413002. It was reverted. When I worked on that CL, I couldn't recognize regression because of lack of TC. I changed my approach comparing to that CL and I wanted to guarantee that new CL didn't create the regression. So we shouldn't get any test fail with/without this patch. (see https://bugs.chromium.org/p/chromium/issues/detail?id=568896#c11) https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline-block/hittest-inline-block-with-abspos.html:19: <div id="abs"><div id="abs-inner"></div> </div> On 2016/03/31 04:20:11, pdr wrote: > Nit: trailing whitespace Done. https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/hittesting/border-hittest-with-image-fallback.html (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/hittesting/border-hittest-with-image-fallback.html:1: <!doctype html> On 2016/03/31 04:20:11, pdr wrote: > In your last reply you said "The new > test(border-hittest-with-image-fallback.html will be failed without this patch > if we revert the patch at https://crrev.com/983693002 that made the early return > in InlineBox.cpp." > > On my machine, this test passes in the following scenarios: > * Without any patch > * With https://crrev.com/983693002 reverted > * With this patch and https://crrev.com/983693002 reverted > * With this patch > > In what case does this test fail? I'm really sorry but I missed to revert https://crrev.com/983693002 at the fourth patch. This is my test result. - PASS * Without any patch * With the fifth patch (= * With this patch and https://crrev.com/983693002 reverted) - Fail * With https://crrev.com/983693002 reverted * With the fifth patch except the C++ change in HTMLImageFallbackHelper.cpp https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, AtomicString("inherit", AtomicString::ConstructFromLiteral)); On 2016/03/31 04:20:11, pdr wrote: > I still don't see why this is needed. Which test fails without this change? border-hittest-with-image-fallback.html is failed without this change. BTW, I checked how to draw the missed image having border-radius on FireFox, they apply the rounded border to the missed image but we don't. I think we need to draw the rounded border of the missed image and it works fine with this path like them.
Regarding with trybot fail, I need to rebase it. As soon as I come back from BZ trip, I will upload the patch based on the latest code.
I've uploaded the patch, PTAL.
On 2016/04/04 at 15:45:38, myid.shin wrote: > I've uploaded the patch, PTAL. Cool, I like this one much better. LGTM
The CQ bit was checked by myid.shin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657483003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657483003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by myid.shin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657483003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657483003/100001
Message was sent while issue was closed.
Description was changed from ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html ========== to ========== Add a border radius property to inherit it from a parent in a fallback content https://crrev.com/983693002 caused the regression that hit testing is stopped in the parent inline, when it has border radius style and locates out of a boundingBox of hitTest. That patch made a early return in the parent inline. We should do hit testing children first before doing the parent's border radius. Instead of that patch, we need to check the border radius for LayoutBox in InlineFlowBox::nodeAtPoint. Also the origin problem is that the fallback content of image does not inherit the border radius property. We should add the border radius property to inherit it from the parent. BUG=568896 R=pdr@chromium.org TEST=fast/inline-block/hittest-child-of-inlineblock.html fast/inline-block/hittest-inline-block-with-abspos.html hittesting/border-hittest-with-image-fallback.html Committed: https://crrev.com/58453820ceb5beada016cb6509d244d152776574 Cr-Commit-Position: refs/heads/master@{#385480} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/58453820ceb5beada016cb6509d244d152776574 Cr-Commit-Position: refs/heads/master@{#385480}
Message was sent while issue was closed.
Hmmm inheriting border-radius seems very wrong, why does image fallback content need to do that? What's special about the fallback content compared to regular content? <img> with fallback content is just a <span> with a bunch of stuff inside it, there's nothing special there. If hit testing was busted you can't just hack <img>, you need to fix the real problem. This seems like it needs to be reverted. You should never need to inherit a non-inherited property.
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
Please revert this and fix whatever the real problem is, adding inherit to the fallback content is wrong. https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, ("inherit")); This is not right.
Message was sent while issue was closed.
On 2016/04/06 at 17:50:08, esprehn wrote: > Please revert this and fix whatever the real problem is, adding inherit to the fallback content is wrong. > > https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): > > https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, ("inherit")); > This is not right. Can you describe another way to solve this? I asked the same thing in this review (3x) and myid.shin showed that a test depends on this behavior. This makes sense to me too, since we want the image fallback content to inherit the rounded radius of the image.
Message was sent while issue was closed.
On 2016/04/06 at 17:58:05, pdr wrote: > On 2016/04/06 at 17:50:08, esprehn wrote: > > Please revert this and fix whatever the real problem is, adding inherit to the fallback content is wrong. > > > > https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right): > > > > https://codereview.chromium.org/1657483003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:47: container->setInlineStyleProperty(CSSPropertyBorderRadius, ("inherit")); > > This is not right. > > Can you describe another way to solve this? I asked the same thing in this review (3x) and myid.shin showed that a test depends on this behavior. This makes sense to me too, since we want the image fallback content to inherit the rounded radius of the image. @myid.shin, Elliott and I talked offline and I'm convinced we have another bug here. Do you mind reverting just the inherit part and filing a bug? Something doesn't seem right with using inherit there and I'd like to address it separate from the core fix in this patch.
Message was sent while issue was closed.
> @myid.shin, Elliott and I talked offline and I'm convinced we have another bug > here. Do you mind reverting just the inherit part and filing a bug? Something > doesn't seem right with using inherit there and I'd like to address it separate > from the core fix in this patch. I've uploaded the reverting patch at https://codereview.chromium.org/1877733002/. BTW, could you explain what issue we can get by |inherit| in detail? It would be helpful to the next patch and prevent the mistake for the next time. in_reply_to: 5743868070854656
Message was sent while issue was closed.
On 2016/04/10 at 14:01:35, myid.shin wrote: > > @myid.shin, Elliott and I talked offline and I'm convinced we have another bug > > here. Do you mind reverting just the inherit part and filing a bug? Something > > doesn't seem right with using inherit there and I'd like to address it separate > > from the core fix in this patch. > > I've uploaded the reverting patch at https://codereview.chromium.org/1877733002/. > BTW, could you explain what issue we can get by |inherit| in detail? It would be helpful to the next patch and prevent the mistake for the next time. > in_reply_to: 5743868070854656 I've added a comment to the bug (568896). My current thinking is that this isn't really a bug with your patch, but instead it's a bug in the html image fallback code. I think it would be useful to try to confirm that theory by reproducing it in regular shadow dom. If we can confirm this is really a bug in HTMLImageFallbackHelper, I don't think it's particularly high-priority to fix compared to other hit testing bugs like crbug.com/468497. |
