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

Status: VERIFIED FIXED

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

Description

[Oana Botisan, Desktop QA]

Attachment: horizontal scrolling.gif

[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.

Comment 1

[Marion Daly [:mdaly]]

Hi Gabriel, can we get a traige on this?

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Brad, correct me if I am wrong, but did we have a duplicate bug for this that you were already working on?

Comment 3

[Brad Werth [:bradwerth]]

I am not aware of a duplicate. There are many RDM touch simulation bugs at the moment and they may have similar root causes.

Comment 4

[Brad Werth [:bradwerth]]

And I'll take this one and try to figure it out.

Comment 5

[Brad Werth [:bradwerth]]

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.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 1: Update viewport size from the MobileViewportManager even when not zoomed.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

CCing Botond.

Comment 9

[Brad Werth [:bradwerth]]

(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.

Comment 10

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=gl!

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 3: Add tests of viewport RDM scroll behavior, with and without touch simulation. r=gl!

Depends on D13173

Comment 12

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5556bb2c469d9b280d58b29390751b39096b51

Comment 13

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98fffa620ff1587bf0a1e487f5bf1dd0ce2d8cfd

Comment 14

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c17b0b5f0942773585d03e685b0f8b52ca471845

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1504659) for browser_scroll.js failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&selectedJob=215518251&revision=bcebb3fd72d3cec5ce1cb7478d7f7aeb3cffca09

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&fromchange=77b100d5fe369ba2cc7ae86c66ee5c4bd9a6519a&selectedJob=215518251&searchStr=os%2Cx%2C10.10%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64%2Fdebug-mochitest-devtools-chrome-e10s-2%2Cm-e10s%28dt2%29

backout: https://hg.mozilla.org/integration/autoland/rev/38b4e8f428f7f41feca673ee8833733f27a37460

Comment 17

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c667a95d33475796cfba7bf46cab4da8fabb3a

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on test_innerWidthHeight_script.html 

Backout link: https://hg.mozilla.org/integration/autoland/rev/4e65e04a0d2bdfa613ee1623b7bd6bd1003a152b
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=215619797&revision=f2574f5b186e520f5ee9a1626c8bc828f2761bf1
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=215619797&repo=autoland&lineNumber=24246

Comment 20

[Narcis Beleuzu [:NarcisB]]

There were also reftest failures:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=215614534&repo=autoland&lineNumber=1813

Comment 21

[Narcis Beleuzu [:NarcisB]]

Also Android reftests:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=215636699&repo=autoland&lineNumber=2470

Comment 22

[Brad Werth [:bradwerth]]

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.

Comment 23

[Botond Ballo [:botond]]

(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

Comment 24

[Brad Werth [:bradwerth]]

(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)

Comment 25

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 2: Make window.innerWidth and innerHeight getters and setters consistently use the visual viewport size.

Depends on D13173

Comment 26

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 6: Update tests to distinguish between layout and visual viewports.

Depends on D13174

Comment 27

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b5f3b6bd53bdd9f633eb1d3a415044a2a18981e

Comment 28

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deaf7651430591dc54cc997b0f2cacf68c90dcf7

Comment 29

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 3: Make VisualViewport properly report size of fixed-size viewports.

Depends on D14491

Comment 30

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 7: Update XUL tests to check window size after load has been fired (so presshell is inited).

Depends on D14492

Comment 31

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 8: Update tests to remove expectation that docshell sizes are unchanged when swapping docshells.

Depends on D14646

Comment 32

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 9: Make popup menu tests less sensitive to sub-pixel differences in positioning.

Depends on D14648

Comment 33

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f49fd685759d04d0866a611623b087e503ec0a

Comment 34

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead9d70d1977d01c6e79a33752966b764f2599a1

Comment 35

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 4: Prevent nsWindowWatcher::SizeOpenedWindow from using negative chrome extents.

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

Comment 36

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4cb1ee5a80583f0ecf49c744637ec17a4d2dbc

Comment 37

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=989f43a04e51c575cf9ad3589878c9a2d0dbd816

Comment 38

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8eedd350fe893b74993aaf997ab4cf811016a9

Comment 39

[Pulsebot]

Pushed by [e-mail]:
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

Comment 40

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 41

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 2: Update innerWidth/Height getters and setters to better handle overridden visual viewports.

Depends on D13173

Comment 42

[Brad Werth [:bradwerth]]

Attachment: Bug 1504659 Part 4: Update tests to distinguish between layout and visual viewports.

Depends on D13174

Comment 43

[Brad Werth [:bradwerth]]

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.

Comment 44

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=feef945b97f8ffc3966e25f2234078a02a367745

Comment 45

[Botond Ballo [:botond]]

(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.

Comment 46

[Botond Ballo [:botond]]

(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.

Comment 47

[Botond Ballo [:botond]]

(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.)

Comment 48

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a84f76c8250364042898276102acc8b780fdb5

Comment 49

[Brad Werth [:bradwerth]]

Attachment: 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.

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

Comment 50

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cdec093733a33056b61522079dda4cb3b29cab

Comment 51

[Botond Ballo [:botond]]

(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.

Comment 52

[Botond Ballo [:botond]]

(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.

Comment 53

[Botond Ballo [:botond]]

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.

Comment 54

[Botond Ballo [:botond]]

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

Comment 55

[Botond Ballo [:botond]]

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).

Comment 56

[Botond Ballo [:botond]]

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.

Comment 57

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c73a01ed48123d975866a151a0e5e5c311bbad0

Comment 58

[Pulsebot]

Pushed by [e-mail]:
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

Comment 59

[Narcis Beleuzu [:NarcisB]]

Backed out for reftest failures on frame-reconstruction-scroll-clamping.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/73cd4b7b53ba8edbc148e40d101fd9427846ea26
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=android%2C4.3%2Capi16%2B%2Copt%2Creftests%2Ctest-android-em-4.3-arm7-api-16%2Fopt-reftest-28%2Cr%28r28%29&revision=749c9dcbbd7f7d8146a71d8cb2a1acb07db1b8c7
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221902914&repo=autoland&lineNumber=3829

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

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)

Comment 61

[Hiroyuki Ikezoe (:hiro)]

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

Comment 62

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b985aeff52886090370e899fb43925af08f1fdd4

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e12097e2b60
Part 1: Make RefreshVisualViewportSize allow non-APZ zooming, and call it during RefreshViewportSize. r=botond

Comment 64

[Pulsebot]

Pushed by [e-mail]:
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

Comment 65

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28bcc4da3092
Backed out 5 changesets for android reftest failures. CLOSED TREE

Comment 66

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2Creftest&fromchange=3e12097e2b601e2dd06b868e98bf7167c6c6aa94&tochange=50582093318ac50411fc4ddc0a4a730fb0b748d4&selectedJob=222261932

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222261932&repo=autoland&lineNumber=1955
Ref analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/GjDmfcIOTmmjgR99YHy6Cw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

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

Comment 67

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10646a9aa9a78a6aa9c2d9f35c11d38cb71549a9

Comment 68

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ecb5f570e54b29ffbf0879c3bcbdc9c3ee2419f

Comment 69

[Pulsebot]

Pushed by [e-mail]:
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

Comment 70

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ba34bc4f9903
https://hg.mozilla.org/mozilla-central/rev/571aef2b0929
https://hg.mozilla.org/mozilla-central/rev/c017d1d14820
https://hg.mozilla.org/mozilla-central/rev/ddc9999232d6
https://hg.mozilla.org/mozilla-central/rev/d3d3222460fc

Comment 71

[Sayed [:LiquidO2]]

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]

Comment 72

[Brad Werth [:bradwerth]]

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.

Comment 73

[Brad Werth [:bradwerth]]

Bug 1567237 seems very likely to the be regression culprit.

Comment 74

[Oana Botisan, Desktop QA]

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.