Closed Bug 1504659 Opened 6 years ago Closed 5 years ago

Horizontal navigation from the keyboards doesn't work if Touch Simulation is enabled

Categories

(DevTools :: Responsive Design Mode, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 verified)

VERIFIED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- verified

People

(Reporter: obotisan, Assigned: bradwerth)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(6 files, 8 obsolete files)

1.06 MB, image/gif
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
[Affected versions]:
- Firefox 64.0b6
- Nightly 65.0a1

[Affected platforms]:
- macOS 10.13
- Ubuntu 16.04 x64
- Windows 10 x64


[Steps to reproduce]:
1. Go to https://en.wikipedia.org/wiki/Firefox.
2. Open the Responsive Design Mode (Ctrl+Shift+M - Windows, Linux; Cmd+Opt+M - mac).
3. Enable Touch Simulation.
4. Click on the page and navigate horizontally using the arrows from the keyboard.

[Expected result]:
- The page is scrolled horizontally.

[Actual result]:
- The page is not scrolled horizontally.


[Regression range]:
- last good build: 2018-10-30
- first bad build: 2018-10-04
- mozregression concluded that bug 1290420 (Part 5: Change test function waitForViewportResizeTo to check viewport size.) might have caused this issue.

[Additional notes]:
- Please look at the attached gif.
- You can still scroll vertically using the keyboard.
- Scrolling horizontally works fine if the touch simulation is not active.
Hi Gabriel, can we get a traige on this?
Flags: needinfo?(gl)
Brad, correct me if I am wrong, but did we have a duplicate bug for this that you were already working on?
Flags: needinfo?(gl) → needinfo?(bwerth)
I am not aware of a duplicate. There are many RDM touch simulation bugs at the moment and they may have similar root causes.
Flags: needinfo?(bwerth)
And I'll take this one and try to figure it out.
Assignee: nobody → bwerth
The problem is that nsIScrollableFrame::GetPerceivedScrollingDirections determines that the page is already maximally visible horizontally, and therefore no horizontal scrolling is needed. Setting touch override sets the viewport size to be wider than the actual viewport, and this part of our scrolling code checks this virtual viewport size to determine the need to scroll. I'm looking for the way to get the actual visible size of the viewport and pass it through to this function.
I've heard that we'd like to enable apz.allow_zooming on RDM too.  Is this issue fixed by enabling the pref?  If so I think we should try to enable the pref instead.  Bug 1459312 is one of the blockers IIRC.
CCing Botond.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> I've heard that we'd like to enable apz.allow_zooming on RDM too.  Is this
> issue fixed by enabling the pref?  If so I think we should try to enable the
> pref instead.  Bug 1459312 is one of the blockers IIRC.

Could be. To be clear, attachment 9023797 [details] does not solve the problem. I think it will be part of the solution and I'll update, set reviewers, build tests when I am convinced I have it solved.
Priority: -- → P2
Attachment #9023797 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1dbe3852e298
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/bcebb3fd72d3
Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45f63618f66e
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/f2574f5b186e
Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl
I've decided that the proposed approach in attachment 9028142 [details] is not fixable. The issue is correctly characterized in comment 5, but this patch attempts to solve the problem by making the viewport size match the visible rect area. This causes the tests for Bug 602580 to fail. I believe the correct solution will require a new GetVisibleRect method to be defined and for that method to return the visible rect in viewport space. The code that checks scrollability should then be changed to call this new method. I'll proceed with this plan and ask for re-review.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #22)
> I've decided that the proposed approach in attachment 9028142 [details] is
> not fixable. The issue is correctly characterized in comment 5, but this
> patch attempts to solve the problem by making the viewport size match the
> visible rect area. This causes the tests for Bug 602580 to fail. I believe
> the correct solution will require a new GetVisibleRect method to be defined
> and for that method to return the visible rect in viewport space. The code
> that checks scrollability should then be changed to call this new method.
> I'll proceed with this plan and ask for re-review.

I looked into this a bit, and there's some fun history behind this :)

