Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1034433003:
Hittest for RootInlineBox/InlineFlowBox should take care of border-radius. (Closed)
|
Created:
5 years, 9 months ago by Abhijeet Kandalkar Slow Modified:
5 years, 8 months ago CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionHitTest for RootInlineBox/InlineFlowBox should take care of border-radius.
Current behavior of hit testing works properly for LayoutBlock and InlineBox
having border style but doesn't take care of RootInlineBox/InlineFlowBox
with border style and hence returns the wrong element. This patch adds the
implementation for border style handling to RootInlineBox/InlineFlowBox
to try to match FF.
BUG=468760
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192994
Patch Set 1 #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : Patch for landing #
Total comments: 4
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 23 (3 generated)
abhijeet.k@samsung.com changed reviewers: + davve@opera.com, leviw@chromium.org, vivekg@chromium.org
Please review patch.
https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:14: #test:hover { You want to keep the hover rule for manual testing? https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:24: <div id="console"></div> You may drop this element? Would probably require you to regenerate the expected file though... https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:27: description('If this test passes, area outside is body element.'); ... area outside border radius is body element ... https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... Source/core/layout/line/InlineFlowBox.cpp:981: FloatRoundedRect border = layoutObject().style()->getRoundedBorderFor(borderRect); I don't think this is correct since we'll have a InlineFlowBox for each line. <!doctype html> <style> span { background-color: lightgray; border-radius: 2em; padding: 1em; line-height:4em; } span:hover { background-color:lightblue; } </style> <span> Hovering non-rounded corners should<br/> make background turn blue. </span> Notice that in this snippet you will not hit the span in any corner, not even the non-rounded ones.
Please review updated patch. https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:14: #test:hover { On 2015/03/24 13:26:03, David Vest wrote: > You want to keep the hover rule for manual testing? Yes, it is for manual testing. Keeping span:hover mangle with <span> inserted by js-test script and produce undesirable visual output. https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:24: <div id="console"></div> On 2015/03/24 13:26:03, David Vest wrote: > You may drop this element? Would probably require you to regenerate the expected > file though... Same as above. For better readability during manual testing. https://codereview.chromium.org/1034433003/diff/1/LayoutTests/fast/borders/bo... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:27: description('If this test passes, area outside is body element.'); On 2015/03/24 13:26:03, David Vest wrote: > ... area outside border radius is body element ... Done. https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... Source/core/layout/line/InlineFlowBox.cpp:981: FloatRoundedRect border = layoutObject().style()->getRoundedBorderFor(borderRect); On 2015/03/24 13:26:03, David Vest wrote: > I don't think this is correct since we'll have a InlineFlowBox for each line. > > <!doctype html> > <style> > span { > background-color: lightgray; > border-radius: 2em; > padding: 1em; > line-height:4em; > } > span:hover { > background-color:lightblue; > } > </style> > <span> > Hovering non-rounded corners should<br/> make background turn blue. > </span> > > Notice that in this snippet you will not hit the span in any corner, not even > the non-rounded ones. Above use case is working fine with implementation in patch. I have tested manually by creating page from about HTML snippets.
https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... Source/core/layout/line/InlineFlowBox.cpp:981: FloatRoundedRect border = layoutObject().style()->getRoundedBorderFor(borderRect); Hm, when I apply your patch to tip-of-tree and run http://jsbin.com/xolowiyipe/1/ I get the result BODY, BODY, not the result BODY SPAN I expect (which is also what Firefox returns). Did I misunderstand?
Please review latest patch. It is working fine for line breaking. https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/1/Source/core/layout/line/Inl... Source/core/layout/line/InlineFlowBox.cpp:981: FloatRoundedRect border = layoutObject().style()->getRoundedBorderFor(borderRect); On 2015/03/24 15:49:14, David Vest wrote: > Hm, when I apply your patch to tip-of-tree and run > http://jsbin.com/xolowiyipe/1/ I get the result BODY, BODY, not the result BODY > SPAN I expect (which is also what Firefox returns). Did I misunderstand? Thanks for inputs. I modified patch to match FF behavior.
https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:22: Two line with <br/>a hard line break. lines https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:31: shouldBe('elementInTopLeftCorner.nodeName', '"BODY"'); Instead of these two shouldBe you could use shouldBeEqualToString to avoid the extra quoting. https://codereview.chromium.org/1034433003/diff/40001/Source/core/layout/line... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/40001/Source/core/layout/line... Source/core/layout/line/InlineFlowBox.cpp:982: if (!locationInContainer.intersects(border)) We're heading in the right direction but it's still not quite there. We need to consider nested inlines sticking out as well (http://jsbin.com/bejevugilo/1/edit). BoxPainter::getBackgroundRoundedRect might be a source of inspiration, but I don't like having to duplicate non-trivial pieces of code. I might have to defer to an owner on the details here.
David thanks for your inputs. Please review patch. https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:22: Two line with <br/>a hard line break. On 2015/03/26 10:03:36, David Vest wrote: > lines Done. https://codereview.chromium.org/1034433003/diff/40001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:31: shouldBe('elementInTopLeftCorner.nodeName', '"BODY"'); On 2015/03/26 10:03:36, David Vest wrote: > Instead of these two shouldBe you could use shouldBeEqualToString to avoid the > extra quoting. Done. https://codereview.chromium.org/1034433003/diff/40001/Source/core/layout/line... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/40001/Source/core/layout/line... Source/core/layout/line/InlineFlowBox.cpp:982: if (!locationInContainer.intersects(border)) On 2015/03/26 10:03:36, David Vest wrote: > We're heading in the right direction but it's still not quite there. We need to > consider nested inlines sticking out as well > (http://jsbin.com/bejevugilo/1/edit). BoxPainter::getBackgroundRoundedRect might > be a source of inspiration, but I don't like having to duplicate non-trivial > pieces of code. I might have to defer to an owner on the details here. Added implementation to handle nested inlines and updated layout test too.
https://codereview.chromium.org/1034433003/diff/60001/Source/core/layout/line... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/60001/Source/core/layout/line... Source/core/layout/line/InlineFlowBox.cpp:985: for (InlineBox* curr = lastChild(); curr; curr = curr->prevOnLine()) { Instead of having a loop here and a preliminary hit-test, could we instead move this entire check to somewhere around the "Now check ourselves." part of the function? The idea being that we have already ruled out hitting children by then.
Please review latest patch. https://codereview.chromium.org/1034433003/diff/60001/Source/core/layout/line... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1034433003/diff/60001/Source/core/layout/line... Source/core/layout/line/InlineFlowBox.cpp:985: for (InlineBox* curr = lastChild(); curr; curr = curr->prevOnLine()) { On 2015/03/27 12:40:20, David Vest wrote: > Instead of having a loop here and a preliminary hit-test, could we instead move > this entire check to somewhere around the "Now check ourselves." part of the > function? The idea being that we have already ruled out hitting children by > then. Done.
Big thanks for hanging in there. Non-owner LGTM.
Code change looks great! Test could be cleaner. https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:6: body { margin: 2em; } Why? https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:15: span:hover { Are these hover styles neessary? https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:39: <br><br><br> Are all these BRs necessary? Can you take them out? At a minimum, inconsistent <br> vs <br /> vs <br/> should be cleaned up...
Please review latest patch. https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:6: body { margin: 2em; } On 2015/03/27 17:42:26, leviw wrote: > Why? Was added for manual test. Removed it. https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:15: span:hover { On 2015/03/27 17:42:26, leviw wrote: > Are these hover styles neessary? Was added for manual test. Removed it. https://codereview.chromium.org/1034433003/diff/80001/LayoutTests/fast/border... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:39: <br><br><br> On 2015/03/27 17:42:26, leviw wrote: > Are all these BRs necessary? Can you take them out? > > At a minimum, inconsistent <br> vs <br /> vs <br/> should be cleaned up... Unnecessary <br/> are removed.
https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:24: <br/><br/><br/> Why not just put the tests inside of blocks instead separating inlines with brs? https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:26: <br/><br/><br/> Ditto.
@leviw : Incorporated review comments please review. https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:24: <br/><br/><br/> On 2015/03/30 20:45:57, leviw wrote: > Why not just put the tests inside of blocks instead separating inlines with brs? Added <div> tag with margin to separate test and avoid overlapping. After modification behavior of test is same as of FF. https://codereview.chromium.org/1034433003/diff/100001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:26: <br/><br/><br/> On 2015/03/30 20:45:57, leviw wrote: > Ditto. Added <div> tag with margin to separate test and avoid overlapping. After modification behavior of test is same as of FF.
lgtm https://codereview.chromium.org/1034433003/diff/120001/LayoutTests/fast/borde... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/120001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:34: <span id="D" style="padding: 2em;">D<label id="E" href="#" style="padding: 1em;">E</label></span> Is the href needed?
@leviw : Updated layout test as per review comment. Please review latest patch. https://codereview.chromium.org/1034433003/diff/120001/LayoutTests/fast/borde... File LayoutTests/fast/borders/border-hittest-inlineFlowBox.html (right): https://codereview.chromium.org/1034433003/diff/120001/LayoutTests/fast/borde... LayoutTests/fast/borders/border-hittest-inlineFlowBox.html:34: <span id="D" style="padding: 2em;">D<label id="E" href="#" style="padding: 1em;">E</label></span> On 2015/03/31 18:00:19, leviw wrote: > Is the href needed? No,Removed it.
Updated your description for you to correct some mistakes... lgtm.
On 2015/04/01 20:13:04, leviw wrote: > Updated your description for you to correct some mistakes... lgtm. Thanks for review.
The CQ bit was checked by abhijeet.k@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from davve@opera.com Link to the patchset: https://codereview.chromium.org/1034433003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1034433003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192994 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
