Closed Bug 1516056 Opened 5 years ago Closed 5 years ago

Restrict the layout scroll offset to the layout scroll range

Categories

(Core :: Panning and Zooming, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(10 files, 1 obsolete file)

830 bytes, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Attached file Testcase
STR
  1. Load the attached testcase in Fennec
  2. Observe that the reported values for window.scrollMaxX/Y
     are (0,0). That is, this is a page where the layout
     scroll position should never change.
  3. Zoom in and pan such that the light blue div is out of view.
  4. Click the "Scroll Into View" button.

Actual results:

  The reported values for window.scrollX/Y (which are displayed
  inside the light blue div) are something larger than (0,0).

Expected results:

  The values of window.scrollX/Y respect the maximums reported
  in window.scrollMaxX/Y.

This is a regression from bug 1423011.
The problem here is that GetScrollRangeForClamping() is returning the visual scroll range even when we are clamping the layout scroll position.
I have some patches that fix this, but they're failing mobile/android/tests/browser/chrome/test_session_scroll_position.html:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=a8ee2531479d747fe6901815af8b26981179f914
I guess this might also fix the problems I've seen with http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html,which I was going to file a bug about.

As for the test failure, I think it could be genuine. The problems the visual viewport has caused with session history and the session store have so far been masked because the tests have been changing the scroll position by calling scrollTo(), which so far has forced the layout viewport to the expected visual viewport scroll position and therefore also triggered the expected layout scroll events, unlike what happens when a a real user has zoomed in and is now drag-scrolling the page around.
The failure happens in the sessionStoreScrollPositionForFrames subtest, which, if I remember correctly, by coincidence is the only test using a page (the frameset itself) that is only scrollable when you zoom in, i.e. by all rights the layout viewport should never scroll.
Because of this bug, it scrolls anyway when using scrollTo(), but once you fix that, you run into the problem that the session store is still targeting the layout viewport even though it should really be using the visual viewport instead.

So I'm afraid you'll have to wait until I've finished with fixing that, which I'll resume working on shortly, now that the visual viewport events have finally landed.
Thanks for the analysis!

Is the work you mention the one tracked in bug 1498812? Does it have a dependency on bug 1507279?
The former and its related bugs. Given that for now and in the near future we won't be recording visual and layout viewport separately, I don't think I need something to set them independently, either, i.e. I think scrollTo() should be good enough. If my assessment of that changes, I'll let you know.
See Also: → 1516722
I've tried a build with your patch, but by itself I don't think it's doing the correct thing, either. Because you're now simply clamping the scroll position according to the layout viewport, this means that - having pinch-zoomed in - scrollTo() and friends now cannot scroll to a scroll position where the relative offset between layout and visual viewport isn't "0,0" (i.e. basically what bug 1516722 seems to be about).

This isn't the expected behaviour according to http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html, where after pinch-zooming in and then scrolling all the way to the bottom right, calling scrollTo() with some very large coordinates should change neither layout nor visual viewports, i.e. their positions should be both clamped independently, and it'll also break session history/the session store once again after I've just managed to unbreak them again in bug 1498812.
From a user perspective, a wrong layout viewport offset is clearly the lesser evil as compared to having the wrong visual viewport offset.

Hence I strongly object to landing your patch in its current from, and besides, even with my patches for bug 1498812 applied this will still fail the session store test because the scroll position on the frame set cannot be restored if it gets clamped according to the layout viewport's requirements.
(In reply to Jan Henning [:JanH] from comment #7)
> I've tried a build with your patch, but by itself I don't think it's doing
> the correct thing, either. Because you're now simply clamping the scroll
> position according to the layout viewport, this means that - having
> pinch-zoomed in - scrollTo() and friends now cannot scroll to a scroll
> position where the relative offset between layout and visual viewport isn't
> "0,0" (i.e. basically what bug 1516722 seems to be about).

This is mostly intentional. As far as web content is concerned, the usual APIs for querying the scroll position (e.g. scrollTop) and modifying it (e.g. scrollTo()) all apply to the layout scroll position. The visual scroll position is an implementation detail that web content is largely oblivious to, except that it can observe it using the new VisualViewport API (not change it, though).

Now, changing the layout scroll position can result in the visual scroll position also changing, mostly to preserve the constraint that the visual viewport remains inside the layout viewport. However, again, this is an implementation detail, and web content should not rely on the visual scroll position changing in a particular way.

The story is different for browser internals: unlike web content, we do want to allow browser internals to change the visual scroll position in specific ways. However, they cannot do that using APIs that web content can also use, like scrollTo(), since those APIs do not have that power. Rather, browser internals that need to set the visual scroll position need to do so using an internal-only API, such as the one I am proposing to add in bug 1507279.

> This isn't the expected behaviour according to
> http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html, where
> after pinch-zooming in and then scrolling all the way to the bottom right,
> calling scrollTo() with some very large coordinates should change neither
> layout nor visual viewports, i.e. their positions should be both clamped
> independently,

I haven't debugged the test case yet, but this seems achievable.

> and it'll also break session history/the session store once
> again after I've just managed to unbreak them again in bug 1498812.
> From a user perspective, a wrong layout viewport offset is clearly the
> lesser evil as compared to having the wrong visual viewport offset.
> 
> Hence I strongly object to landing your patch in its current from, and
> besides, even with my patches for bug 1498812 applied this will still fail
> the session store test because the scroll position on the frame set cannot
> be restored if it gets clamped according to the layout viewport's
> requirements.

Well, this is why I imagined that bug 1498812 would have a dependency on bug 1507279.

The fix for bug 1498812 should not assume that scrollTo() can be used to control the visual viewport offset, for the reasons explained above. The API to be added in bug 1507279 should be used to control the visual viewport offset.
Okay, now I understand better what you're on about.

(In reply to Botond Ballo [:botond] from comment #8)
> > This isn't the expected behaviour according to
> > http://w3c-test.org/visual-viewport/viewport-scroll-event-manual.html, where
> > after pinch-zooming in and then scrolling all the way to the bottom right,
> > calling scrollTo() with some very large coordinates should change neither
> > layout nor visual viewports, i.e. their positions should be both clamped
> > independently,
> 
> I haven't debugged the test case yet, but this seems achievable.

Hm yes, I guess the test's expected behaviour could also be achieved by not moving the visual viewport here as long as the bigger viewport is still enclosing the smaller viewport (i.e. https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/gfx/layers/FrameMetrics.cpp#23).

> The fix for bug 1498812 should not assume that scrollTo() can be used to
> control the visual viewport offset, for the reasons explained above. The API
> to be added in bug 1507279 should be used to control the visual viewport
> offset.

Bug 1507279 isn't available though, whereas I'm ready to go now, and I have a suspicion that properly implementing separate APIs for setting both viewports could turn out a little more complicated as well. Hence I'll take whatever works at the moment, which right now means scrollTo() - it was bad enough that the visual viewport implementation was allowed to cause the regressions it did. So I hope you forgive me if my priority is getting back into a sane state regarding the *user-visible* scroll position as fast as possible. We can always properly clean up the rest of viewport handling afterwards, as long as the current major regressions have been fixed again and we're more careful to not cause any new regressions.
Let's consider a concrete example. Suppose you have a page with content size 400x400 (all measurements in CSS pixels) and layout viewport size 200x200, and you're zoomed in such that the visual viewport size is 100x100.

Under these conditions the layout scroll range is (0,0)-(200,200), and the visual scroll range is (0,0)-(300,300).

IIUC the question is what should window.scrollTo(300,300) do on such a page.

Currently, the Firefox implementation sets the layout scroll offset to (300,300), because it's doing something nonsensical - using the visual scroll range to bound the layout scroll offset (i.e., this bug). As a side effect of the layout scroll offset being set to (300,300), the visual scroll offset is also set to (300,300).

The patches in this bug change things such that the layout scroll offset is correctly bounded by the layout scroll range, so the behaviour with the patches would be that the layout scroll offset is set to (200,200). (And again, as a side effect, the visual scroll offset will also become (200,200).)

It sounds like the behaviour you'd like is for scrollTo(300,300) to set the layout scroll offset to (200,200) and the visual scroll offset to (300,300)?

I don't believe that's the intended behaviour for scrollTo(). I tested the behaviour with Chrome, using this page [1], and the behaviour I see is that Chrome:

  - Uses the layout scroll range to bound the layout scroll offset

  - Does not do additional visual scrolling, beyond scrolling to
    the new layout scroll offset

(The behaviour is still different from Firefox's with these patches, because Chrome sizes the layout viewport differently than Firefox (bug 1423013). On this test page, it expands the layout viewport to be almost the whole page, and as a result the page has almost no layout scroll range, and scrollTo() barely scrolls at all. However, the observations above hold.)

Given that, my preference would be to avoid introducing additional places where we rely on the current unintended behaviour of scrollTo() (but see below).

[1] https://theres-waldo.github.io/mozilla-testcases/viewport/scrollto.html

(In reply to Jan Henning [:JanH] from comment #9)
> Bug 1507279 isn't available though, whereas I'm ready to go now, and I have
> a suspicion that properly implementing separate APIs for setting both
> viewports could turn out a little more complicated as well. Hence I'll take
> whatever works at the moment, which right now means scrollTo() - it was bad
> enough that the visual viewport implementation was allowed to cause the
> regressions it did. So I hope you forgive me if my priority is getting back
> into a sane state regarding the *user-visible* scroll position as fast as
> possible. We can always properly clean up the rest of viewport handling
> afterwards, as long as the current major regressions have been fixed again
> and we're more careful to not cause any new regressions.

This is a fair point, and I certainly don't want to hold back a fix for bug 1498812.

Are you looking to possibly uplift bug 1498812 to 65, or are you targeting 66?

In the latter case, I'd like to at least try to fix bug 1507279 first, and rebase bug 1498812 on top of it.

To be clear, here what I imagine that would involve:

  - Bug 1507279 adds a new API, nsIDOMWindowUtils.scrollToVisual(point),
    which behaves the way you'd like window.scrollTo() to behave. That
    is, on the page mentioned above, scrollToVisual(300,300) will scroll
    the layout viewport to (200,200) and the visual viewport to (300,300).

  - The bug 1498812 patches are modified to use scrollToVisual() instead
    of scrollTo() whenever this behaviour is desired.

Does that sound reasonable to you?
(And of course, this bug wouldn't land until the other two are sorted out.)
Depends on: 1507279, 1498812
> It sounds like the behaviour you'd like is for scrollTo(300,300) to set the layout scroll offset to (200,200) and the visual scroll offset to (300,300)?

Not generally, of course. But the way I understand that particular web platform test, if through manual drag scrolling both viewports have been scrolled as far right and down as possible (200,200 for the layout viewport and 300,300 for the visual viewport), then a call to scrollTo(10000, 10000) shouldn't change the position of either viewport [1] - the layout viewport naturally remains unchanged because it is already scrolled as far as is possible, and as for the visual viewport...
- either the logic is that if the layout viewport effectively didn't move in result to a scrollTo() call, then the visual viewport shouldn't move either
- or that after a call the scrollTo(), the visual viewport position is adjusted only if the condition that the smaller viewport be bounded by the larger viewport no longer holds, in which case there's equally no reason to move the visual viewport
- or ??? leading to the same result

The only thing that's left open is what happens after a subsequent call to scrollTo(0,0) [2]: Maybe the relative offset remains unchanged (so the visual viewport ends up at 100,100), or maybe the visual viewport is scrolled to 0,0 as well, or in theory anything inbetween.

> - The bug 1498812 patches are modified to use scrollToVisual() instead
>   of scrollTo() whenever this behaviour is desired.

I don't know how much effort that would be, though - it might be straightforward (for the session store it probably will be), but (especially for the session history logic in nsGfxScrollFrame) it might also throw up some additional complications.
On top of that, while I had a little more time to work on all this over the holidays, now I'm returning back to work as well and as such any additional complications would take longer to resolve from my side.
And yes, while it's getting somewhat late in the cycle, I'd still like have a chance of uplifting bug 1498812 if possible.

So you're desire to not further rely on the current behaviour of scrollTo() notwithstanding, I'd still prefer to land bug 1498812 ASAP in its current state and then sort out this and bug 1507279 without the immediate pressure that a missed deadline means yet another Firefox version with borky scroll position restoring.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/testing/web-platform/tests/visual-viewport/viewport-scroll-event-manual.html#121-135
[2] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/testing/web-platform/tests/visual-viewport/viewport-scroll-event-manual.html#153-163
(In reply to Jan Henning [:JanH] from comment #12)
> > It sounds like the behaviour you'd like is for scrollTo(300,300) to set the layout scroll offset to (200,200) and the visual scroll offset to (300,300)?
> 
> Not generally, of course. But the way I understand that particular web
> platform test, if through manual drag scrolling both viewports have been
> scrolled as far right and down as possible (200,200 for the layout viewport
> and 300,300 for the visual viewport), then a call to scrollTo(10000, 10000)
> shouldn't change the position of either viewport [1] - the layout viewport
> naturally remains unchanged because it is already scrolled as far as is
> possible, and as for the visual viewport...
> - either the logic is that if the layout viewport effectively didn't move in
> result to a scrollTo() call, then the visual viewport shouldn't move either
> - or that after a call the scrollTo(), the visual viewport position is
> adjusted only if the condition that the smaller viewport be bounded by the
> larger viewport no longer holds, in which case there's equally no reason to
> move the visual viewport
> - or ??? leading to the same result

But is that sufficient for the session store?

If you're saving a visual scroll offset of (300,300) on such a page, and later restoring it, do you not need an API that will take you from an initial state of both offsets being (0,0), to a state where the layout offset is (200,200) and the visual offset is (300,300)?

> > - The bug 1498812 patches are modified to use scrollToVisual() instead
> >   of scrollTo() whenever this behaviour is desired.
> 
> I don't know how much effort that would be, though - it might be
> straightforward (for the session store it probably will be), but (especially
> for the session history logic in nsGfxScrollFrame) it might also throw up
> some additional complications.
> On top of that, while I had a little more time to work on all this over the
> holidays, now I'm returning back to work as well and as such any additional
> complications would take longer to resolve from my side.
> And yes, while it's getting somewhat late in the cycle, I'd still like have
> a chance of uplifting bug 1498812 if possible.
> 
> So you're desire to not further rely on the current behaviour of scrollTo()
> notwithstanding, I'd still prefer to land bug 1498812 ASAP in its current
> state and then sort out this and bug 1507279 without the immediate pressure
> that a missed deadline means yet another Firefox version with borky scroll
> position restoring.

Ok, fair enough, we can do that.
Depends on: 1517895
No longer depends on: 1498812, 1507279
Summary: scrollIntoView() can cause window.scrollX/Y to exceed window.scrollMaxX/Y → Restrict the layout scroll offset to the layout scroll range
(In reply to Botond Ballo [:botond] from comment #13)
> But is that sufficient for the session store?
> 
> If you're saving a visual scroll offset of (300,300) on such a page, and
> later restoring it, do you not need an API that will take you from an
> initial state of both offsets being (0,0), to a state where the layout
> offset is (200,200) and the visual offset is (300,300)?

Oh, I see where there might be some confusion. Everything I've mentioned above in relation to that web platform test was intended only towards the future behaviour of scrollTo(), but *not* about the bug 1507279 API. So yes, for the session store I do indeed want the new replacement API to attempt to scroll both viewports as closely as possible to the requested coordinates.

I was mentioning that web platform test mainly because while your initial patch for this bug does indeed solve the problem of the layout viewport not exceeding scrollMaxX/Y, it still didn't improve our score on that test. Therefore I thought that you might be able to kill two birds with one stone and, once the new API is in place and the session store/history have been switched over, you might want to change the scrollTo behaviour such that it not only fixes the original issue from this bug, but also makes us pass that test as well.
Another thing I'm wondering - what will scrollTo() do if the visual viewport is larger than the layout viewport?
As long as the layout viewport is >= the visual viewport, simply clamping the scroll position according to the layout viewport and then scrolling *both* viewports to the resulting coordinates will be fine.

However when the visual viewport is larger than the layout viewport, that would result in the opposite problem as today, wouldn't it? In that case all of a sudden the visual viewport would be scrolled to an impossible position.

But properly fixing that (i.e. if calling scrollTo() is no longer guaranteed to scroll both viewports to the same position) might also have further implications for things like https://phabricator.services.mozilla.com/D15691#inline-85153 - unless we make it so that scrollTo() is simply clamped according to the larger of both viewports [1], but I don't know if that makes sense in practice or causes yet other problems elsewhere.

(In reply to Botond Ballo [:botond] from comment #10)
> I don't believe that's the intended behaviour for scrollTo(). I tested the
> behaviour with Chrome, using this page [1], and the behaviour I see is that
> Chrome:
> 
>   - Uses the layout scroll range to bound the layout scroll offset
> 
>   - Does not do additional visual scrolling, beyond scrolling to
>     the new layout scroll offset

As for Chrome, regarding "Does not do additional visual scrolling", I haven't had an opportunity to actually try that, but by that do you mean that the current *relative* visual viewport offset is left unchanged if possible [1]? I guess that would fit with the way they've specced the visual viewports scroll events as firing only whenever the relative offset changes.

[1] A bit like we do for fixed position elements, i.e. we always attach them to the larger of both viewports if I remember correctly, right?
[2] And as long as the layout viewport is the larger one, there is indeed never a reason to change the *relative* visual viewport offset in response to the layout viewport scrolling.
(In reply to Jan Henning [:JanH] from comment #14)
> I was mentioning that web platform test mainly because while your initial
> patch for this bug does indeed solve the problem of the layout viewport not
> exceeding scrollMaxX/Y, it still didn't improve our score on that test.
> Therefore I thought that you might be able to kill two birds with one stone
> and, once the new API is in place and the session store/history have been
> switched over, you might want to change the scrollTo behaviour such that it
> not only fixes the original issue from this bug, but also makes us pass that
> test as well.

Thanks - I've taken note of the web platform test, and will be sure to get it passing. Not sure if yet if it will be in this bug or a follow-up.

(In reply to Jan Henning [:JanH] from comment #15)
> Another thing I'm wondering - what will scrollTo() do if the visual viewport
> is larger than the layout viewport?
> As long as the layout viewport is >= the visual viewport, simply clamping
> the scroll position according to the layout viewport and then scrolling
> *both* viewports to the resulting coordinates will be fine.
> 
> However when the visual viewport is larger than the layout viewport, that
> would result in the opposite problem as today, wouldn't it? In that case all
> of a sudden the visual viewport would be scrolled to an impossible position.
> 
> But properly fixing that (i.e. if calling scrollTo() is no longer guaranteed
> to scroll both viewports to the same position) might also have further
> implications for things like
> https://phabricator.services.mozilla.com/D15691#inline-85153 - unless we
> make it so that scrollTo() is simply clamped according to the larger of both
> viewports [1], but I don't know if that makes sense in practice or causes
> yet other problems elsewhere.

The possibility of the visual viewport being larger than the layout viewport will go away very soon, in bug 1423013, where we will start expanding the layout viewport, the way Chrome does, to the minimum-scale size, which is also the largest the visual viewport can be.

> (In reply to Botond Ballo [:botond] from comment #10)
> > I don't believe that's the intended behaviour for scrollTo(). I tested the
> > behaviour with Chrome, using this page [1], and the behaviour I see is that
> > Chrome:
> > 
> >   - Uses the layout scroll range to bound the layout scroll offset
> > 
> >   - Does not do additional visual scrolling, beyond scrolling to
> >     the new layout scroll offset
> 
> As for Chrome, regarding "Does not do additional visual scrolling", I
> haven't had an opportunity to actually try that, but by that do you mean
> that the current *relative* visual viewport offset is left unchanged if
> possible [1]? I guess that would fit with the way they've specced the visual
> viewports scroll events as firing only whenever the relative offset changes.

That's not what I had in mind (I had in mind that the visual scroll offset would become the same as the layout scroll offset), but the test case doesn't actually discriminate the two possibilities because the relative offset starts as (0,0). I will adjust the testcase and report back.

(In reply to Botond Ballo [:botond] from comment #16)

As for Chrome, regarding "Does not do additional visual scrolling", I
haven't had an opportunity to actually try that, but by that do you mean
that the current relative visual viewport offset is left unchanged if
possible [1]? I guess that would fit with the way they've specced the visual
viewports scroll events as firing only whenever the relative offset changes.

That's not what I had in mind (I had in mind that the visual scroll offset
would become the same as the layout scroll offset), but the test case
doesn't actually discriminate the two possibilities because the relative
offset starts as (0,0). I will adjust the testcase and report back.

Ok, I adjusted the testcase to test the Chrome behaviour better.

The STR is now:

  • Pinzh-zoom in such that the visual viewport corresponds roughly
    to the blue rect.

  • Scroll down until the blue rect is just out of view, and the
    "scrollTo()" button is near the top left corner of the screen.
    At this point, the relative offset between the two viewports
    is approximately (0, screenHeight).

  • Click the scrollTo() button. This performs approximately
    scrollTo(screenWidth, 0).

If this takes you visually to (screenWidth, screenHeight), the relative offset is preserved. If it takes you to (screenWidth, 0), it is not.

My testing is showing that it is indeed taking you to (screenWidth, screenHeight), so Chrome is preserving the relative offset.

For us, the way to do that would be to have scrollTo() use the mechanism from bug 1453425. scrollBy() already has this behaviour, and I believe we would like to eventually have it for scrollTo() as well (certainly, the Chrome behaviour tested here is an argument in favour of it).

Blocks: 1519013

This is blocking bug 1519013, which has been split off from / is conceptually a part of bug 1423013, which we'd like to fix ASAP.

Priority: P3 → P2
Depends on: 1519621

Updated patch series on top of bug 1517895:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=bc373653cba565ebe5dd52c457a98770484a9ef3

There is one test failure on Android, testing/marionette/harness/marionette_harness/tests/unit/test_typing.py, which I can't reproduce locally.

(In reply to Botond Ballo [:botond] [standards meeting Feb 18-23] from comment #20)

There is one test failure on Android, testing/marionette/harness/marionette_harness/tests/unit/test_typing.py, which I can't reproduce locally.

After rebasing, the newly added GeckoView PanZoomControllerTest is failing as well.

PanZoomControllerTest are probably failing because the new GeckoView APIs use window.scrollBy() and window.scrollTo() as well as the visual viewport to scroll by a factor of the screen size:

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/mobile/android/chrome/geckoview/GeckoViewContentChild.js#287

If the behavior of the visible viewport or virtual viewport has changed that's probably why.

Depends on D20281

This avoids the testcase running into bug 1516722.

Depends on D20284

Blocks: 1531057
Assignee: nobody → botond

Adding some dependencies to make sure this doesn't regress Fire TV.

This is unlikely to land in the 67 cycle, but I will see if I can come up with a more targeted fix that addresses bug 1519007 / bug 1531057 and can be uplifted to 67.

No longer blocks: 1531057
No longer blocks: 1519013
No longer blocks: 1538762
Attachment #9044839 - Attachment is obsolete: true

Rebased and updated to account for bug 1531531 and bug 1538762.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16b36c86155d
Rename ScrollFrameHelper::GetScrollRange() to GetLayoutScrollRange(). r=kats
https://hg.mozilla.org/integration/autoland/rev/3b7f8dede084
Rename ScrollFrameHelper::GetScrollRangeForClamping() to GetVisualScrollRange(). r=kats
https://hg.mozilla.org/integration/autoland/rev/a84c86ad7031
Call GetLayoutScrollRange() instead of GetVisualScrollRange() when we are using the scroll range to clamp the layout scroll position. r=kats
https://hg.mozilla.org/integration/autoland/rev/be6e27fbdb2c
Add a mochitest. r=kats
https://hg.mozilla.org/integration/autoland/rev/946181497fb1
Adjust ScrollToRestorePosition() to reflect that the layout scroll offset is clamped to the layout viewport. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/62f15d6bd366
Avoid calling clamped() with impossible bounds in AsyncPanZoomController::ZoomToRect(). r=kats
https://hg.mozilla.org/integration/autoland/rev/09351cbff684
Prevent zooming in marionette test_typing.py testcase. r=hiro
https://hg.mozilla.org/integration/autoland/rev/945a64d1b467
Remove SessionStoreUtils workaround added in bug 1538762. r=kats

(In reply to Botond Ballo [:botond] from comment #32)

This is unlikely to land in the 67 cycle, but I will see if I can come up with a more targeted fix that addresses bug 1519007 / bug 1531057 and can be uplifted to 67.

Looks like those two other bugs got a fix uplifted to 67, I guess we can call this one wontfix for that release?

(In reply to Julien Cristau [:jcristau] from comment #38)

(In reply to Botond Ballo [:botond] from comment #32)

This is unlikely to land in the 67 cycle, but I will see if I can come up with a more targeted fix that addresses bug 1519007 / bug 1531057 and can be uplifted to 67.

Looks like those two other bugs got a fix uplifted to 67, I guess we can call this one wontfix for that release?

+1. This patch series has some regression risk and we have more targeted fixes in 67, so we don't need this in 67.

Blocks: 1543485

(In reply to Botond Ballo [:botond] from comment #16)

(In reply to Jan Henning [:JanH] from comment #14)

I was mentioning that web platform test mainly because while your initial
patch for this bug does indeed solve the problem of the layout viewport not
exceeding scrollMaxX/Y, it still didn't improve our score on that test.
Therefore I thought that you might be able to kill two birds with one stone
and, once the new API is in place and the session store/history have been
switched over, you might want to change the scrollTo behaviour such that it
not only fixes the original issue from this bug, but also makes us pass that
test as well.

Thanks - I've taken note of the web platform test, and will be sure to get
it passing. Not sure if yet if it will be in this bug or a follow-up.

Filed bug 1543485 to track getting this WPT passing.

Regressions: 1545096
Regressions: 1549625
No longer blocks: 1423011
Regressed by: 1423011
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/498a364e1d4f
Follow-up to remove dangling reference to a fixed bug. r=botond
Regressions: 1703141
No longer regressions: 1703141
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.