WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
136273
overflow:scroll elements should not latch to the body if the body is overflow:hidden
https://bugs.webkit.org/show_bug.cgi?id=136273
Summary
overflow:scroll elements should not latch to the body if the body is overflow...
Beth Dakin
Reported
2014-08-26 18:06:33 PDT
overflow:scroll elements should not latch to the body if the body is overflow:hidden. Right now, since we do latch to the body, this prevents you from rubber-banding an overflow area when you are pinned to the edge of the direction you are trying to rubber-band. That behavior feels wrong when the gesture results in absolutely nothing happening, which is the case when the body is overflow:hidden. This affects the web inspector.
Attachments
Patch
(12.47 KB, patch)
2014-08-26 18:12 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Patch
(13.87 KB, patch)
2014-08-27 11:29 PDT
,
Beth Dakin
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2014-08-26 18:12:43 PDT
Created
attachment 237191
[details]
Patch
WebKit Commit Bot
Comment 2
2014-08-26 18:14:05 PDT
Attachment 237191
[details]
did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Beth Dakin
Comment 3
2014-08-27 11:29:04 PDT
Created
attachment 237233
[details]
Patch Should fix Windows and style problems.
Darin Adler
Comment 4
2014-08-27 11:39:10 PDT
Comment on
attachment 237233
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=237233&action=review
> Source/WebCore/page/FrameView.cpp:3268 > +bool FrameView::isScrollable(Scrollability defineScrollable)
I would call this definitionOfScrollable or scrollabilityDefinition or some other noun phrase. “define scrollable” is a verb phrase.
> Source/WebCore/page/FrameView.cpp:3310 > + return parentView->enclosingLayer() ? parentView->enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false;
Could the local variable be the layer rather than the parentView? The point of the local variable is to make the null check possible, and apparently the parent view can’t be null. I prefer && for these rather than ? : false. RenderLayer* layer = parentFrameView()->renderView()->enclosingLayer(); return layer && layer->hasScrollableOrRubberbandableAncestor();
> Source/WebCore/page/FrameView.h:405 > + bool isScrollable(Scrollability defineScrollable = Scrollability::Scrollable);
Should omit the argument name here. Unless the new one makes it really easy to understand.
> Source/WebCore/page/FrameView.h:406 > + virtual bool hasScrollableOrRubberbandableAncestor() override;
Can this be private instead of public?
> Source/WebCore/rendering/RenderLayer.cpp:3068 > +bool RenderLayer::hasScrollableOrRubberbandableAncestor()
Seems messy for this function to special-case RenderView and RenderBox rather than using a virtual function to deal with that.
> Source/WebCore/rendering/RenderLayer.cpp:3078 > + return nextLayer;
Should say return true here. This is just a roundabout way to write it!
> Source/WebCore/rendering/RenderLayer.h:442 > + virtual bool hasScrollableOrRubberbandableAncestor() override;
Does this need to be public?
> Source/WebCore/rendering/RenderListBox.cpp:792 > + return enclosingLayer() ? enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false;
I prefer && instead of ? : false.
Darin Adler
Comment 5
2014-08-27 11:39:37 PDT
Comment on
attachment 237233
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=237233&action=review
>> Source/WebCore/rendering/RenderLayer.h:442 >> + virtual bool hasScrollableOrRubberbandableAncestor() override; > > Does this need to be public?
Oops, meant to remove that comment. Clearly it does.
Beth Dakin
Comment 6
2014-08-27 12:24:34 PDT
(In reply to
comment #4
)
> (From update of
attachment 237233
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=237233&action=review
> > > Source/WebCore/page/FrameView.cpp:3268 > > +bool FrameView::isScrollable(Scrollability defineScrollable) > > I would call this definitionOfScrollable or scrollabilityDefinition or some other noun phrase. “define scrollable” is a verb phrase. >
Changed this to definitionOfScrollable
> > Source/WebCore/page/FrameView.cpp:3310 > > + return parentView->enclosingLayer() ? parentView->enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false; > > Could the local variable be the layer rather than the parentView? The point of the local variable is to make the null check possible, and apparently the parent view can’t be null. >
Actually, I think we do need null checks here! Eek! Added those.
> I prefer && for these rather than ? : false. > > RenderLayer* layer = parentFrameView()->renderView()->enclosingLayer(); > return layer && layer->hasScrollableOrRubberbandableAncestor(); >
I changed this to use &&.
> > Source/WebCore/page/FrameView.h:405 > > + bool isScrollable(Scrollability defineScrollable = Scrollability::Scrollable); > > Should omit the argument name here. Unless the new one makes it really easy to understand. >
I think that the new one (definitionOfScrollable) is helpful, so I left it.
> > Source/WebCore/rendering/RenderLayer.cpp:3068 > > +bool RenderLayer::hasScrollableOrRubberbandableAncestor() > > Seems messy for this function to special-case RenderView and RenderBox rather than using a virtual function to deal with that. >
I added a virtual function on RenderBox, overridden by RenderView.
> > Source/WebCore/rendering/RenderLayer.cpp:3078 > > + return nextLayer; > > Should say return true here. This is just a roundabout way to write it! >
Silly! The code is not here anymore now that I added the virtual function on RenderBox.
> > Source/WebCore/rendering/RenderListBox.cpp:792 > > + return enclosingLayer() ? enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false; > > I prefer && instead of ? : false.
Changed to &&.
Beth Dakin
Comment 7
2014-08-27 12:54:03 PDT
http://trac.webkit.org/changeset/173014
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug