Issue 731413002: Adjust DOMWindow properties to emulate old style pinch semantics

Issue 731413002: Adjust DOMWindow properties to emulate old style pinch semantics (Closed)

Created:
6 years, 1 month ago by bokan

Modified:
6 years, 1 month ago

Reviewers:
Rick Byers

CC:
blink-reviews

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Adjust DOMWindow properties to emulate old style pinch semantics. This patch prevents virtual viewport from changing the semantics of several of DOMWindow's properties. These include: innerWidth/outerWidth scrollX/scrollY scrollTo scrollBy BUG=433794 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185730

Patch Set 1 #

Patch Set 2 : Added test #

Total comments: 15

Patch Set 3 : Changes from feedback #

Total comments: 2

Patch Set 4 : Check for scroll event handling in RAF callback #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+297 lines, -10 lines)			Patch
M	LayoutTests/SmokeTests	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/TestExpectations	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html	View	1 2 3	1 chunk	+156 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Window/window-scaled-viewport-properties-expected.txt	View	1 2	1 chunk	+59 lines, -0 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/frame/LocalDOMWindow.h	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/core/frame/LocalDOMWindow.cpp	View	1 2	7 chunks	+68 lines, -10 lines	0 comments	Download
M	Source/core/frame/PinchViewport.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/frame/PinchViewport.cpp	View		1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Rick Byers

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right): https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html#newcode2 LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:2: <html> nit: omit html, head and body tags: http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-html-head-body- ...

6 years, 1 month ago (2014-11-18 20:53:31 UTC) #3

bokan

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right): https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html#newcode2 LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:2: <html> On 2014/11/18 20:53:31, Rick Byers wrote: > nit: ...

6 years, 1 month ago (2014-11-18 23:21:51 UTC) #4

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right):

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:2: <html>
On 2014/11/18 20:53:31, Rick Byers wrote:
> nit: omit html, head and body tags:
>
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

Done.

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:119:
setTimeout(runTests, 100);
On 2014/11/18 20:53:31, Rick Byers wrote:
> where does this 100 come from?  You should be able to start running
immediately,
> no?  Otherwise it may be flaky.  If you like you can wait until the load event
> (eg. 'onload = function() { ...').  But please don't add arbitrary timeouts to
> tests.

Yah, I was worried about flakiness but there's some initialization issues
related to onload (crbug.com/343477) so I wanted to run the test after onload.
Anyway, after you mentioned this I realized adding scrollTo(0,0) will force a
layout which is what I needed so Done.

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:121:
</script>
On 2014/11/18 20:53:31, Rick Byers wrote:
> you also fixed a bug with firing missing 'scroll' events, right?  Please test
> that case too.

Right, thanks, added. The only issue is that scroll events are sent async on new
frames (I think) so I added an "(quasi-)arbitrary timeout" since otherwise the
test will wait an eternity before failing and wont give output. Is there a
better way we handle this case? I think I've seen similar patterns in other
layout tests.

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1415: view->scrollBy(scaledOffset,
scrollBehavior);
On 2014/11/18 20:53:31, Rick Byers wrote:
> There are a couple other places in the code that call FrameView::scrollBy. 
> Should any of them be updated to use this logic too to preserve behavior? 
> Perhaps this logic really belongs in FrameView instead of LocalDOMWidow (seems
> similar to FrameView::scrollElementToRect)?

Technically yes, but they're only used from SpacialNavigation and PanScrolling
so I don't think they apply to Android. This patch will need to get merged so I
don't want to touch anything I don't have to. This logic doesn't really belong
in FrameView either and I have bugs to remove it from there as well so I'd
rather leave it here for now. I've added a FIXME here referencing the bug to
remove dependencies on PinchViewport (crbug.com/371896).

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1449:
pinchViewport.move(FloatPoint(excessDelta.width(), excessDelta.height()));
On 2014/11/18 20:53:31, Rick Byers wrote:
> How does this interact with smooth scrolling?  It looks like PinchViewport
> doesn't have a way to honor ScrollBehavior "smooth" if that's what was
> requested.  This is a behavior change compared to the old viewport path,
right?
> 
This is a change, though is ScrollBehavior gone live yet? Nothing on the web
mentions it so I think maybe it's still behind a flag?

> I'm OK treating that as an orthogonal issue, but please file a bug for it and
> add a FIXME referencing that bug here.
Done. crbug.com/434497

https://codereview.chromium.org/731413002/diff/40001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right):

https://codereview.chromium.org/731413002/diff/40001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:21:
window.eventSender.setPageScaleFactorLimits(0.5, 4.0);
I changed the min scale factor to 0.5 so that there's outer viewport scrolling.
I removed the "PinchedOut" test since it's no different from the "PinchedIn" in
the context of what I'm testing (Everything except for min-scaled is "pinched
in").

Rick Byers

Sorry for the delay. LGTM assuming my suggestion for removing the timeout from the test ...

6 years, 1 month ago (2014-11-20 19:23:31 UTC) #6

Sorry for the delay.  LGTM assuming my suggestion for removing the timeout from
the test works.

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right):

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:119:
setTimeout(runTests, 100);
On 2014/11/18 23:21:50, bokan wrote:
> On 2014/11/18 20:53:31, Rick Byers wrote:
> > where does this 100 come from?  You should be able to start running
> immediately,
> > no?  Otherwise it may be flaky.  If you like you can wait until the load
event
> > (eg. 'onload = function() { ...').  But please don't add arbitrary timeouts
to
> > tests.
> 
> Yah, I was worried about flakiness but there's some initialization issues
> related to onload (crbug.com/343477) so I wanted to run the test after onload.
> Anyway, after you mentioned this I realized adding scrollTo(0,0) will force a
> layout which is what I needed so Done.

Great.  Note that there are lots of tests that explicitly provoke a layout (eg.
by reading a property like offsetTop).  It's also fine to use setTimeout(0). 
But any non-zero value is almost always a test bug...

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:121:
</script>
On 2014/11/18 23:21:50, bokan wrote:
> On 2014/11/18 20:53:31, Rick Byers wrote:
> > you also fixed a bug with firing missing 'scroll' events, right?  Please
test
> > that case too.
> 
> Right, thanks, added. The only issue is that scroll events are sent async on
new
> frames (I think) so I added an "(quasi-)arbitrary timeout" since otherwise the
> test will wait an eternity before failing and wont give output. Is there a
> better way we handle this case? I think I've seen similar patterns in other
> layout tests.

I'm a little worried about this being flaky - eg. on Valgrind bots or something.
 scroll events are fired immediately before RAF every frame.  So what about
putting your timeout inside of a RAF callback?  If a RAF callback occurs without
having seen a scroll, then you should be able to fail completely reliably.  That
should also be simpler than what you have :-).

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1415: view->scrollBy(scaledOffset,
scrollBehavior);
On 2014/11/18 23:21:50, bokan wrote:
> On 2014/11/18 20:53:31, Rick Byers wrote:
> > There are a couple other places in the code that call FrameView::scrollBy. 
> > Should any of them be updated to use this logic too to preserve behavior? 
> > Perhaps this logic really belongs in FrameView instead of LocalDOMWidow
(seems
> > similar to FrameView::scrollElementToRect)?
> 
> Technically yes, but they're only used from SpacialNavigation and PanScrolling
> so I don't think they apply to Android. This patch will need to get merged so
I
> don't want to touch anything I don't have to. This logic doesn't really belong
> in FrameView either and I have bugs to remove it from there as well so I'd
> rather leave it here for now. I've added a FIXME here referencing the bug to
> remove dependencies on PinchViewport (crbug.com/371896).

SGTM, thanks.

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/731413002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1449:
pinchViewport.move(FloatPoint(excessDelta.width(), excessDelta.height()));
On 2014/11/18 23:21:50, bokan wrote:
> On 2014/11/18 20:53:31, Rick Byers wrote:
> > How does this interact with smooth scrolling?  It looks like PinchViewport
> > doesn't have a way to honor ScrollBehavior "smooth" if that's what was
> > requested.  This is a behavior change compared to the old viewport path,
> right?
> > 
> This is a change, though is ScrollBehavior gone live yet? Nothing on the web
> mentions it so I think maybe it's still behind a flag?

Oh right, it is.  CSSOMSmoothScroll is still marked experimental.

> > I'm OK treating that as an orthogonal issue, but please file a bug for it
and
> > add a FIXME referencing that bug here.
> Done. crbug.com/434497

Thanks.  I've blocked ajuma's smooth scroll bug on this.

https://codereview.chromium.org/731413002/diff/40001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right):

https://codereview.chromium.org/731413002/diff/40001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:21:
window.eventSender.setPageScaleFactorLimits(0.5, 4.0);
On 2014/11/18 23:21:51, bokan wrote:
> I changed the min scale factor to 0.5 so that there's outer viewport
scrolling.
> I removed the "PinchedOut" test since it's no different from the "PinchedIn"
in
> the context of what I'm testing (Everything except for min-scaled is "pinched
> in").

SGTM

bokan

https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right): https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html#newcode121 LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:121: </script> On 2014/11/20 19:23:30, Rick Byers wrote: > On ...

6 years, 1 month ago (2014-11-20 20:30:41 UTC) #7

Rick Byers

On 2014/11/20 20:30:41, bokan wrote: > https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html > File LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html (right): > > https://codereview.chromium.org/731413002/diff/20001/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html#newcode121 > ...

6 years, 1 month ago (2014-11-20 20:58:12 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/731413002/60001

6 years, 1 month ago (2014-11-20 22:42:12 UTC) #10

commit-bot: I haz the power

6 years, 1 month ago (2014-11-21 00:19:24 UTC) #11

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185730

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages