Bug 63982 – Web Inspector: [REGRESSION r89753-89754] highlight does not respect scroller location.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

1. Open any page with the vertical scroller
2. Scroll a bit
2. Try highlighting the element

Highlighting is off.

[Joseph Pecoraro]

Couldn't get to the tonight (updating + building took longer then expected!). The highlight rect seems to scroll twice as much as the content. So if you start scrolling down and a <div> moves up, the highlight rect for that <div> moves up even further. I'll attach my test case from before with some vertical scrolling. I'll also test real sites.

[Joseph Pecoraro]

Created attachment 99932 [details]
[TEST] Test Page

[Pavel Feldman]

What exactly was original patch fixing? Does it make sense to roll it out before this regression is fixed? This one breaks on a very general case.

[Joseph Pecoraro]

Sure, rolling out the original change would be reasonable.

[Joseph Pecoraro]

Switching back to "localToAbsolute" looks like it fixed the issue. I'll investigate more tonight, I'm out most of the day. let me know if you rollout the previous change.


        // FloatQuad absContentQuad = containingView->convertFromRenderer(renderer, FloatRect(contentBox));
        // FloatQuad absPaddingQuad = containingView->convertFromRenderer(renderer, FloatRect(paddingBox));
        // FloatQuad absBorderQuad = containingView->convertFromRenderer(renderer, FloatRect(borderBox));
        // FloatQuad absMarginQuad = containingView->convertFromRenderer(renderer, FloatRect(marginBox));

        FloatQuad absContentQuad = renderBox->localToAbsoluteQuad(FloatRect(contentBox));
        FloatQuad absPaddingQuad = renderBox->localToAbsoluteQuad(FloatRect(paddingBox));
        FloatQuad absBorderQuad = renderBox->localToAbsoluteQuad(FloatRect(borderBox));
        FloatQuad absMarginQuad = renderBox->localToAbsoluteQuad(FloatRect(marginBox));

[Joseph Pecoraro]

Created attachment 100061 [details]
[PATCH] First Attempt

This fixes all the issues I encountered with highlight rects being wrong when
I scrolled in the main frame. I tested scrolling in the main frame, subframes,
and transformed versions of nested frames etc.

I'm a bit frustrated now that the FloatQuad version of FrameView::convertFromRenderer
is so different from the IntRect version. I wonder why these two differences (root view
and fixed content) aren't handled in the IntRect version and if maybe my understanding
of what this function really does is wrong and it should be renamed. I'd like to look deeper
at this when I have some more time, but this appears to fix the regression.

[Simon Fraser (smfr)]

Would be nice to do some archaeology to see if the non-quad versions got fixed without fixing the quad versions.

[Joseph Pecoraro]

I just added the FloatQuad versions.

[Simon Fraser (smfr)]

Comment on attachment 100061 [details]
[PATCH] First Attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=100061&action=review

> Source/WebCore/page/FrameView.cpp:2823
> -    if (!wasFixed) {
> +    if (parent() && !wasFixed) {
>          IntPoint scroll = scrollPosition();
>          quad.move(-scroll.x(), -scroll.y());

I'd like to understand why this method has the !wasFixed check, and now needs the parent() check, when the IntRect version does not. Is the IntRect version broken?

[Pavel Feldman]

Comment on attachment 100061 [details]
[PATCH] First Attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=100061&action=review

>> Source/WebCore/page/FrameView.cpp:2823
>>          quad.move(-scroll.x(), -scroll.y());
> 
> I'd like to understand why this method has the !wasFixed check, and now needs the parent() check, when the IntRect version does not. Is the IntRect version broken?

Can we land this fix now and follow up on the IntRect? It renders inspector unusable.

[Simon Fraser (smfr)]

I'd rather make the change suggested in comment 5 for now.

[Joseph Pecoraro]

(In reply to comment #11)
> I'd rather make the change suggested in comment 5 for now.

That actually didn't fix the problem when inspecting an element in subframes.
I could probably do something like:

    if (containingView->parent()) {
        // Not the root container? Use convertFromRenderer.
    } else {
        // Root container, use localToAbsoluteQuad.
    }

I can try that.

[Joseph Pecoraro]

(In reply to comment #12)
> (In reply to comment #11)
> > I'd rather make the change suggested in comment 5 for now.
> 
> That actually didn't fix the problem when inspecting an element in subframes.
> I could probably do something like:
> 
>     if (containingView->parent()) {
>         // Not the root container? Use convertFromRenderer.
>     } else {
>         // Root container, use localToAbsoluteQuad.
>     }
> 
> I can try that.

Nope, this didn't work for subframes, because eventually in the recursive
convertToRoot when reaching the root is reached and the scroll position
is doubly added there. My next step will be looking at the IntRect version
and seeing if that is wrong for these cases.

[Pavel Feldman]

Created attachment 100259 [details]
Patch

[Pavel Feldman]

Committed r90734: <http://trac.webkit.org/changeset/90734>

[Simon Fraser (smfr)]

Rather than rolling this all out, why not just do the patch in comment 5?

[Joseph Pecoraro]

(In reply to comment #16)
> Rather than rolling this all out, why not just do the patch in comment 5?

In comment 13 I mentioned that was not sufficient, subframes were still broken.


> My next step will be looking at the IntRect version and seeing if that is wrong
> for these cases.

I haven't actually been able to trigger a breakpoint on these functions. Looking
at the code it looks like this should run in code related to Scrollbars, but testing
a bunch of custom and non-custom scrollbars I never his this. Blaming the code
to see when it was added I came across the following test:

    LayoutTests/platform/mac/fast/forms/listbox-scrollbar-hit-test.html

The test seems to succeed in DRT but the scrollbars don't work properly in Safari.
Scrollwheel events seem to work, but clicking on the scrollbar, using up/down etc
all don't work.

[Simon Fraser (smfr)]

(In reply to comment #17)

> The test seems to succeed in DRT but the scrollbars don't work properly in Safari.
> Scrollwheel events seem to work, but clicking on the scrollbar, using up/down etc
> all don't work.

There may be WK1/WK2 differences here, and we don't run any eventSender tests in WK2 :(