The core of the question is, should window.inner{Width,Height} report the dimensions of the visual viewport, or the layout viewport. (Background on these terms: [1]).

  * Initially, before the days of mobile, they would report the 
    dimensions of the visual viewport, because that's all there was.

  * In bug 602580 (2011), it was decided that it should report the
    dimensions of the layout viewport (called the "CSS viewport"
    at the time) instead. This was implemented, and the test in
    question was added to test this behaviour.

  * In bug 919437 (2013), it was decided that it should report the
    dimensions of the visual viewport after all. The implementation
    was changed accordingly. However, the change only affected
    mobile, and the test added in bug 602580 didn't run on mobile
    (I think because at the time we hadn't stood up all of our
    test suites on mobile yet), so it didn't start failing.

  * In bug 1075071 (2014), people were trying to get our tests
    running on mobile and noticed that this test was failing, so
    it was annotated as permafailing on Android, an annotation
    which is still present today [2].

  * Now, we are trying to bring some mobile behaviour to desktop
    for RDM purposes, and that's causing the test to now fail on
    desktop.

Based on this history, it seems to me that the test from bug 602580 is (now) wrong. It has been wrong ever since bug 919437, it's just never been caught (or rather, it was caught in bug 1075071, but the test was disabled instead of being fixed).

Therefore, I think we should change or fix the test, rather than changing our approach to the fix here.

(Ironically, we may end up changing the semantics of window.inner{Width,Height} yet again. The model Blink has settled on [3] is to have them report layout viewport dimensions, and we'll probably have to follow suit. However, that shouldn't be a concern in this bug. If and when we make that change, we'll update the relevant tests accordingly.)

[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md
[2] https://searchfox.org/mozilla-central/rev/3160ddc1f0ab55d230c595366662c62950e5c785/dom/tests/mochitest/dom-level0/mochitest.ini#15
[3] https://docs.google.com/spreadsheets/d/11DfDDa-s1hePVwBn3PZidlPJZ9ahhkJ44dyuMiOQtrc/edit#gid=0
(In reply to Botond Ballo [:botond] from comment #23)
> I looked into this a bit, and there's some fun history behind this :)
> 
> The core of the question is, should window.inner{Width,Height} report the
> dimensions of the visual viewport, or the layout viewport. (Background on
> these terms: [1]).
> 
>   * Initially, before the days of mobile, they would report the 
>     dimensions of the visual viewport, because that's all there was.
> 
>   * In bug 602580 (2011), it was decided that it should report the
>     dimensions of the layout viewport (called the "CSS viewport"
>     at the time) instead. This was implemented, and the test in
>     question was added to test this behaviour.
> 
>   * In bug 919437 (2013), it was decided that it should report the
>     dimensions of the visual viewport after all. The implementation
>     was changed accordingly. However, the change only affected
>     mobile, and the test added in bug 602580 didn't run on mobile
>     (I think because at the time we hadn't stood up all of our
>     test suites on mobile yet), so it didn't start failing.

This is incredibly helpful background. Okay, to use these terms, the existing patch Part 1 set the ScrollFrameHelper::mScrollPort equal to the visual viewport even in non-APZzoom (non-mobile) situations. This is intentional and correct. Given this, I will:

1) Add a comment to Part 1 of the patch that makes the mScrollPort = visual viewport relationship more explicit.
2) Update Part 2 of the patch to correct the test logic for the Bug 602580, and mark it as expected-pass on all platforms.
3) Review the other failing tests from the try runs above and see if they need updates or indicate the need for code fixes. (tests for Bug 1133905, and Bug 486935)
Attachment #9028143 - Attachment description: Bug 1504659 Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl! → Bug 1504659 Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!
Attachment #9031266 - Attachment description: Bug 1504659 Part 2: Make window.innerWidth and innerHeight always reflect the visual viewport size. → Bug 1504659 Part 2: Make window.innerWidth and innerHeight getters and setters consistently use the visual viewport size.
Attachment #9028143 - Attachment description: Bug 1504659 Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl! → Bug 1504659 Part 4: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!
Attachment #9031268 - Attachment description: Bug 1504659 Part 4: Update existing tests to distinguish between layout and visual viewports. → Bug 1504659 Part 5: Update tests to distinguish between layout and visual viewports.
See Also: → 1501665
Negative chrome extents are not useful. Nothing in this patch stack seems
to be generating these negative extents, but tests aren't passing because
of them. My testing seems to show that these tests fail on the current tree!
Until we understand this better, this seems like a reasonable way to get
the tests passing again.

