Bug 56966 – [GTK] window.innerWidth and window.innerHeight are wrong when using native scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

fast/overflow/horizontal-scroll-after-back.html fails on the bots. It appears that window.innerWidth and window.innerHeight do not include the space allocated for scrollbars, yet they should.

[Martin Robinson]

Created attachment 86710 [details]
Patch

[Martin Robinson]

Committed r81817: <http://trac.webkit.org/changeset/81817>

[Xan Lopez]

Comment on attachment 86710 [details]
Patch

Get out of the queue.

[Martin Robinson]

Comment on attachment 86710 [details]
Patch

Remarking for review. The committed patch just skipped the original failing test. :)

[Martin Robinson]

Comment on attachment 86710 [details]
Patch

r- because my ChangeLog is actually quite inadequate.

[Martin Robinson]

Created attachment 91182 [details]
Patch with better ChangeLog

[Eric Seidel (no email)]

The commit-queue does a last-minute check before landing to make sure the bug is still open, the patch is still not-obsolete, and cq+ is still set.  You never have to worry about racing the cq.   You can land things yourself and it won't complain, you can also mark things cq-, and unless you're exceptionally unlucky you'll always win. :)

[Martin Robinson]

(In reply to comment #7)
> The commit-queue does a last-minute check before landing to make sure the bug is still open, the patch is still not-obsolete, and cq+ is still set.  You never have to worry about racing the cq.   You can land things yourself and it won't complain, you can also mark things cq-, and unless you're exceptionally unlucky you'll always win. :)

Was this comment meant for another bug?

[Eric Seidel (no email)]

(In reply to comment #8)
> (In reply to comment #7)
> > The commit-queue does a last-minute check before landing to make sure the bug is still open, the patch is still not-obsolete, and cq+ is still set.  You never have to worry about racing the cq.   You can land things yourself and it won't complain, you can also mark things cq-, and unless you're exceptionally unlucky you'll always win. :)
> 
> Was this comment meant for another bug?

Nope.  Was replying mostly to comment #3. :)

[Martin Robinson]

(In reply to comment #9)

> Was this comment meant for another bug?

Oh, I see. Here is the sequence of events in this case:

1. I opened this bug about a failing test.
2. I landed a patch with --no-close, referencing this bug. The patch skipped the test.
3. I uploaded a patch containing a real fix to this same bug.
4. Xan saw comment #2 and assumed the first patch fixed the bug. He removed the r? from the patch.

I talked about this with Daniel Bates the other day and he said his tactic is to land the first (skiplist) patch and them mark the bug as REOPENED. Perhaps that would make things less confusing in the future.

[Eric Seidel (no email)]

Comment on attachment 91182 [details]
Patch with better ChangeLog

Do other ports do similarly?

[Martin Robinson]

(In reply to comment #11)
> (From update of attachment 91182 [details])
> Do other ports do similarly?

GTK+ is unique in itss ScrollView implementation because main frame scrollbars are drawn and owned by the containing GtkScrolledWindow. Thus, GTK+'s ScrollView must provide custom implementations of some ScrollView methods. At the same time GTK+ ScrollViews do not have a native widget backing them. At some point I'd like to increase code sharing, but it seems orthogonal to this patch.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 91182 [details] [details])
> > Do other ports do similarly?
> 
> GTK+ is unique in itss ScrollView implementation because main frame scrollbars are drawn and owned by the containing GtkScrolledWindow.

Really?  Doesn't the Mac port use native scrollbars too?  Maybe those are drawn by the WebView though instead of an NSScrollView.

> Thus, GTK+'s ScrollView must provide custom implementations of some ScrollView methods. At the same time GTK+ ScrollViews do not have a native widget backing them.

I'm not sure I understand but I believe you this stuff is complicated. :)

> At some point I'd like to increase code sharing, but it seems orthogonal to this patch.

Yeah, I just worry that we're moving further from that, rather than closer.

[Martin Robinson]

This test is passing now that we've switched to using WebCore scrollbars in the DRT.

[Martin Robinson]

(In reply to comment #14)
> This test is passing now that we've switched to using WebCore scrollbars in the DRT.

I should mention that window.innerWidth and window.innerHeight are still incorrect when using native GTK+ main frame scrollbars.

[Martin Robinson]

WebKit2 doesn't use native scrollbars.