Closed
Bug 1516056
Opened 6 years ago
Closed 6 years ago
Restrict the layout scroll offset to the layout scroll range
Categories
(Core :: Panning and Zooming, defect, P2)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla68
People
(Reporter: botond, Assigned: botond)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(10 files, 1 obsolete file)
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details |
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.
|
Assignee | |
Comment 1•6 years ago
|
||
The problem here is that GetScrollRangeForClamping() is returning the visual scroll range even when we are clamping the layout scroll position.
|
|
Assignee | |
Comment 2•6 years ago
|
||
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
|
||
Comment 3•6 years ago
|
||
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.
|
|
||
Comment 4•6 years ago
|
||
|
|
Assignee | |
Comment 5•6 years ago
|
||
Thanks for the analysis!
Is the work you mention the one tracked in bug 1498812? Does it have a dependency on bug 1507279?
|
|
||
Comment 6•6 years ago
|
||
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.
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 7•6 years ago
|
||
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.
|
|
Assignee | |
Comment 8•6 years ago
|
||
(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.
|
|
||
Comment 9•6 years ago
|
||
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.
|
|
Assignee | |
Comment 10•6 years ago
|
||
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?
|
|
Assignee | |
Comment 11•6 years ago
|
||
(And of course, this bug wouldn't land until the other two are sorted out.)
|
|
||
Comment 12•6 years ago
|
||
> 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
|
|
Assignee | |
Comment 13•6 years ago
|
||
(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.
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 14•6 years ago
|
||
(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.
|
|
||
Comment 15•6 years ago
|
||
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.
|
|
Assignee | |
Comment 16•6 years ago
|
||
(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.
|
|
Assignee | |
Comment 17•6 years ago
|
||
(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).
|
|
Assignee | |
Comment 19•6 years ago
|
||
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
status-firefox66:
--- → affected
|
|
Assignee | |
Comment 20•6 years ago
|
||
Updated patch series on top of bug 1517895:
There is one test failure on Android, testing/marionette/harness/marionette_harness/tests/unit/test_typing.py, which I can't reproduce locally.
|
|
Assignee | |
Comment 21•6 years ago
|
||
(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.
|
||
Comment 22•6 years ago
|
||
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:
If the behavior of the visible viewport or virtual viewport has changed that's probably why.
|
|
Assignee | |
Comment 23•6 years ago
|
||
All right, I seemed to have fixed the test failures:
|
|
Assignee | |
Comment 24•6 years ago
|
||
|
|
Assignee | |
Comment 25•6 years ago
|
||
Depends on D20279
|
|
Assignee | |
Comment 26•6 years ago
|
||
Depends on D20280
|
|
Assignee | |
Comment 27•6 years ago
|
||
Depends on D20281
|
|
Assignee | |
Comment 28•6 years ago
|
||
Depends on D20282
|
|
Assignee | |
Comment 29•6 years ago
|
||
Depends on D20283
|
|
Assignee | |
Comment 30•6 years ago
|
||
This avoids the testcase running into bug 1516722.
Depends on D20284
|
|
Assignee | |
Comment 31•6 years ago
|
||
Depends on D20285
|
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → botond
|
|
Assignee | |
Comment 32•6 years ago
|
||
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.
|
|
Assignee | |
Comment 33•6 years ago
|
||
Depends on D20285
|
||
Updated•6 years ago
|
Attachment #9044839 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 34•6 years ago
|
||
Rebased and updated to account for bug 1531531 and bug 1538762.
|
|
Assignee | |
Comment 35•6 years ago
|
||
Try push showing this + bug 1531531 passing tests:
|
||
Comment 36•6 years ago
|
||
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
|
||
Comment 37•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/16b36c86155d
https://hg.mozilla.org/mozilla-central/rev/3b7f8dede084
https://hg.mozilla.org/mozilla-central/rev/a84c86ad7031
https://hg.mozilla.org/mozilla-central/rev/be6e27fbdb2c
https://hg.mozilla.org/mozilla-central/rev/946181497fb1
https://hg.mozilla.org/mozilla-central/rev/62f15d6bd366
https://hg.mozilla.org/mozilla-central/rev/09351cbff684
https://hg.mozilla.org/mozilla-central/rev/945a64d1b467
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
|
||
Comment 38•6 years ago
|
||
(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?
|
|
Assignee | |
Comment 39•6 years ago
|
||
(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.
|
|
Assignee | |
Comment 40•6 years ago
|
||
(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.
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 41•4 years ago
|
||
DONTBUILD because comment-only change.
|
|
||
Comment 42•4 years ago
|
||
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
|
||
Comment 43•4 years ago
|
||
| bugherder | ||
|
||
Updated•4 years ago
|
Keywords: regression
|
|
||
Updated•3 years ago
|
Has Regression Range: --- → yes
You need to log in
before you can comment on or make changes to this bug.
Description
•