Bug 36125 – -[WebFrame setAlwaysHideHorizontal/VerticalScroller:] prevents keyboard scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Sullivan]

The new SPI methods -[WebFrame setAlwaysHideHorizontalScroller:] and -[WebFrame setAlwaysHideVerticalScroller:]  are intended not to prevent scrolling, as is documented by comments in WebFramePrivate.h. However, they currently prevent keyboard scrolling.

[John Sullivan]

This is in Radar as 7726768.

[John Sullivan]

Created attachment 50720 [details]
Patch that still allows keyboard scrolling in WebFrameView after setAlwaysHideHorizontal/VerticalScroller:YES has been called.

[Adam Roben (:aroben)]

Comment on attachment 50720 [details]
Patch that still allows keyboard scrolling in WebFrameView after setAlwaysHideHorizontal/VerticalScroller:YES has been called.

> +        (-[WebFrameView _isScrollable]):
> +        New method, calls -[WebDynamicScrollBarsView horizontalScrollingAllowed] and
> +        -[WebDynamicScrollBarsView verticalScrollingAllowed]
> +        (-[WebFrameView _largestScrollableChild]):
> +        New method, like _largestChildWithScrollBars but uses _isScrollable.
> +        (-[WebFrameView _hasScrollBars]):
> +        Added a comment that this can probably now be deleted. (I'll do so separately.)
> +        (-[WebFrameView _largestChildWithScrollBars]):
> +        Ditto.

If you were to rename these methods in place, it would be a lot easier to see what your changes are. Then you could move them into their sorted location separately.

> @@ -169,11 +179,16 @@ - (void)updateScrollers
>          newHasHorizontalScroller = (hScroll == ScrollbarAlwaysOn);
>      if (vScroll != ScrollbarAuto)
>          newHasVerticalScroller = (vScroll == ScrollbarAlwaysOn);
> -        
> -    newHasHorizontalScroller &= !hideHorizontalScroller;
> -    newHasVerticalScroller &= !hideVerticalScroller;
>      
>      if (!documentView || suppressLayout || suppressScrollers || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
> +        horizontalScrollingAllowedButScrollerHidden = newHasHorizontalScroller && alwaysHideHorizontalScroller;
> +        if (horizontalScrollingAllowedButScrollerHidden)
> +            newHasHorizontalScroller = NO;
> +        
> +        verticalScrollingAllowedButScrollerHidden = newHasVerticalScroller && alwaysHideVerticalScroller;
> +        if (verticalScrollingAllowedButScrollerHidden)
> +            newHasVerticalScroller = NO;
> +        
>          inUpdateScrollers = YES;
>          if (hasHorizontalScroller != newHasHorizontalScroller)
>              [self setHasHorizontalScroller:newHasHorizontalScroller];
> @@ -212,9 +227,14 @@ - (void)updateScrollers
>      if (!newHasVerticalScroller && hasVerticalScroller && hScroll != ScrollbarAlwaysOn)
>          newHasHorizontalScroller = NO;
>  
> -    newHasHorizontalScroller &= !hideHorizontalScroller;
> -    newHasVerticalScroller &= !hideVerticalScroller;
> -
> +    horizontalScrollingAllowedButScrollerHidden = newHasHorizontalScroller && alwaysHideHorizontalScroller;
> +    if (horizontalScrollingAllowedButScrollerHidden)
> +        newHasHorizontalScroller = NO;
> +    
> +    verticalScrollingAllowedButScrollerHidden = newHasVerticalScroller && alwaysHideVerticalScroller;
> +    if (verticalScrollingAllowedButScrollerHidden)
> +        newHasVerticalScroller = NO;
> +        

It's too bad this code is repeated.

r=me

[John Sullivan]

I realized after sending the patch that the no-longer-used methods cannot be immediately deleted because it would break WebKit nightly build compatibility with older versions of Safari. So I reworded the comments in the code and in the ChangeLog to mention that Safari will no longer use them, and that they can probably be deleted once we no longer care about WebKit nightly compatibility with Safari 4.0.x and earlier. Given that, I can't really rename the methods in place, since the new and old methods have to coexist for awhile.

I didn't see a clean way around repeating the little block of code you called out, since it modifies a couple of local variables. Note that it's only executed once for any call of the method though.

[John Sullivan]

Committed in 56008.