Bug 38418 - [Qt] REGRESSION: fixed elements don't behave correctly when composited (AC)
Summary: [Qt] REGRESSION: fixed elements don't behave correctly when composited (AC)
Status: RESOLVED DUPLICATE of bug 33150
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-05-01 00:49 PDT by Noam Rosenthal
Modified: 2010-06-11 15:33 PDT (History)
3 users (show)

See Also:


Attachments
Fix (3.76 KB, patch)
2010-05-02 03:01 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Re-upload (3.04 KB, patch)
2010-05-02 03:02 PDT, Noam Rosenthal
hausmann: review-
Details | Formatted Diff | Diff
Different approach (3.51 KB, patch)
2010-06-02 15:02 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-05-01 00:49:54 PDT
This regression was probably introduced with some changes to the scrolling mechanism from a few weeks ago.
some of the tests under LayoutTests/compositing/geometry now show red when they shouldn't.
Comment 1 Noam Rosenthal 2010-05-02 03:01:02 PDT
Created attachment 54864 [details]
Fix

This probably needs another set of eyes because it affects non-AC code paths as well.
Comment 2 Noam Rosenthal 2010-05-02 03:02:12 PDT
Created attachment 54865 [details]
Re-upload
Comment 3 Simon Hausmann 2010-05-02 14:14:44 PDT
It looks like this only covers the case for the main frame, but not if the same happens within a child frame. AFAICS in the mac code that's done per frame.

I wonder if the right fix is cross-platform inside WebCore instead of going through the WebKit layer and then back into WebCore?
Comment 4 Noam Rosenthal 2010-05-02 14:25:39 PDT
I think it should be platform specific - there's no way for WebCore to know when the platform actually scrolled.
Comment 5 Simon Hausmann 2010-05-02 14:30:53 PDT
(In reply to comment #4)
> I think it should be platform specific - there's no way for WebCore to know
> when the platform actually scrolled.

Why not?

In your patch you're modifying ChromeClient(Qt)::scroll, which _is_ cross-platform and the place where WebCore tells the platform to scroll.

The other way around, when the upper application layer wants to scroll the WebCore::ScrollView, then it calls functions like scrollBy() inside WebCore.

I must be missing something :)
Comment 6 Noam Rosenthal 2010-05-02 14:40:23 PDT
hmm, I guess you're right. But wouldn't that mess with the Apple implementation?
Comment 7 Noam Rosenthal 2010-05-03 18:09:57 PDT
I researched further: this has to be platform specific, as other platforms might choose to implement scrolling in an async way. scrollPoitionChanged() should be called after the async scroll operation actually took place. In Qt we do sync scrolling, so it happens that we can call scrollPositionChanged() directly - but that behavior is platform specific.
Therefore, I can't think of a differrent/more appropriate way to do this than ChromeClientQt :)
Comment 8 Simon Hausmann 2010-06-02 14:15:19 PDT
(In reply to comment #7)
> I researched further: this has to be platform specific, as other platforms might choose to implement scrolling in an async way. scrollPoitionChanged() should be called after the async scroll operation actually took place. In Qt we do sync scrolling, so it happens that we can call scrollPositionChanged() directly - but that behavior is platform specific.
> Therefore, I can't think of a differrent/more appropriate way to do this than ChromeClientQt :)

You're right.

Ok, what about the main frame vs. child frames point? :)
Comment 9 Noam Rosenthal 2010-06-02 15:02:20 PDT
Created attachment 57707 [details]
Different approach

How about this approach then (see new attachment). It's not for review yet, just discussion.
Comment 10 Simon Hausmann 2010-06-07 04:14:00 PDT
Comment on attachment 57707 [details]
Different approach

I like this approach :)

WebCore/platform/HostWindow.h:60
 +      // Returns true if
The docs should give some more help, so that the other ports have an idea what to do here ;)
Comment 11 Simon Hausmann 2010-06-07 04:14:45 PDT
Comment on attachment 54865 [details]
Re-upload

r- because of lack of frame handling, but the new patch looks promising :)
Comment 12 Noam Rosenthal 2010-06-10 16:53:10 PDT
What's missing for this to handle frames? It's implemented entirely in ScrollView so it should work for anything.
Comment 13 Noam Rosenthal 2010-06-10 16:54:19 PDT
Also, can we add it for the common use case and test frames later? 
I don't get the logic of "Let's reject a bug that fixes FOO because it doesn't fix BAR" :)
No'am
Comment 14 Simon Hausmann 2010-06-10 22:43:55 PDT
I think your third party is good, I like the approach. I r-'ed the second patch, because your third patch replaces it. Please add a changelog to the third one and put it up for review :)
Comment 15 Noam Rosenthal 2010-06-11 11:51:41 PDT
ah, I see. my misunderstanding. New patch coming up :)
Comment 16 Noam Rosenthal 2010-06-11 13:05:50 PDT
This was apparently fixed, see bug 33150.

*** This bug has been marked as a duplicate of bug 33150 ***
Comment 17 Benjamin Poulain 2010-06-11 15:28:05 PDT
(In reply to comment #16)
> This was apparently fixed, see bug 33150.
> 
> *** This bug has been marked as a duplicate of bug 33150 ***

I have not checked the bug but it looks unrelated to 33150.

On scroll, the layers are updated via FrameView::scrollPositionChanged()
See https://bugs.webkit.org/show_bug.cgi?id=36783 && https://bugs.webkit.org/show_bug.cgi?id=38286

https://bugs.webkit.org/show_bug.cgi?id=33150 improves performance but don't really change the behavior.
Comment 18 Noam Rosenthal 2010-06-11 15:33:43 PDT
It could have been 39918 then. The point is the bug is no longer there.