[Chromium] Fix find in page rects for overflowing content.
Created attachment 179286 [details] Patch
Leandro, John could you take a look at this
Comment on attachment 179286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179286&action=review > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:59 > + } while (!renderer->hasOverflowClip() && !renderer->isRenderView()); Can we ensure renderer is always going to be non-NULL? > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:112 > + for (const RenderBlock* container = containingScrollable(renderer); container; Could you try this with non-scrollable containers that have a CSS transform? (e.g. a translation + a rotation)
Created attachment 179490 [details] Patch
I tried it as you suggested (please see how I modified the test case, to check that I understood you correctly) and it worked.
Comment on attachment 179490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179490&action=review LGTM, just a minor comment. > Source/WebKit/chromium/tests/data/find_in_page_frame.html:75 > +<div style="height:0px"> Could you double-check that the "transform" class properties are not overriding this height: 0px in any way? Transform defines height to be 100px.
Been chatting with Sami, who points out that perhaps what we really want to do is to normalise with respect to scrollable layers, rather than scrollable containing boxes. RenderLayer* RenderLayer::enclosingScrollableLayer() const might be what we want to use. Anything with a transformation will get it's own layer. The calls to localToAbsolute should be handling any transformations we need.
(In reply to comment #7) > Been chatting with Sami, who points out that perhaps what we really want to do is to normalise with respect to scrollable layers, rather than scrollable containing boxes. > > RenderLayer* RenderLayer::enclosingScrollableLayer() const > > might be what we want to use. Anything with a transformation will get it's own layer. > > The calls to localToAbsolute should be handling any transformations we need. That sounds ok, but I suggest to check it with jchaffraix since if I remember right he's the one who suggested using containingBlock. See https://bugs.webkit.org/show_bug.cgi?id=94343 for more details.
Comment on attachment 179490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179490&action=review >> Source/WebKit/chromium/tests/data/find_in_page_frame.html:75 >> +<div style="height:0px"> > > Could you double-check that the "transform" class properties are not overriding this height: 0px in any way? Transform defines height to be 100px. Thanks, I did check this and the transformed div has height 120px, as expected, but the child <div> does indeed have height 0px.
Hi Julian, I would appreciate your thoughts on this bug (also CC'd you on the downstream bug). The problem we are having is that the current find-in-page coordinate system is trying to normalise a RenderObject's coordinates to the maxLayoutOverflow() rect of its containing box. This fails if the containing box has zero height or width. An example of where this happens is http://www.fastclick.com/, where the <body> has height:0, which in this case is actually due to the DOCTYPE declaration. In general, however, the situation is easy to reproduce by having a height:0px <div> in the containing box. I've demonstrated this in the change to the webkit unit test in Source/WebKit/chromium/tests/data/find_in_page_frame.html . I think we actually don't want to normalise against the closest containing box. Rather, I think that we should normalize against the closest enclosing scrollable box, or the render view. This is because it is only really the scrollable content that requires special handling, so that the tick mark appears side by side with the box that contains the scrollable content, independent of whether the content is actually scrolled into view. Do you agree with Sami's suggestion to look for scrollable layers instead of scrollable containing boxes? This has the advantage that some code might be reused.
> The problem we are having is that the current find-in-page coordinate system is trying to normalise a RenderObject's coordinates to the maxLayoutOverflow() rect of its containing box. This fails if the containing box has zero height or width. Your analysis is wrong. The issue is not about the height / width being zero, it's about what we track into maxLayoutOverflow() which is completely different. As we don't track overhanging floats in all cases, you can end up with the wrong answer. > Do you agree with Sami's suggestion to look for scrollable layers instead of scrollable containing boxes? This has the advantage that some code might be reused. The only issue I would see with using RenderLayers is that RenderListBlock (used for displaying <select>) doesn't necessarily have a layer, yet it is a scrollable area. Not sure if you are displaying these renderers anyway but you should consider that. Apart from this case, you should be able to use scrollable layers / renderers (not sure why you / Sami are putting a difference between the 2).
Many thanks, Julian. Your comments make sense, and I think perhaps the patch is then fine as it is. Please could you review it? I tried an experiment with the <select>, as you suggested, and find in page does not find any matches within the select, so we don't need to worry about this particular case.
Comment on attachment 179490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179490&action=review > Many thanks, Julian. It's Julien, not Julian. Thanks. > Your comments make sense, and I think perhaps the patch is then fine as it is. Please could you review it? In the future, you should mark patches for review as such. > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:61 > +static const RenderBlock* containingScrollable(const RenderObject* renderer) > +{ > + // Trace up the containingBlocks until we reach either the render view or a scrollable object. > + if (renderer->isRenderView()) > + return 0; > + do { > + renderer = renderer->containingBlock(); > + } while (!renderer->hasOverflowClip() && !renderer->isRenderView()); > + return static_cast<const RenderBlock*>(renderer); > +} The current code is badly written: * I usually advise against do .. while as I find them less straightforward than a regular for / while. * the types are all wrong as you reuse |renderer| which is of type RenderObject (and not RenderBlock). * also we don't do static_cast<const RenderBlock*> for renderer (this should have been a red flag for you), we use the asserting toRenderBlock. If you use RenderLayer::enclosingScrollableLayer, you lose the guarantee that your associated renderer is a RenderBlock so I fear this will make the code worse but I can't tell until seeing the patch. > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:113 > + for (const RenderBlock* container = containingScrollable(renderer); container; > + container = containingScrollable(container)) { We don't have a wrap limit so this should be on one line. >>> Source/WebKit/chromium/tests/data/find_in_page_frame.html:75 >>> +<div style="height:0px"> >> >> Could you double-check that the "transform" class properties are not overriding this height: 0px in any way? Transform defines height to be 100px. > > Thanks, I did check this and the transformed div has height 120px, as expected, but the child <div> does indeed have height 0px. You should add some testing for <select> here.
Created attachment 180152 [details] Patch
Thanks, Julien. Hopefully, I've made the getScrollableContainer code more readable now. I realise that I don't need to have a separate check for isRenderView() at the beginning because getContainingBlock() should return NULL when run on a RenderView. I added some tests for <select>. These are trivial at the moment, because find in page doesn't actually report matches for the option text at all. This is the same behaviour as in Firefox, so I'm not sure if we want to change this behaviour. I am happy to try to create a enclosingScrollableLayer-based patch if you feel that we should also investigate this approach. Please let me know.
Ping. Any comments on the latest patch would be much appreciated, thanks!
Comment on attachment 180152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180152&action=review > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:52 > +static const RenderBlock* containingScrollable(const RenderObject* renderer) The name is missing a noun: what is a containing scrollable? Better suggestion for the name: enclosingScrollableAncestor, enclosingScrollableContainingBlock > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:56 > + while (container && !container->hasOverflowClip() && !container->isRenderView()) I don't think you need the |container| check unless you expect to be called on a detached tree, in which case calling containingBlock() is really not a good idea. > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:107 > + for (const RenderObject* renderer = baseContainer; renderer; ) { |renderer| is of type RenderBlock so we should tighten the type. > Source/WebKit/chromium/tests/WebFrameTest.cpp:909 > + // Find in <select> content. Expect not to find any matches. > + EXPECT_FALSE(frame->find(findIdentifier, WebString::fromUTF8("bar5"), options, false, 0)); > + // Confirm stopFinding(false) clears the selection. We usually prefer 'why' comments as none of the above explains what's the purpose of the cryptic calls. Like why is is important to clear the selection? Also your comment conflict with the comment above: // Confirm stopFinding(false) sets the selection on the found text. (which I guess is because you have no match but that's something to explain). > Source/WebKit/chromium/tests/data/find_in_page_frame.html:84 > +</select> You may want to add some tests with positioned elements as it will yield to results which you may find weird: <div style="position: relative; overflow: hidden; height: 100px;"> <div style="overflow: hidden; height: 50px"> <div style="position: absolute; top: 60px; height: 20px; width: 80px; background-color: green">result 18</div> </div> </div> (This will yield to a visible result that will be normalized to the most outer div)
Comment on attachment 180152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180152&action=review >> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:107 >> + for (const RenderObject* renderer = baseContainer; renderer; ) { > > |renderer| is of type RenderBlock so we should tighten the type. On line 125, |renderer| gets reassigned to the frame's ownerRenderer, which is of type RendererPart. Shall I change it to RenderBox, as this is the tightest type that will cover both RenderBlock and RenderPart?
Comment on attachment 180152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180152&action=review >>> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:107 >>> + for (const RenderObject* renderer = baseContainer; renderer; ) { >> >> |renderer| is of type RenderBlock so we should tighten the type. > > On line 125, |renderer| gets reassigned to the frame's ownerRenderer, which is of type RendererPart. Shall I change it to RenderBox, as this is the tightest type that will cover both RenderBlock and RenderPart? Right, I missed that. RenderBox is fine.
Created attachment 185240 [details] Patch
Comment on attachment 180152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180152&action=review >> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:52 >> +static const RenderBlock* containingScrollable(const RenderObject* renderer) > > The name is missing a noun: what is a containing scrollable? > > Better suggestion for the name: enclosingScrollableAncestor, enclosingScrollableContainingBlock enclosingScrollableAncestor sounds good to me, will rename. >> Source/WebKit/chromium/src/FindInPageCoordinates.cpp:56 >> + while (container && !container->hasOverflowClip() && !container->isRenderView()) > > I don't think you need the |container| check unless you expect to be called on a detached tree, in which case calling containingBlock() is really not a good idea. It is possible for this code to be called with RenderView object as the passed in renderer. For example, on line 110, the idea is to chain up the containing scrollable ancestors until we get to the RenderView, stopping when the container returned is NULL. However, by changing the code a little at line 110, I can ensure that this function is never called with renderer = renderView. >> Source/WebKit/chromium/tests/data/find_in_page_frame.html:84 >> +</select> > > You may want to add some tests with positioned elements as it will yield to results which you may find weird: > > <div style="position: relative; overflow: hidden; height: 100px;"> > <div style="overflow: hidden; height: 50px"> > <div style="position: absolute; top: 60px; height: 20px; width: 80px; background-color: green">result 18</div> > </div> > </div> > > (This will yield to a visible result that will be normalized to the most outer div) To clarify, we would expect, ordinarily, that the inner-most div be normalized to the closest scrollable enclosing block, but in the case of position:absolute elements, containingBlock() skips right up to the root element stopping only for position:relative blocks. I think that this is desirable behaviour, since this matches the intended use of position:absolute (to be relative to the page, or to the nearest position:relative block). This patch should also not affect this behaviour. Without the patch, the match is normalized against every containingBlock. With the patch, the match is normalized only against those containingBlocks that are also scrollable, so in both cases the middle div in the example above would be skipped. I've added a test that checks that this is happening as expected, by adding a result in the immediately containing div as well as one inside the position:absolute div. By pushing the immediately containing div down a bit with a margin-top style, I can show that the find in page rect has been normalised against the position:relative rather than the immediately containing div.
Comment on attachment 185240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185240&action=review > Source/WebKit/chromium/ChangeLog:15 > + (WebKit): Let's remove this useless entry and fill in the other ChangeLog function entries. > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:53 > +{ If you assume that this function shouldn't be called on the RenderView, you should add ASSERT(!renderer->isRenderView()) here. > Source/WebKit/chromium/src/FindInPageCoordinates.cpp:-114 > - ASSERT(renderer->isRenderView()); I still think this ASSERT is valuable (even if you are guaranteed not to hit it). It insures that you don't forget any renderer when walking up the tree as jumping to renderer->frame()->ownerRenderer() could skip part of the tree.
Created attachment 185259 [details] Patch
Comment on attachment 185259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185259&action=review If you want your change to be committed, you should ask for the commit-queue flag (cq?). I don't need to see the next iteration (ie you can carry over my r+ to the new patch). > Source/WebKit/chromium/ChangeLog:19 > + * src/FindInPageCoordinates.cpp: > + (WebKit::enclosingScrollableAncestor): > + (WebKit::toNormalizedRect): > + (WebKit::findInPageRectFromAbsoluteRect): > + * tests/WebFrameTest.cpp: > + * tests/data/find.html: > + * tests/data/find_in_page_frame.html: What I meant is that I prefer the ChangeLog to include some low-level details here about the exact change. For example for the first entry above: (WebKit::enclosingScrollableAncestor): Helper function to find the enclosing containing block with an overflow clip.
Created attachment 185454 [details] Patch
Comment on attachment 185454 [details] Patch Clearing flags on attachment: 185454 Committed r141252: <http://trac.webkit.org/changeset/141252>
All reviewed patches have been landed. Closing bug.