Depends on D14643
Attachment #9028143 - Attachment description: Bug 1504659 Part 4: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl! → Bug 1504659 Part 5: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!
Attachment #9031268 - Attachment description: Bug 1504659 Part 5: Update tests to distinguish between layout and visual viewports. → Bug 1504659 Part 6: Update tests to distinguish between layout and visual viewports.
Attachment #9031624 - Attachment description: Bug 1504659 Part 6: Update XUL tests to check window size after load has been fired (so presshell is inited). → Bug 1504659 Part 7: Update XUL tests to check window size after load has been fired (so presshell is inited).
Attachment #9031626 - Attachment description: Bug 1504659 Part 7: Update tests to remove expectation that docshell sizes are unchanged when swapping docshells. → Bug 1504659 Part 8: Update tests to remove expectation that docshell sizes are unchanged when swapping docshells.
Attachment #9031627 - Attachment description: Bug 1504659 Part 8: Make popup menu tests less sensitive to sub-pixel differences in positioning. → Bug 1504659 Part 9: Make popup menu tests less sensitive to sub-pixel differences in positioning.
Attachment #9034077 - Attachment is obsolete: true
Attachment #9031621 - Attachment is obsolete: true
Attachment #9031266 - Attachment is obsolete: true
Attachment #9031627 - Attachment is obsolete: true
Attachment #9031626 - Attachment is obsolete: true
Attachment #9031624 - Attachment is obsolete: true
Attachment #9031268 - Attachment is obsolete: true
Attachment #9028143 - Attachment description: Bug 1504659 Part 5: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl! → Bug 1504659 Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ebae5c1463b
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/0b7bbe6dda1d
Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl

Backed out 2 changesets (Bug 1504659) for test_innerWidthHeight_script.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0b7bbe6dda1d98b81149a86c62801e66b40ea32c&selectedJob=220471736

Backout link: https://hg.mozilla.org/integration/autoland/rev/b3639a45cfc3d49ed698ea2cbee64a4ffbe4922c

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220471736&repo=autoland&lineNumber=5744

[task 2019-01-08T01:15:13.212Z] 01:15:13 INFO - TEST-START | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html
[task 2019-01-08T01:15:13.963Z] 01:15:13 INFO - TEST-INFO | started process screentopng
[task 2019-01-08T01:15:14.402Z] 01:15:14 INFO - TEST-INFO | screentopng: exit 0
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html | innerWidth is css viewport width - got 600, expected 320
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - runSubTest@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:13:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - onload@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:1:1
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html | innerHeight is css viewport height - got 400, expected 320
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - runSubTest@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:14:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - onload@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:1:1
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html | innerWidth returns value that was set - got 600, expected 300
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - runSubTest@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:17:5
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - onload@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:1:1
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-01-08T01:15:14.406Z] 01:15:14 INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html | innerHeight returns value that was set - got 400, expected 300
[task 2019-01-08T01:15:14.407Z] 01:15:14 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:5
[task 2019-01-08T01:15:14.407Z] 01:15:14 INFO - runSubTest@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:21:5
[task 2019-01-08T01:15:14.407Z] 01:15:14 INFO - onload@dom/tests/mochitest/dom-level0/innerWidthHeight_script.html:1:1
[task 2019-01-08T01:15:14.408Z] 01:15:14 INFO - GECKO(5364) | MEMORY STAT | vsize 20973372MB | residentFast 390MB
[task 2019-01-08T01:15:14.408Z] 01:15:14 INFO - TEST-OK | dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html | took 1007ms

Flags: needinfo?(bwerth)
Attachment #9028143 - Attachment description: Bug 1504659 Part 2: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl! → Bug 1504659 Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!

I still can't figure out a way to land this WITHOUT updating our getters and setters of innerWidth and innerHeight, which will have broad impact on web behavior.

At https://searchfox.org/mozilla-central/rev/c3ebaf6de2d481c262c04bb9657eaf76bf47e2ac/dom/base/nsGlobalWindowOuter.cpp#3078, the setter for innerWidth currently does not check if the visual viewport has been set (as the getter does). Instead, it checks if the CSS viewport has been explictly set, then modifies that. This makes test dom/tests/mochitest/dom-level0/test_innerWidthHeight_script.html fail because otherwise sane JS code that sets then gets innerWidth doesn't get the value that it put in because (with this patch) the visual viewport has been overridden by the window opener, and the CSS viewport has been overridden by the meta viewport tag.

Without this patch, the innerWidth setter does one of two things:

  1. Set the css viewport size only. It will do this if the css viewport has been overridden through a meta viewport tag. This will also effectively set the visual viewport size as long as it hasn't already been overridden.
  2. Set both viewports to the same value.

With this patch, the innerWidth setter will do one of these two slightly different things:

  1. Set the visual viewport only. It will do this if the visual viewport has already been overridden.
  2. Set both viewports to the same value, possibly by manipulating the css viewport override if it has already been overrridden.

