Bug 63982 - Web Inspector: [REGRESSION r89753-89754] highlight does not respect scroller location.
Summary: Web Inspector: [REGRESSION r89753-89754] highlight does not respect scroller ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-06 02:06 PDT by Pavel Feldman
Modified: 2011-07-11 17:08 PDT (History)
11 users (show)

See Also:


Attachments
[TEST] Test Page (797 bytes, text/html)
2011-07-06 21:52 PDT, Joseph Pecoraro
no flags Details
[PATCH] First Attempt (2.37 KB, patch)
2011-07-07 18:48 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (36.06 KB, patch)
2011-07-11 02:53 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-07-06 02:06:19 PDT
1. Open any page with the vertical scroller
2. Scroll a bit
2. Try highlighting the element

Highlighting is off.
Comment 1 Joseph Pecoraro 2011-07-06 21:51:52 PDT
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.
Comment 2 Joseph Pecoraro 2011-07-06 21:52:11 PDT
Created attachment 99932 [details]
[TEST] Test Page
Comment 3 Pavel Feldman 2011-07-06 23:28:31 PDT
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.
Comment 4 Joseph Pecoraro 2011-07-07 09:50:28 PDT
Sure, rolling out the original change would be reasonable.
Comment 5 Joseph Pecoraro 2011-07-07 10:23:45 PDT
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));
Comment 6 Joseph Pecoraro 2011-07-07 18:48:39 PDT
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.
Comment 7 Simon Fraser (smfr) 2011-07-07 22:09:02 PDT
Would be nice to do some archaeology to see if the non-quad versions got fixed without fixing the quad versions.
Comment 8 Joseph Pecoraro 2011-07-07 22:10:59 PDT
I just added the FloatQuad versions.
Comment 9 Simon Fraser (smfr) 2011-07-07 22:26:31 PDT
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?
Comment 10 Pavel Feldman 2011-07-08 02:35:41 PDT
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.
Comment 11 Simon Fraser (smfr) 2011-07-08 08:20:52 PDT
I'd rather make the change suggested in comment 5 for now.
Comment 12 Joseph Pecoraro 2011-07-08 10:04:17 PDT
(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.
Comment 13 Joseph Pecoraro 2011-07-08 19:19:02 PDT
(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.
Comment 14 Pavel Feldman 2011-07-11 02:53:09 PDT
Created attachment 100259 [details]
Patch
Comment 15 Pavel Feldman 2011-07-11 04:55:20 PDT
Committed r90734: <http://trac.webkit.org/changeset/90734>
Comment 16 Simon Fraser (smfr) 2011-07-11 10:06:35 PDT
Rather than rolling this all out, why not just do the patch in comment 5?
Comment 17 Joseph Pecoraro 2011-07-11 17:00:50 PDT
(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.
Comment 18 Simon Fraser (smfr) 2011-07-11 17:08:20 PDT
(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 :(