Bug 51318 - Make FrameView::calculateScrollbarModesForLayout private again.
Summary: Make FrameView::calculateScrollbarModesForLayout private again.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-19 21:14 PST by Antonio Gomes
Modified: 2010-12-21 07:12 PST (History)
3 users (show)

See Also:


Attachments
patch v1 (4.45 KB, patch)
2010-12-19 21:20 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-12-19 21:14:37 PST
Method was made public by r72522, however after stuff its code and its code path, I do not think we need have it as public. Instead, we can rely on the already existent and public ScrollView::scrollbarModes method.

Reason: in spatial navigation, before any move we have the following call stack:

Document::updateLayoutIgnorePendingStylesheets ->
Document::updateLayout -> FrameView::layout ->
FrameView::calculateScrollbarModesForLayout ->
ScrollView::setScrolbarModes.

That means we have already the proper scrollbar modes set, and we can just query for it with ScrollView::scrollbarModes when needed.

All tests pass.
Comment 1 Antonio Gomes 2010-12-19 21:20:08 PST
Created attachment 76974 [details]
patch v1
Comment 2 Yael 2010-12-20 06:24:32 PST
Antonio, Did you test this with an application that explicitly sets the scrollbar mode to off via the WebKit API?
Comment 3 Antonio Gomes 2010-12-20 06:26:15 PST
(In reply to comment #2)
> Antonio, Did you test this with an application that explicitly sets the scrollbar mode to off via the WebKit API?

I will make this test before landing. If it turns out it is needed, I have another patch with such a test coming...
Comment 4 Antonio Gomes 2010-12-20 21:41:15 PST
Yael, what is the desired behavior here: Should spatial navigation perform scrolling even we set ScrollBarPolicy::AlwaysOff?

Note: when we set scrollbarpolicy::alwaysoff, qwebpage does not scroll with arrow keys (see handle scroll method). Should we be consistent?

I have a layouttest can sets scrollbar policy (Qt specific, since Qt is the only DRT that supports it). I am wondering if we should scroll or not.
Comment 5 Yael 2010-12-21 05:52:39 PST
(In reply to comment #4)
> Yael, what is the desired behavior here: Should spatial navigation perform scrolling even we set ScrollBarPolicy::AlwaysOff?
> 
> Note: when we set scrollbarpolicy::alwaysoff, qwebpage does not scroll with arrow keys (see handle scroll method). Should we be consistent?
> 
> I have a layouttest can sets scrollbar policy (Qt specific, since Qt is the only DRT that supports it). I am wondering if we should scroll or not.

I believe we should scroll when scrollbars are set to off.
The reason is that both spatial navigation and turning off the scrollbars seem to be more mobile oriented features than desktop features. A modern mobile browser would not want scrollbars on the main frame, but would want to scroll the content of that main frame.
Comment 6 Antonio Gomes 2010-12-21 07:06:47 PST
> I believe we should scroll when scrollbars are set to off.
> The reason is that both spatial navigation and turning off the scrollbars seem to be more mobile oriented features than desktop features. A modern mobile browser would not want scrollbars on the main frame, but would want to scroll the content of that main frame.

Ok, lets change this bug then. Patch with a layout test coming ...
Comment 7 Antonio Gomes 2010-12-21 07:12:18 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Yael, what is the desired behavior here: Should spatial navigation perform scrolling even we set ScrollBarPolicy::AlwaysOff?
> > 
> > Note: when we set scrollbarpolicy::alwaysoff, qwebpage does not scroll with arrow keys (see handle scroll method). Should we be consistent?
> > 
> > I have a layouttest can sets scrollbar policy (Qt specific, since Qt is the only DRT that supports it). I am wondering if we should scroll or not.
> 
> I believe we should scroll when scrollbars are set to off.
> The reason is that both spatial navigation and turning off the scrollbars seem to be more mobile oriented features than desktop features. A modern mobile browser would not want scrollbars on the main frame, but would want to scroll the content of that main frame.

bug 51396.