Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
685963002:
We need to account for culled inline parents of the hit-tested nodes.(Reland) (Closed)
|
Created:
6 years, 1 month ago by Miyoung Shin(g) Modified:
5 years, 10 months ago CC:
blink-reviews, blink-reviews-rendering, 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. |
DescriptionWe need to account for culled inline parents of the hit-tested nodes.(Reland)
There are two kind of problem.
One is that we get the wrong hit-test result from culled inlines
because we try to overrun checking them until top of parent.
The other is that mouse events aren't fired when there is the
element culled inline like label element that doesn't has any
background color or border or self-layer.
This patch makes hit-test include not only area-based hit-tests
but also point-based ones at the place handling culled inline.
More information: https://docs.google.com/presentation/d/1KlT_FQxCTGeH-lPWjzXnPdZ3Bb1p7GL-IwoP-SfGvEo/edit#slide=id.g535467cad_40
BUG=312199, 435483
R=rbyers@chromium.org, pdr@chromium.org, leviw@chromium.org
TEST=fast/events/hit-test-culled_inline.html
=fast/dom/nodesFromRect/nodesFromRect-culled-inlines-between-silblings.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187593
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 19
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Messages
Total messages: 58 (10 generated)
dstockwell@, PTAL
dstockwell@chromium.org changed reviewers: + tkent@chromium.org
I'm not very familiar with hit-testing, but this doesn't seem like the right fix for issue 312199. This appears as though it would change behavior in general and not just for the case of <input> with <label>.
On 2014/10/29 02:46:48, dstockwell wrote: > I'm not very familiar with hit-testing, but this doesn't seem like the right fix > for issue 312199. This appears as though it would change behavior in general and > not just for the case of <input> with <label>. Right, It's intended. Because this problem can be happened at all elements culled inline. But my test case seems like caring about only <label> and <input>. If you think that TC is not in general, I would change TC.
tkent@, PTAL, I'm not sure why win_blink_rel is failed. When I test manually http/tests/media/media-source/mediasource-play-then-seek-back.html in my local windows environment, it is OK. Could you run a trybot for win_blink_rel again ? This is the log with my patch. ------------------- Log ----------------------- C:\Dev_cr\chromium\src\third_party\WebKit>Tools\Scripts\run-webkit-tests --nocheck-sys-deps LayoutTests\http\tests\media\media-source\mediasource-play-then-seek-back.html Using port 'win-win7' Test configuration: <win7, x86, release> View the test results at file://C:\Dev_cr\chromium\src\webkit\Release\layout-test-results/results.html View the archived results dashboard at file://C:\Dev_cr\chromium\src\webkit\Release\layout-test-results/dashboard.html Baseline search path: win -> generic Using Release build Pixel tests enabled Regular timeout: 6000, slow test timeout: 30000 Command line: C:\Dev_cr\chromium\src\out\Release\content_shell.exe --dump-render-tree --enable-direct-write --enable-crash-reporter --crash-dumps-dir=C:\Dev_cr\chromium\src\out\Release\crash-dumps - Found 1 test; running 1, skipping 0. Ruby is not installed; can't generate pretty patches. Running 1 content_shell. The test ran as expected.
tkent@chromium.org changed reviewers: - tkent@chromium.org
I'm not familiar with this code. Please find an appropriate reviewer.
myid.o.shin@gmail.com changed reviewers: + pdr@chromium.org - dstockwell@chromium.org
pdr@, I'm not sure that you are familiar with hit-testing. It's really hard to find reviewer because origin code was from WebKit and at that time I think there was no any reviewer about it on Blink. Could you take a look ?
On 2014/10/31 at 11:55:19, myid.o.shin wrote: > pdr@, > I'm not sure that you are familiar with hit-testing. > It's really hard to find reviewer because origin code was from WebKit and at that time I think there was no any reviewer about it on Blink. > Could you take a look ? I can give it a shot. Can you explain why the inline box was culled to begin with? My understanding of the culling optimization [1] is that we omit inline boxes that will not have an effect. Given that, the root bug seems to be that this inline was incorrectly culled to begin with. Does this happen with other elements too? Compared to divs and spans, labels and checkboxes are relatively obscure. This check could end up being expensive. [1] https://bugs.webkit.org/show_bug.cgi?id=88376#c25 Nit: should we just change the comment to say "We need to account for culled inline parents of the hit-tested nodes." ?
> Can you explain why the inline box was culled to begin with? My understanding of > the culling optimization [1] is that we omit inline boxes that will not have an > effect. Given that, the root bug seems to be that this inline was incorrectly > culled to begin with. RenderInline.cpp:222 bool alwaysCreateLineBoxesNew = hasSelfPaintingLayer() || hasBoxDecorationBackground() || newStyle->hasPadding() || newStyle->hasMargin() || newStyle->hasOutline(); The condition to cull inline boxes is like above. At first, I thought also that this inline was incorrectly culled to begin with. But I realized we wouldn't get any good effect by culled inline boxes if we couldn't cull the inline box that has transparent background. > Does this happen with other elements too? Compared to divs and spans, labels and > checkboxes are relatively obscure. This check could end up being expensive. Yes, this can happen with other elements that has transparent baground color. > Nit: should we just change the comment to say "We need to account for culled > inline parents of the hit-tested nodes." ? OK I got it
On 2014/11/03 at 02:17:01, myid.o.shin wrote: > > Does this happen with other elements too? Compared to divs and spans, labels and > > checkboxes are relatively obscure. This check could end up being expensive. > > Yes, this can happen with other elements that has transparent baground color. Are you sure? Can you add a test for divs or spans?
On 2014/11/07 02:58:02, pdr wrote: > On 2014/11/03 at 02:17:01, myid.o.shin wrote: > > > Does this happen with other elements too? Compared to divs and spans, labels > and > > > checkboxes are relatively obscure. This check could end up being expensive. > > > > Yes, this can happen with other elements that has transparent background color. > > Are you sure? Can you add a test for divs or spans? pdr@, I've added a test for general inline elements at Patch Set 2. e.g. <div><span id="span"><em id="em">Click the empty area</em><em>in middle</em></span></div> If <span> has transparent background color and <em> has margin, we couldn't get the hit-test on margin area of <em>. I think we should get the result of hit-test from <span>.
On 2014/11/08 at 03:40:55, myid.o.shin wrote: > On 2014/11/07 02:58:02, pdr wrote: > > On 2014/11/03 at 02:17:01, myid.o.shin wrote: > > > > Does this happen with other elements too? Compared to divs and spans, labels > > and > > > > checkboxes are relatively obscure. This check could end up being expensive. > > > > > > Yes, this can happen with other elements that has transparent background color. > > > > Are you sure? Can you add a test for divs or spans? > > pdr@, I've added a test for general inline elements at Patch Set 2. > > e.g. > <div><span id="span"><em id="em">Click the empty area</em><em>in middle</em></span></div> > > If <span> has transparent background color and <em> has margin, > we couldn't get the hit-test on margin area of <em>. > I think we should get the result of hit-test from <span>. I tried this in Firefox and they do not hit test the empty space in the <em> case either. Are you sure this is a bug? The <label> example works in firefox and not in chrome so there may be a bug there, but the <em> example works the same in both browsers.
> I tried this in Firefox and they do not hit test the empty space in the <em> > case either. Are you sure this is a bug? The <label> example works in firefox > and not in chrome so there may be a bug there, but the <em> example works the > same in both browsers. I tested my test case in FireFox on Linux and Mac, it works well. It means that <span> a parent of <em> is, is hit. I think you can see easily whether it is works well or not from cursor type, when you move the mouse pointer on margin area of <em>.
pdr@chromium.org changed reviewers: + leviw@chromium.org, rbyers@chromium.org
On 2014/11/08 at 04:11:07, myid.o.shin wrote: > > I tried this in Firefox and they do not hit test the empty space in the <em> > > case either. Are you sure this is a bug? The <label> example works in firefox > > and not in chrome so there may be a bug there, but the <em> example works the > > same in both browsers. > > I tested my test case in FireFox on Linux and Mac, it works well. It means that <span> a parent of <em> is, is hit. > I think you can see easily whether it is works well or not from cursor type, when you move the mouse pointer on margin area of <em>. I misunderstood and you are correct. I created this little fiddle to demonstrate the difference: http://jsfiddle.net/progers/1pk6tqb8/ I now realize this will affect a lot of sites and a lot of content. I am not sure if we could land such a change due to sites depending on the previous behavior. leviw and rbyers, do you have thoughts on this patch?
On 2014/11/08 04:27:18, pdr wrote: > On 2014/11/08 at 04:11:07, myid.o.shin wrote: > > > I tried this in Firefox and they do not hit test the empty space in the <em> > > > case either. Are you sure this is a bug? The <label> example works in > firefox > > > and not in chrome so there may be a bug there, but the <em> example works > the > > > same in both browsers. > > > > I tested my test case in FireFox on Linux and Mac, it works well. It means > that <span> a parent of <em> is, is hit. > > I think you can see easily whether it is works well or not from cursor type, > when you move the mouse pointer on margin area of <em>. > > I misunderstood and you are correct. I created this little fiddle to demonstrate > the difference: http://jsfiddle.net/progers/1pk6tqb8/ > > I now realize this will affect a lot of sites and a lot of content. I am not > sure if we could land such a change due to sites depending on the previous > behavior. > > leviw and rbyers, do you have thoughts on this patch? Interesting - good find. I don't know anything about the culling optimization, but here's the pros (+) and cons (-) I see here: + This corrects behavior that certainly seems like a bug (as pdr's fiddle shows). Eg. Element.getClientRects returns entries covering the space between the ems, why shouldn't hit testing include it too? + It matches Firefox + Treating point and rect-based hit tests the same here makes sense (I don't see any reasonable justification for treating them differently - in general doing so is a bug farm in itself) - IE 10 and (not surprisingly) Safari appear to also have the bug - I'm concerned about increasing the cost of hit testing. pdr: When you said it'll affect a lot of sites, do you mean that many hit tests will be affected? I can see why many sites would have area that could be affected, but is the area where hit tests would actually take a different path likely to be non-trivial? Miyoung: perhaps it's worth looking up who added the case for the rect-based hit-test. Perhaps they had a justification for not doing the same thing for point-based ones? Regardless it would be valuable to see their test case. Regardless, this makes enough conceptual sense to me (without knowing the details), that I'm OK with attempting to land it (at least I haven't heard any justification why we shouldn't be trying to fix this bug). If either perf or compat is a real problem, we should find out quickly enough and can revert and learn something.
https://codereview.chromium.org/685963002/diff/20001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-hit-test-culled_inline.html (right): https://codereview.chromium.org/685963002/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-hit-test-culled_inline.html:19: label.addEventListener('click', function() { labelClicked = true; }); There are two test cases being tested simultaneously here. Please refactor the test into a helper function and call it twice with the two different candidate notes. This strengthens the test to, for example, validate that your getting the click on the element you expect when you expect it. This also makes it easy to add more test cases if we find these two aren't sufficiently representative.
On 2014/11/10 at 18:40:02, rbyers wrote: > On 2014/11/08 04:27:18, pdr wrote: > > On 2014/11/08 at 04:11:07, myid.o.shin wrote: > > > > I tried this in Firefox and they do not hit test the empty space in the <em> > > > > case either. Are you sure this is a bug? The <label> example works in > > firefox > > > > and not in chrome so there may be a bug there, but the <em> example works > > the > > > > same in both browsers. > > > > > > I tested my test case in FireFox on Linux and Mac, it works well. It means > > that <span> a parent of <em> is, is hit. > > > I think you can see easily whether it is works well or not from cursor type, > > when you move the mouse pointer on margin area of <em>. > > > > I misunderstood and you are correct. I created this little fiddle to demonstrate > > the difference: http://jsfiddle.net/progers/1pk6tqb8/ > > > > I now realize this will affect a lot of sites and a lot of content. I am not > > sure if we could land such a change due to sites depending on the previous > > behavior. > > > > leviw and rbyers, do you have thoughts on this patch? > > Interesting - good find. I don't know anything about the culling optimization, but here's the pros (+) and cons (-) I see here: > + This corrects behavior that certainly seems like a bug (as pdr's fiddle shows). Eg. Element.getClientRects returns entries covering the space between the ems, why shouldn't hit testing include it too? > + It matches Firefox > + Treating point and rect-based hit tests the same here makes sense (I don't see any reasonable justification for treating them differently - in general doing so is a bug farm in itself) > - IE 10 and (not surprisingly) Safari appear to also have the bug > - I'm concerned about increasing the cost of hit testing. I wouldn't expect a large impact on performance, since we don't normally have to hit test multiple line trees in a given test. > > pdr: When you said it'll affect a lot of sites, do you mean that many hit tests will be affected? I can see why many sites would have area that could be affected, but is the area where hit tests would actually take a different path likely to be non-trivial? We would only see a difference for sites that happen to hit our culled inline optimization, and have event handlers on them. In those cases, we'd undoubtably be doing what the author expected to begin with. > > Miyoung: perhaps it's worth looking up who added the case for the rect-based hit-test. Perhaps they had a justification for not doing the same thing for point-based ones? Regardless it would be valuable to see their test case. > > Regardless, this makes enough conceptual sense to me (without knowing the details), that I'm OK with attempting to land it (at least I haven't heard any justification why we shouldn't be trying to fix this bug). If either perf or compat is a real problem, we should find out quickly enough and can revert and learn something. Agreed. The code change is fine with me.
Sounds like we have quorum :) Lets give it a go once the test is updated.
Thanks, I've uploaded the patch to change the test case. PTAL
https://codereview.chromium.org/685963002/diff/40001/LayoutTests/fast/events/... File LayoutTests/fast/events/hit-test-culled_inline.html (right): https://codereview.chromium.org/685963002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/hit-test-culled_inline.html:20: debug(parent.nodeName + ' that is a parent of ' + element.nodeName + ' is clicked.'); Thanks for the refactor, but now we've lost the explicit PASS/FAIL result from the test. Someone changing the test output might not fully understand what you expected to happen. Maybe set a 'gotClick' bool here, and add a shouldBeTrue('gotClick') below the mouseUp. shouldBe* requires using globals, so you'll have to re-initialize gotClick to false at the start of each call to hitTest. https://codereview.chromium.org/685963002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/hit-test-culled_inline.html:28: hitTest('checkbox'); you've also lost your test that verifies the checkbox got clicked. That seems nice to have. You can re-add it here (and even add a shouldBeFalse('checkbox.clicked' above to really verify that it's the hitTest doing the click). Alternately if you only want to test hit test behavior (and not all of click) you can simplify this test by using elementFromPoint instead of eventSender. They use the same hit-test logic under the covers.
rbyers@,
Thank you for your good tip.
I've updated the test case to use elementFromPoint, shouldBeTrue('gotClick') and
shouldBeTrue('checkbox.clicked') you suggested.
Thanks for improving the test. The improvement appears to uncovered a bug that needs investigating! https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... File LayoutTests/fast/events/hit-test-culled_inline-expected.txt (right): https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... LayoutTests/fast/events/hit-test-culled_inline-expected.txt:10: LABEL that is a parent of INPUT is clicked. This output indicates the click handler is getting invoked twice, which could be a serious bug! Any idea what's happening
On 2014/11/13 19:37:01, Rick Byers wrote: > Thanks for improving the test. The improvement appears to uncovered a bug that > needs investigating! > > https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... > File LayoutTests/fast/events/hit-test-culled_inline-expected.txt (right): > > https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... > LayoutTests/fast/events/hit-test-culled_inline-expected.txt:10: LABEL that is a > parent of INPUT is clicked. > This output indicates the click handler is getting invoked twice, which could be > a serious bug! Any idea what's happening Currently the sequence is, 1. click event occurs on <label> by user behavior. 2. <label> clicked by user makes a click be simulated for <input> 3. click event simulated by <label> occurs on <input> 4. click event occurs on <label> by bubbling event of <input> If I understand correctly, we run synthetic click activation steps[1] on an element. [1] http://www.w3.org/TR/html5/dom.html#run-synthetic-click-activation-steps When I test to click <label>, I can see the same behavior on all of the browser(Chrome, FF, IE, Safari). I think this is not a bug. PTAL.
On 2014/11/14 11:14:13, Miyoung Shin(g) wrote: > On 2014/11/13 19:37:01, Rick Byers wrote: > > Thanks for improving the test. The improvement appears to uncovered a bug > that > > needs investigating! > > > > > https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... > > File LayoutTests/fast/events/hit-test-culled_inline-expected.txt (right): > > > > > https://codereview.chromium.org/685963002/diff/60001/LayoutTests/fast/events/... > > LayoutTests/fast/events/hit-test-culled_inline-expected.txt:10: LABEL that is > a > > parent of INPUT is clicked. > > This output indicates the click handler is getting invoked twice, which could > be > > a serious bug! Any idea what's happening > > Currently the sequence is, > 1. click event occurs on <label> by user behavior. > 2. <label> clicked by user makes a click be simulated for <input> > 3. click event simulated by <label> occurs on <input> > 4. click event occurs on <label> by bubbling event of <input> > > If I understand correctly, we run synthetic click activation steps[1] on an > element. > [1] http://www.w3.org/TR/html5/dom.html#run-synthetic-click-activation-steps > > When I test to click <label>, I can see the same behavior on all of the > browser(Chrome, FF, IE, Safari). > I think this is not a bug. PTAL. Fascinating, thank you! I played with this as well. At first I assumed this would be really bad - it means click handlers on the label or some ancestor would get invoked twice. But I guess in many cases we'd need to trigger click handlers on the checkbox, and we probably can't reasonable stop propagation of that click. So I guess it makes sense. Thanks - LGTM.
The CQ bit was checked by myid.o.shin@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/685963002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/3...)
On 2014/11/16 at 16:15:30, commit-bot wrote: > Try jobs failed on following builders: > linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/3...) This failure seems unrelated. Lets try again.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/685963002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 185417
pdr@, rbyers@, PTAL.
pdr@, rbyers@, PTAL.
On 2014/12/04 06:53:52, Miyoung Shin(g) wrote: > pdr@, rbyers@, PTAL. Sorry for the delay. I defer to pdr@ - he's got a better understanding of line box rendering than I do. Please add a link in the CL description to your excellent doc explaining the situation: https://docs.google.com/presentation/d/1KlT_FQxCTGeH-lPWjzXnPdZ3Bb1p7GL-IwoP-....
On 2014/12/04 at 16:42:10, rbyers wrote: > On 2014/12/04 06:53:52, Miyoung Shin(g) wrote: > > pdr@, rbyers@, PTAL. > > Sorry for the delay. I defer to pdr@ - he's got a better understanding of line box rendering than I do. Please add a link in the CL description to your excellent doc explaining the situation: https://docs.google.com/presentation/d/1KlT_FQxCTGeH-lPWjzXnPdZ3Bb1p7GL-IwoP-.... Sorry, I didn't get to this today. I'll review it tomorrow.
On 2014/12/05 01:40:43, pdr wrote: > On 2014/12/04 at 16:42:10, rbyers wrote: > > On 2014/12/04 06:53:52, Miyoung Shin(g) wrote: > > > pdr@, rbyers@, PTAL. > > > > Sorry for the delay. I defer to pdr@ - he's got a better understanding of > line box rendering than I do. Please add a link in the CL description to your > excellent doc explaining the situation: > https://docs.google.com/presentation/d/1KlT_FQxCTGeH-lPWjzXnPdZ3Bb1p7GL-IwoP-.... > > Sorry, I didn't get to this today. I'll review it tomorrow. pdr@, I need to check my patch more. As a result of webkit_tests, failures: editing/selection/caret-at-bidi-boundary.html I came up with the problem that this patch doesn't consider the case of complex Bidi . I will check it.
https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1031: // Check all of children including culled inlines. This may be fantastic code but I was unable to understand what it's doing in 20 minutes of studying it. To have a maintainable codebase we have to clean this up so a reasonable person can understand it based on the code and comments alone. This isn't necessarily your fault but we'll have to make it understandable to get it through review. I've added some questions below. I recommend we improve this top comment to better describe what's going on. In your suggested diagram, steps 2 and 3, why do we stop at text -> a -> span instead of going up to the font? https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1035: InlineBox* currentBox = lastChild(); Why do we need to track the current box and current renderer? https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1041: while (escapedRenderer) { What is this inner loop really doing? Would a for loop be more understandable here? https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1042: if (currentBox->renderer().isText() || !currentBox->boxModelObject()->hasSelfPaintingLayer()) { isText() and !hasSelfPaintingLayer() seem completely unrelated. What is this conditional really checking? https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1058: currentRenderer = currentRenderer->previousInPreOrder(); previousInPreOrder could be rather expensive in this loop.
https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1031: // Check all of children including culled inlines. On 2014/12/05 21:21:01, pdr wrote: > This may be fantastic code but I was unable to understand what it's doing in 20 > minutes of studying it. To have a maintainable codebase we have to clean this up > so a reasonable person can understand it based on the code and comments alone. > This isn't necessarily your fault but we'll have to make it understandable to > get it through review. I agree with your opinion. I will add the comment about what we do in more detail. > > I've added some questions below. I recommend we improve this top comment to > better describe what's going on. > > In your suggested diagram, steps 2 and 3, why do we stop at text -> a -> span > instead of going up to the font? Because we should check first the children before parent. https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1035: InlineBox* currentBox = lastChild(); On 2014/12/05 21:21:01, pdr wrote: > Why do we need to track the current box and current renderer? current box is for hit-testing inline boxes in inlineflowbox. current renderer is for hit-testing culled parent of inline box. But I will update the new patch because I should consider BIDI derection of inline box. And I will roll-back the main loop based on current box as it was. https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1041: while (escapedRenderer) { On 2014/12/05 21:21:01, pdr wrote: > What is this inner loop really doing? > > Would a for loop be more understandable here? I will re-factor this and remove it. If I remain the comment in more detail, the second loop is more understandable. https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1042: if (currentBox->renderer().isText() || !currentBox->boxModelObject()->hasSelfPaintingLayer()) { On 2014/12/05 21:21:01, pdr wrote: > isText() and !hasSelfPaintingLayer() seem completely unrelated. What is this > conditional really checking? It was from origin code and I tried to keep them. I'm not that sure about them. https://codereview.chromium.org/685963002/diff/80001/Source/core/rendering/In... Source/core/rendering/InlineFlowBox.cpp:1058: currentRenderer = currentRenderer->previousInPreOrder(); On 2014/12/05 21:21:01, pdr wrote: > previousInPreOrder could be rather expensive in this loop. I will re-factor this and remove it. the next patch will be more cheaper
pdr@, I've uploaded the patch that includes to consider BIDI direction, PTAL.
I apologize for the slow review. I like this patch and I think it's going in the right direction. https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/dom/no... File LayoutTests/fast/dom/nodesFromRect/nodesFromRect-culled-inlines-between-silblings.html (right): https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/dom/no... LayoutTests/fast/dom/nodesFromRect/nodesFromRect-culled-inlines-between-silblings.html:1: <!DOCTYPE html> This example doesn't seem to work with your patch: http://jsfiddle.net/progers/1pk6tqb8/embedded/result. Do we need more test coverage, or is the example not correct? https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/hit-test-culled_inline.html (right): https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/hit-test-culled_inline.html:1: <!DOCTYPE html> Please rename this test hit-test-culled-inline.html https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1031: // Check all of children including culled inlines. These comments helped me a lot, thank you :) I tried to rephrase what the algorithm does in a simpler way. What do you think about this comment instead? // We need to hit test both our inline children (InlineBoxes) and culled inlines (RenderObjects). We // check our inlines in the same order as line layout but for each inline we additionally need to // hit test its culled inline parents. While hit testing culled inline parents, we can stop once we reach // a non-inline parent or a culled inline associated with a different inline box. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1047: if (curr->renderer().isText() || !curr->boxModelObject()->hasSelfPaintingLayer()) { I think I understand what this is doing now. If you look at InlineFlowBoxPainter::paint, this check is just mirroring how the inlines are painted. I think we can replace this check with the following: // Layers will handle hit testing themselves. if (curr->boxModelObject() && curr->boxModelObject()->hasSelfPaintingLayer()) continue; https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1054: if (!prev || curr->renderer() != prev->renderer()) { Can you help me understand the "curr->renderer() != prev->renderer()" check? What is this trying to check for? It may make your algorithm a little easier to read if you refactor this like so: // Short comment here explaining what the check is doing. if (prev && curr->renderer() == prev->renderer()) continue; https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->style()->isLeftToRightDirection()) { Imagine a case where you have three culled inline siblings and you are trying to hit test the middle one. Wont this check will prevent you from ever hit testing the middle one? Can you add a test of this case? (or, just add a few more tests to nodesFromRect-culled-inlines-between-silblings.html) Additionally, the bidi check here doesn't make sense to me because the text direction should be unrelated to hit test order. Can you help me understand a case where we do need to consider the direction? https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1066: if (culledParent && culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset)) { In the worst case, we'll iterate up the culled inline renderers until we hit the parent renderer. Is it ever possible for culledParent to be null? We may just want to add ASSERT(currentParent) right under "culledParent = culledParent->parent();"
https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/dom/no... File LayoutTests/fast/dom/nodesFromRect/nodesFromRect-culled-inlines-between-silblings.html (right): https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/dom/no... LayoutTests/fast/dom/nodesFromRect/nodesFromRect-culled-inlines-between-silblings.html:1: <!DOCTYPE html> On 2014/12/10 07:16:19, pdr wrote: > This example doesn't seem to work with your patch: > http://jsfiddle.net/progers/1pk6tqb8/embedded/result. Do we need more test > coverage, or is the example not correct? I added two kind of tests in this patch. One is this for hit-test based on rect. Another is hit-test-culled_inline.html for hit-test based on point and it covers your example. https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/hit-test-culled_inline.html (right): https://codereview.chromium.org/685963002/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/hit-test-culled_inline.html:1: <!DOCTYPE html> On 2014/12/10 07:16:19, pdr wrote: > Please rename this test hit-test-culled-inline.html Ok. I will rethink which of name is proper. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1031: // Check all of children including culled inlines. On 2014/12/10 07:16:19, pdr wrote: > These comments helped me a lot, thank you :) > > I tried to rephrase what the algorithm does in a simpler way. What do you think > about this comment instead? > // We need to hit test both our inline children (InlineBoxes) and culled inlines > (RenderObjects). We > // check our inlines in the same order as line layout but for each inline we > additionally need to > // hit test its culled inline parents. While hit testing culled inline parents, > we can stop once we reach > // a non-inline parent or a culled inline associated with a different inline > box. thank you. it's perfect. Should I replace all of comment ? or remain Behavior description ? https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1047: if (curr->renderer().isText() || !curr->boxModelObject()->hasSelfPaintingLayer()) { On 2014/12/10 07:16:19, pdr wrote: > I think I understand what this is doing now. If you look at > InlineFlowBoxPainter::paint, this check is just mirroring how the inlines are > painted. I think we can replace this check with the following: > > // Layers will handle hit testing themselves. > if (curr->boxModelObject() && curr->boxModelObject()->hasSelfPaintingLayer()) > continue; Ok, I will try. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1054: if (!prev || curr->renderer() != prev->renderer()) { On 2014/12/10 07:16:19, pdr wrote: > Can you help me understand the "curr->renderer() != prev->renderer()" check? > What is this trying to check for? current inlinebox and next inlinbox in the main loop can be the same renderer. ex) <span> text1 text2 <span> if text2 is the current inlinebox, then text1 would be the next inlinebox in the main loop, and they are the same renderer. "curr->renderer() != prev->renderer() is to check if renderer is changed. if not change(text2 -> text1), we should yield the hit-test to next inlinebox. > It may make your algorithm a little easier to > read if you refactor this like so: > > // Short comment here explaining what the check is doing. > if (prev && curr->renderer() == prev->renderer()) > continue; OK https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->style()->isLeftToRightDirection()) { On 2014/12/10 07:16:19, pdr wrote: > Imagine a case where you have three culled inline siblings and you are trying to > hit test the middle one. Wont this check will prevent you from ever hit testing > the middle one? Can you add a test of this case? (or, just add a few more tests > to nodesFromRect-culled-inlines-between-silblings.html) Ah... you're right. I was back on track. I should reconsider this case and I will add the test to nodesFromRect-culled-inlines-between-silblings.html for this case. > > Additionally, the bidi check here doesn't make sense to me because the text > direction should be unrelated to hit test order. Can you help me understand a > case where we do need to consider the direction? It is for the element's direction, not for text. we arrange inlineboxes due to bidi derection. ex) in editing/selection/caret-at-bidi-boundary.html <dt><span dir="rtl">12<b>קל43</b></span>ab</dt> inlinebox flow is: ab -> 12 -> קל43 if there isn't rtl, then the flow is, ab -> קל43 -> 12 in case of '12' with rtl, we should yield the hit-test to next sibling('קל43'), so I need to consider the direction. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1066: if (culledParent && culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset)) { On 2014/12/10 07:16:19, pdr wrote: > In the worst case, we'll iterate up the culled inline renderers until we hit the > parent renderer. Is it ever possible for culledParent to be null? We may just > want to add ASSERT(currentParent) right under "culledParent = > culledParent->parent();" I think, it's impossible to be null. I will add ASSERT(culledParent).
https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1031: // Check all of children including culled inlines. On 2014/12/12 at 14:46:57, Miyoung Shin(g) wrote: > On 2014/12/10 07:16:19, pdr wrote: > > These comments helped me a lot, thank you :) > > > > I tried to rephrase what the algorithm does in a simpler way. What do you think > > about this comment instead? > > // We need to hit test both our inline children (InlineBoxes) and culled inlines > > (RenderObjects). We > > // check our inlines in the same order as line layout but for each inline we > > additionally need to > > // hit test its culled inline parents. While hit testing culled inline parents, > > we can stop once we reach > > // a non-inline parent or a culled inline associated with a different inline > > box. > > thank you. it's perfect. > Should I replace all of comment ? or remain Behavior description ? Lets replace the entire comment. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1054: if (!prev || curr->renderer() != prev->renderer()) { On 2014/12/12 at 14:46:57, Miyoung Shin(g) wrote: > On 2014/12/10 07:16:19, pdr wrote: > > Can you help me understand the "curr->renderer() != prev->renderer()" check? > > What is this trying to check for? > > current inlinebox and next inlinbox in the main loop can be the same renderer. > ex) > <span> > text1 > text2 > <span> > if text2 is the current inlinebox, then text1 would be the next inlinebox in the main loop, and they are the same renderer. > > "curr->renderer() != prev->renderer() is to check if renderer is changed. > if not change(text2 -> text1), we should yield the hit-test to next inlinebox. Ah, I see. Should this check be moved up then, so it's the first statement in the for loop? https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->style()->isLeftToRightDirection()) { On 2014/12/12 at 14:46:57, Miyoung Shin(g) wrote: > On 2014/12/10 07:16:19, pdr wrote: > > Imagine a case where you have three culled inline siblings and you are trying to > > hit test the middle one. Wont this check will prevent you from ever hit testing > > the middle one? Can you add a test of this case? (or, just add a few more tests > > to nodesFromRect-culled-inlines-between-silblings.html) > > Ah... you're right. I was back on track. > I should reconsider this case and I will add the test to nodesFromRect-culled-inlines-between-silblings.html for this case. > > > > > Additionally, the bidi check here doesn't make sense to me because the text > > direction should be unrelated to hit test order. Can you help me understand a > > case where we do need to consider the direction? > > It is for the element's direction, not for text. > we arrange inlineboxes due to bidi derection. > > ex) in editing/selection/caret-at-bidi-boundary.html > <dt><span dir="rtl">12<b>קל43</b></span>ab</dt> > > inlinebox flow is: > ab > -> 12 > -> קל43 > > if there isn't rtl, then the flow is, > ab > -> קל43 > -> 12 > > in case of '12' with rtl, we should yield the hit-test to next sibling('קל43'), so I need to consider the direction. I still don't really understand why the order matters. Won't hitTestCulledInline return true for one of them and false for the other? How did the old code handle this properly without checking direction?
https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1054: if (!prev || curr->renderer() != prev->renderer()) { On 2014/12/12 21:21:08, pdr wrote: > On 2014/12/12 at 14:46:57, Miyoung Shin(g) wrote: > > On 2014/12/10 07:16:19, pdr wrote: > > > Can you help me understand the "curr->renderer() != prev->renderer()" check? > > > What is this trying to check for? > > > > current inlinebox and next inlinbox in the main loop can be the same renderer. > > ex) > > <span> > > text1 > > text2 > > <span> > > if text2 is the current inlinebox, then text1 would be the next inlinebox in > the main loop, and they are the same renderer. > > > > "curr->renderer() != prev->renderer() is to check if renderer is changed. > > if not change(text2 -> text1), we should yield the hit-test to next inlinebox. > > Ah, I see. Should this check be moved up then, so it's the first statement in > the for loop? No, we should call inlinebox::nodeAtPoint for each inlinebox before checking this condition. https://codereview.chromium.org/685963002/diff/100001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->style()->isLeftToRightDirection()) { On 2014/12/12 21:21:08, pdr wrote: > On 2014/12/12 at 14:46:57, Miyoung Shin(g) wrote: > > On 2014/12/10 07:16:19, pdr wrote: > > > Imagine a case where you have three culled inline siblings and you are > trying to > > > hit test the middle one. Wont this check will prevent you from ever hit > testing > > > the middle one? Can you add a test of this case? (or, just add a few more > tests > > > to nodesFromRect-culled-inlines-between-silblings.html) > > > > Ah... you're right. I was back on track. > > I should reconsider this case and I will add the test to > nodesFromRect-culled-inlines-between-silblings.html for this case. I realized we don't need to care three culled inline siblings you said. Culled inlines that needs the hit-test have a child inline box and we can do the hit-test for them in next loop. If culled inlines don't have a child inline box, we don't need the hit-test for them, because they don't have any layout. > > > > > > > > Additionally, the bidi check here doesn't make sense to me because the text > > > direction should be unrelated to hit test order. Can you help me understand > a > > > case where we do need to consider the direction? > > > > It is for the element's direction, not for text. > > we arrange inlineboxes due to bidi derection. > > > > ex) in editing/selection/caret-at-bidi-boundary.html > > <dt><span dir="rtl">12<b>קל43</b></span>ab</dt> > > > > inlinebox flow is: > > ab > > -> 12 > > -> קל43 > > > > if there isn't rtl, then the flow is, > > ab > > -> קל43 > > -> 12 > > > > in case of '12' with rtl, we should yield the hit-test to next > sibling('קל43'), so I need to consider the direction. > > I still don't really understand why the order matters. Won't hitTestCulledInline > return true for one of them and false for the other? How did the old code handle > this properly without checking direction? The old code has the problem that go up to the parent even if we should stop. Because old code calls isDescendantOf for whether we should stop but it couldn't check the some case as I explained in PPT. Also isDescendantOf is a little expensive. if we check a direction of current renderobject, we could know time when we stop without isDescendantOf.
pdr@, I've uploaded the patch to apply your comment. PTAL.
This is looking very good. I found a few more issues but I think we are getting close. https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1037: for (InlineBox* curr = lastChild(); curr; curr = prev) { This code is so much easier to understand than the old code! https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1049: // If current inlinebox's renderer and previous inlinebox's renderer is same, Minor nit: "// If [the] current inlinebox's renderer and [the] previous inlinebox's renderer" "// [are] the same, we should yield the hit=test to [the] previous inlinebox." https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1055: if (culledParent->style()->isLeftToRightDirection()) { I still don't understand the direction part here. Maybe a test would make this clearer--can you add a test for this (maybe nodesFromRect-culled-inlines-between-silblings-bidi.html) that fails without the isLeftToRightDirection check but passes with it? https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1055: if (culledParent->style()->isLeftToRightDirection()) { I still don't understand the direction part here. Maybe a test would make this clearer--can you add a test for this (maybe nodesFromRect-culled-inlines-between-silblings-bidi.html) that fails without the isLeftToRightDirection check but passes with it? https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->previousSibling()) This essentially checks if we can early-out because the inlinebox descendant of the previous sibling will handle walking up this chain of culled inlines. This is based on the assumption that the previous sibling must have inline box descendants which seems to be true most of the time. Unfortunately, in the case of text nodes we can have a previous sibling that does not have an inline box descendant. You can see this in the js fiddle I posted before: http://jsfiddle.net/progers/1pk6tqb8/1/embedded/result. Please add a testcase for this. We could solve this by adding a check such as: if (culledParent->previousSibling() && culledParent->previousSibling()->hasInlineBoxes()) But I wasn't able to find a function to do that. RenderObject::childrenInline or RenderObject::slowFirstChild() might work here. Another option might be to add an additional check like this: if (culledParent->previousSibling() && prev && prev->renderer().isDescendantOf(culledParent->previousSibling()))
https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1049: // If current inlinebox's renderer and previous inlinebox's renderer is same, On 2014/12/14 22:06:48, pdr wrote: > Minor nit: > "// If [the] current inlinebox's renderer and [the] previous inlinebox's > renderer" > "// [are] the same, we should yield the hit=test to [the] previous inlinebox." Thanks. https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1055: if (culledParent->style()->isLeftToRightDirection()) { On 2014/12/14 22:06:48, pdr wrote: > I still don't understand the direction part here. Maybe a test would make this > clearer--can you add a test for this (maybe > nodesFromRect-culled-inlines-between-silblings-bidi.html) that fails without the > isLeftToRightDirection check but passes with it? OK. I will add nodesFromRect-culled-inlines-between-silblings-bidi.html https://codereview.chromium.org/685963002/diff/120001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1056: if (culledParent->previousSibling()) On 2014/12/14 22:06:48, pdr wrote: > This essentially checks if we can early-out because the inlinebox descendant of > the previous sibling will handle walking up this chain of culled inlines. This > is based on the assumption that the previous sibling must have inline box > descendants which seems to be true most of the time. Unfortunately, in the case > of text nodes we can have a previous sibling that does not have an inline box > descendant. You can see this in the js fiddle I posted before: > http://jsfiddle.net/progers/1pk6tqb8/1/embedded/result. Please add a testcase > for this. I'm sorry. I think I get it now. I thought that event-on-culled_inline.html covers the test case you posted in the js fiddle. I realized that I have to add a space in my test case. before : <span id="clickme2"><em id="em"> after: <span id="clickme2"> <em id="em"> After changing TC, I found my problem and I caught up what you say. And event-on-culled_inline.html will cover your tc after updating the patch. > We could solve this by adding a check such as: > if (culledParent->previousSibling() && > culledParent->previousSibling()->hasInlineBoxes()) > But I wasn't able to find a function to do that. RenderObject::childrenInline or > RenderObject::slowFirstChild() might work here. > > Another option might be to add an additional check like this: > if (culledParent->previousSibling() && prev && > prev->renderer().isDescendantOf(culledParent->previousSibling())) Thank you. The second option works well.
Patchset #8 (id:140001) has been deleted
pdr@. could you take a look ?
pdr@chromium.org changed reviewers: + l.gombos@samsung.com
Miyoung Shin, this is excellent work; thank you for sticking with it. This was a difficult area of code for a new contributor and you knocked it out of the park. Thank you! LGTM (feel free to fix the small comment below, upload, and click commit without a second review.) https://codereview.chromium.org/685963002/diff/160001/Source/core/rendering/I... File Source/core/rendering/InlineFlowBox.cpp (right): https://codereview.chromium.org/685963002/diff/160001/Source/core/rendering/I... Source/core/rendering/InlineFlowBox.cpp:1049: RenderObject* culledParent = &curr->renderer(); Can you move this down above the while (true) line?
On 2014/12/20 06:55:31, pdr wrote: > Miyoung Shin, this is excellent work; thank you for sticking with it. This was a > difficult area of code for a new contributor and you knocked it out of the park. > Thank you! Thank you for your time and consideration.
The CQ bit was checked by myid.o.shin@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/685963002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187593 |
