Bug 49177 - [GTK] fast/events/scroll-after-click-on-tab-index has been failing on the bots
Summary: [GTK] fast/events/scroll-after-click-on-tab-index has been failing on the bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-11-08 07:48 PST by Martin Robinson
Modified: 2011-02-22 13:02 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.48 KB, patch)
2011-01-14 15:52 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2011-01-17 02:04 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-11-08 07:48:59 PST
Here's the diff:

--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/fast/events/scroll-after-click-on-tab-index-expected.txt	2010-11-08 07:07:21.665738800 -0800
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/fast/events/scroll-after-click-on-tab-index-actual.txt	2010-11-08 07:07:21.665738800 -0800
@@ -4,7 +4,7 @@
 
 TEST COMPLETE
 Scroll position is more than 0
-PASS document.body.scrollTop > 0 is true
+FAIL document.body.scrollTop > 0 should be true. Was false.
Comment 1 Martin Robinson 2011-01-14 15:39:30 PST
The issue here seems to be that dead scrollbars (ones that have been removed from the main frame) can still control the WebCore's scrollbars after they have been removed. In this case dead scrollbars were stomping on the scrollbar value.
Comment 2 Martin Robinson 2011-01-14 15:52:11 PST
Created attachment 79018 [details]
Patch
Comment 3 Xan Lopez 2011-01-16 18:35:54 PST
Comment on attachment 79018 [details]
Patch

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

r=me

> Source/WebCore/platform/gtk/ScrollViewGtk.cpp:84
> +    // we must to inform main frame scrollbars that their adjustments no longer control the

s/we must to/we must/
Comment 4 Martin Robinson 2011-01-16 22:25:20 PST
Committed r75914: <http://trac.webkit.org/changeset/75914>
Comment 5 Martin Robinson 2011-01-16 22:26:46 PST
Comment on attachment 79018 [details]
Patch

Thanks for the review! Fixed and landed.
Comment 6 Philippe Normand 2011-01-17 00:20:07 PST
This patch made the 3 GTK bots really unhappy. Gonna roll out, sorry!
Comment 7 Philippe Normand 2011-01-17 00:27:44 PST
Reverted r75914 for reason:

multiple crashes on GTK

Committed r75924: <http://trac.webkit.org/changeset/75924>
Comment 8 Martin Robinson 2011-01-17 00:32:56 PST
(In reply to comment #7)
> Reverted r75914 for reason:
> multiple crashes on GTK
> Committed r75924: <http://trac.webkit.org/changeset/75924>

Thanks for the rollout. I'll take another look soon.
Comment 9 Martin Robinson 2011-01-17 01:18:24 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Reverted r75914 for reason:
> > multiple crashes on GTK
> > Committed r75924: <http://trac.webkit.org/changeset/75924>
> Thanks for the rollout. I'll take another look soon.

It seems there are some cases where non-mainframe scrollbars do not have a parent. I should have a new, even-simpler fix for this issue posted soon.
Comment 10 Martin Robinson 2011-01-17 02:04:19 PST
Created attachment 79143 [details]
Patch
Comment 11 Martin Robinson 2011-01-17 02:05:03 PST
I've uploaded a new patch which makes the determination if a scrollbar is defunct lazily. It seems there is no reasonable way to determine when the child is removed whether or not it is a main frame scrollbar.
Comment 12 Eric Seidel (no email) 2011-01-24 15:43:10 PST
I feel like Jamesr recently fixed a parent-less scrollbars issue involving tables on some brazilian photos-of-women-being-stupid site the other day.  I wonder if that's at all related to the case where you're seeing scrollbars w/o parents.
Comment 13 James Robinson 2011-01-24 15:49:19 PST
Adding some scrollbar people.  Offhand, I'm not totally sure what's going on here :)
Comment 14 Peter Kasting 2011-01-24 16:03:23 PST
Sorry, I don't think I can offer any insight :(
Comment 15 Martin Robinson 2011-01-24 16:06:58 PST
(In reply to comment #13)
> Adding some scrollbar people.  Offhand, I'm not totally sure what's going on here :)

Just to clarify there are two issues here:

1. The issue of the bug itself, which comes about because GTK+ mainframe scrollbars are native and the WebCore state is controlled asynchronously via GtkAdjustments.

2. The reason for failure of my first attempt, which is that at certain points in the mainframe scrollbar's lifetime it doesn't have a parent. I'm not entirely sure if this is a bug or not, but my second patch does not have that assumption any longer.
Comment 16 Xan Lopez 2011-02-21 17:59:14 PST
Comment on attachment 79143 [details]
Patch

OK, seems reasonable to me.
Comment 17 Martin Robinson 2011-02-22 13:02:04 PST
Committed r79349: <http://trac.webkit.org/changeset/79349>