This is weird, but it maintains continuity with current behavior and ensures that innerWidth always returns the visual viewport. It does not provide a way for the innerWidth setter to override the visual viewport -- but neither does the current implementation.

Flags: needinfo?(bwerth)
Blocks: 1514779

(In reply to Brad Werth [:bradwerth] from comment #43)

I still can't figure out a way to land this WITHOUT updating our getters and setters of innerWidth and innerHeight, which will have broad impact on web behavior.

Note that innerWidth and innerHeight cannot be set by web content, only by internal (chrome JS) code (see this).

So, if we're only changing the behaviour of the setters, that at least should not affect web content.

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

(In reply to Brad Werth [:bradwerth] from comment #43)

I still can't figure out a way to land this WITHOUT updating our getters and setters of innerWidth and innerHeight, which will have broad impact on web behavior.

Note that innerWidth and innerHeight cannot be set by web content, only by internal (chrome JS) code (see this).

Ok, I guess that's not entirely true. Looking at the code, it appears that content JS can set innerWidth/Height under the following circumstances:

  • It's a desktop platform rather than mobile

  • The window was opened by the script via window.open()

  • The tab in question is the only tab in its window,
    which presumably can happen either if the user has
    unchecked "Open links in tabs instead of new windows"
    in Preferences, or if the user has closed the original
    tab which opened the new one

In any case, this doesn't seem applicable to situations where the visual and layout viewport diverge. (And indeed, the notion of changing innerWidth, which essentially amounts to resizing the browser window, in a mobile context doesn't make sense.) So, I think we're fine.

(And, in case you're wondering why test_innerWidthHeight_script.html gets to set innerWidth/Height in spite of it not being a chrome mochitest, it's because it flips the dom.disable_window_move_resize pref, which seems to control this ability among other things.)

The tests for Bug 1133905 all compare the visibility of scrollbars with
differently-sized css viewports. This patch has some affect on the
viewport sizing that I don't understand, and it causes some of the tests
to start passing and some to start failing.

The test for Bug 1242172 has elements sized to height 100% and checking
for the presence or absence of scrollbars. In this case the patch appears
to increase the css viewport height and decrease the width -- again for
reasons I don't understand -- and this affects the scrollbar sizes.

Depends on D15996

(In reply to Brad Werth [:bradwerth] (PTO until 1/14/2019) from comment #49)

Created attachment 9035406 [details]
Bug 1504659 Part 5: Update Android reftest expectations.

The tests for Bug 1133905 all compare the visibility of scrollbars with
differently-sized css viewports. This patch has some affect on the
viewport sizing that I don't understand, and it causes some of the tests
to start passing and some to start failing.

Interesting; I pulled the patches up to and including Part 4, and ran the bug 1133905 tests locally, to try to reproduce and understand the changes in behaviour, but in my local runs the tests are behaving as per the original expectations.

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

Interesting; I pulled the patches up to and including Part 4, and ran the bug 1133905 tests locally, to try to reproduce and understand the changes in behaviour, but in my local runs the tests are behaving as per the original expectations.

Likewise for the bug 12422172 test.

On the other hand, a Try push shows the test behaviour changing as reflected in the Part 5 patch.

I guess that's because in automation, the tests run on an emulator, which has a different screen size than a device.

I've tried to reproduce the failures locally in the emulator, without success so far (bug 1519489, bug 1519588). Will keep trying on Monday.

Ok, I was able to get an x86 emulator working, and I can reproduce the test failures.

They seem to be related to the first patch, which comes into play because Android reftests run with apz.allow_zooming=false (wat).

So here are my thoughts on the the changes in test behaviour:

  • The tests concern scrollbar rendering behaviour.
  • The changes in behaviour are caused by the first patch, likely due to taking different codepaths in ScrollFrameHelper::LayoutScrollbars().
  • The patch in question only affects the configuration where we use MobileViewportManager but apz.allow_zooming=false. This is the case for Android reftests as mentioned above, but not actual production behaviour on Android, where we run with apz.allow_zooming=true. Therefore, this patch should not affect production behaviour on Android.
  • This configuration is, however, used in RDM (after all, that's the point of the patch), so it may alter scrollbar rendering behaviour in RDM.
  • The changes to the test expectations suggest that the alterations to scrollbar rendering behaviour may include both regressions (as some tests start to fail) and improvements (as some tests start to pass).
  • As this patch fixes a fairly significant regression in RDM behaviour, I think it's reasonable to take it even though it may cause some regressions to scrollbar rendering behaviour in RDM, which seems less severe.

I do think it would be a good idea to investigate and fix the test cases that are still failing, as a follow-up. We already have bug 1308702 on file for the bug 1133905 test failures on Android, so we can use that to track.

Attachment #9035183 - Attachment description: Bug 1504659 Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports. → Bug 1504659 Part 2: Update innerWidth/Height setters to better handle overridden visual viewports.
Blocks: 1308702
Attachment #9035183 - Attachment description: Bug 1504659 Part 2: Update innerWidth/Height setters to better handle overridden visual viewports. → Bug 1504659 Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports.
Blocks: rdm-ux
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/738e1ee854eb
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/a88ccc9308e0
Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/231b16b0091e
Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl
https://hg.mozilla.org/integration/autoland/rev/783f7ccbd797
Part 4: Update tests to distinguish between layout and visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/749c9dcbbd7f
Part 5: Update Android reftest expectations. r=botond

I am clearing my needinfo since I am sure Brad will be right on top of this.

@Brad, I think you have set your bugzilla to not allow people to needinfo you and therefore I was needinfo instead (I am assuming you might've left this setting on from your pto)

Flags: needinfo?(gl)

Brad, with your patches and my patches for bug 1423013 made layout/reftests/transform/compound-1a.html fail[1] on Android. I could reproduce the failure and tracked down that the RefreshVisualViewportSize call [2] in RefreshViewportSize() caused the failure. Though I didn't know that apz.allow_zooming is false on reftest, in the failure case the difference between the test and the reference images looks just a blurry top border of the skewed transform box. Probably there is rounding errors somewhere. Anyway, I've confirmed that with apz.allow_zooming=true the reftest works fine so, given that apz.allow_zooming is true on Android, it's not a big problem I suppose. So please add apz.allow_zooming pref for the reftest here. (I did land my patches before I noticed the pref is disabled in reftest harness).

Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=749c9dcbbd7f7d8146a71d8cb2a1acb07db1b8c7&tochange=8af061c4dfc01a8ecaff8072b70110f85f1c5060&selectedJob=221919301&searchStr=android%2C4.3%2Capi16%2B%2Copt%2Creftests%2Ctest-android-em-4.3-arm7-api-16%2Fopt-reftest-27%2Cr%28r27%29
[2] https://hg.mozilla.org/integration/autoland/rev/738e1ee854eb24b72679b35252a4889b9603c003#l1.36

Blocks: 1520320
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e12097e2b60
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd806b413342
Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/0818e2b1f2c5
Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl
https://hg.mozilla.org/integration/autoland/rev/229ed2090371
Part 4: Update tests to distinguish between layout and visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/793e842be7f6
Part 5: Update Android reftest expectations. r=botond
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28bcc4da3092
Backed out 5 changesets for android reftest failures. CLOSED TREE
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba34bc4f9903
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/571aef2b0929
Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/c017d1d14820
Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl
https://hg.mozilla.org/integration/autoland/rev/ddc9999232d6
Part 4: Update tests to distinguish between layout and visual viewports. r=botond
https://hg.mozilla.org/integration/autoland/rev/d3d3222460fc
Part 5: Update Android reftest expectations. r=botond
Flags: needinfo?(bwerth)

I can reproduce this issue with Nightly 65.0a1 (2018-11-05) (64-bit) on Linux x86_64

I am verifying that this is fixed in latest Nightly 66.0a1

Build id             20190117215514
User Agent       Mozilla/5.0 (X11; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

[bugday-20190116]

Regressed by: 1556131

The test added in this bug now fails with devtools.responsive.metaViewport.enabled true, which we are trying to enable in Bug 1521934. This is the regression range:

INFO: Last good revision: b8141448e0baa767e8eff61e28c5a418c438f3d2 (2019-07-22)
INFO: First bad revision: 026812d05de4546d50b277499171050bea4296d4 (2019-07-25)

I'm trying to narrow it down further. The fix for this regression (with the pref turned on), will likely be added to Bug 1521934, and this test will be updated to test with the pref on, as I originally intended.

Bug 1567237 seems very likely to the be regression culprit.

I managed to verified the fix on Nightly from 2019-01-17 using Windows 10 x64. The issue was not reproducing anymore.
But the displayed has changed and on the latest Nightly the image is fit to the margins of the screen and there is no horizontal scroll.

Status: RESOLVED → VERIFIED
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: