Bug 51318 – Make FrameView::calculateScrollbarModesForLayout private again.

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Antonio Gomes]

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.

[Antonio Gomes]

Created attachment 76974 [details]
patch v1

[Yael]

Antonio, Did you test this with an application that explicitly sets the scrollbar mode to off via the WebKit API?

[Antonio Gomes]

(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...

[Antonio Gomes]

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.

[Yael]

(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.

[Antonio Gomes]

> 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 ...

[Antonio Gomes]

(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.