Bug 36125 - -[WebFrame setAlwaysHideHorizontal/VerticalScroller:] prevents keyboard scrolling
Summary: -[WebFrame setAlwaysHideHorizontal/VerticalScroller:] prevents keyboard scrol...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: John Sullivan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-03-15 10:55 PDT by John Sullivan
Modified: 2010-03-15 13:11 PDT (History)
0 users

See Also:


Attachments
Patch that still allows keyboard scrolling in WebFrameView after setAlwaysHideHorizontal/VerticalScroller:YES has been called. (19.44 KB, patch)
2010-03-15 11:41 PDT, John Sullivan
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Sullivan 2010-03-15 10:55:42 PDT
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.
Comment 1 John Sullivan 2010-03-15 10:57:56 PDT
This is in Radar as 7726768.
Comment 2 John Sullivan 2010-03-15 11:41:47 PDT
Created attachment 50720 [details]
Patch that still allows keyboard scrolling in WebFrameView after setAlwaysHideHorizontal/VerticalScroller:YES has been called.
Comment 3 Adam Roben (:aroben) 2010-03-15 12:49:57 PDT
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
Comment 4 John Sullivan 2010-03-15 13:07:59 PDT
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.
Comment 5 John Sullivan 2010-03-15 13:11:13 PDT
Committed in 56008.