1295883 - Handle scroll wheel on a focused number input for other Unix desktop platforms as well

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jan Beich]

X11/GTK focus and scroll behavior are same for every Unix-like system. To avoid confusion let's keep platform-specific behavior under dom/html/ using the same conditionals.

Comment 1

[Jan Beich]

Attachment: Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

Review commit: https://reviewboard.mozilla.org/r/72186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72186/

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Jan, thanks for the patch. I am assigning this bug to you since you are working on it. Please feel free to reset.

Comment 3

[Ming-Chou Shih [:stone]]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

We also need to take Android into consideration. Maybe we can use "#if !defined(XP_MACOSX) && !defined(ANDROID)" in the source codes and "skip-if = (os == 'mac') || (os == 'android')" in the mochitest.ini

And please find module owners to review the patch. You can find "suggested reviewers" when changing patch state to "r?". Thanks.

Comment 4

[Jan Beich]

(In reply to Ming-Chou Shih [:stone] from comment #3)
> We also need to take Android into consideration.

Why? Android defines XP_LINUX. If you don't like statu quo then #ifdef MOZ_WIDGET_COCOA maybe a better idea.

> And please find module owners to review the patch.

For dependent bugs I always ask the original change author first, then its reviewer, then module peer/owner. This order guarantees r+ comes from the person who understands both original change and my fix the most.

> You can find "suggested reviewers" when changing patch state
> to "r?". Thanks.

I don't see "suggested reviewers" for attachment 8781851 [details]. Does the feature work with mozreview?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

https://reviewboard.mozilla.org/r/72186/#review69860

We may want to tweak handling on Android, but that could be a separate patch or bug.

Comment 6

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

https://reviewboard.mozilla.org/r/72186/#review69860

comment 3 was right. Try failed on Android running mochitests. It appears XP_LINUX and os=='linux' are not the same thing.

 29 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug1261673.html | Handle wheel in number input test-1 expect 4 get 5

Comment 7

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72186/diff/1-2/

Comment 8

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

(mozreview didn't reset r+ after update for some reason.)

Comment 9

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

https://reviewboard.mozilla.org/r/72186/#review70642

bug 1261673 and bug 1261674 mochitests are green on Try. Unrelated orange can be ignored.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

https://reviewboard.mozilla.org/r/72186/#review70744

I guess you could add still && !defined(MOZ_B2G)

But, maybe we want #if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
Wouldn't that work with other unix systems too.
I think that would be better. So, with that, r+
(or explain why it doesn't work)

Comment 11

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

https://reviewboard.mozilla.org/r/72186/#review70744

> I guess you could add still && !defined(MOZ_B2G)

`MOZ_B2G` can co-exist with `XP_LINUX` and `MOZ_WIDGET_GTK` i.e., Firefox OS uses Gonk on Android and Gtk on Linux. The platform/emulator was discontinued half a year ago but for some reason isn't even listed as Tier3. Treeherder no longer lists B2G Desktop Linux, and while one can test locally, doing so on BSDs is even less supported.

```sh
$ fgrep app .mozconfig
ac_add_options --enable-application=b2g

$ ./mach test dom/html/test/test_bug1261673.html
It looks like you tried to run a mach command from an invalid context. The mochitest
command failed to meet the following conditions:

  is_buildapp_supported - Must have a firefox or b2g or android or mulet build.

Run |mach help| to show a list of all commands available to the current context.
```

https://discourse.mozilla-community.org/t/firefox-os-connected-devices-announcement/6864
https://developer.mozilla.org/docs/Supported_build_configurations

> But, maybe we want #if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)

`MOZ_WIDGET_GTK == 3` may refer to mobile platforms with Wayland but otherwise would work fine for BSDs. As both bug 1261673 and bug 1282866 landed in Firefox 50, I don't need to care about `MOZ_WIDGET_QT` when backporting to Aurora.

Darwin/X11 (Tier3) uses Gtk but with bug 1261673 comment 19 left unanswered it's not clear which behavior it should use.

Note, the above ifdef has poor style as it mixes `XP_*` with `MOZ_WIDGET_*` but
- dom/html/*.cpp use `#ifdef XP_MACOSX` over `#ifdef MOZ_WIDGET_COCOA`
- dom/html/test/mochitest.ini uses `toolkit == 'windows'` over `os == 'win'`
- `#ifdef MOZ_WIDGET_GTK` would be `toolkit == 'gtk2' || toolkit == 'gtk3'` in mochitest.ini

Comment 12

[Jan Beich]

What needs to be done to proceed? Bug 1261673 is unclear why scrolling differs between platforms other than "because Chrome there behaves so" (strawman). Neither Chrome on Android or Chrome OS were tested, so bringing up Firefox OS (aka B2G) seems unfair. As Firefox OS defines XP_LINUX (triggers bug 1261673 behavior) on Android/Gonk and Linux I'd prefer to keep statu quo unless its maintainers speak up, probably in a separate bug.

Note, the condition for r+ in comment 10 hasn't been met thus needinfo.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

this has r+.
"So, with that, r+ (or explain why it doesn't work)"

Comment 14

[Jan Beich]

(In reply to Olli Pettay [:smaug] from comment #10)
> But, maybe we want #if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
...
> So, with that, r+ (or explain why it doesn't work)

I disagreed with *that* in comment 11. Hmm, maybe you can file a separate bug for B2G instead.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

How do you disagree with "(or explain why it doesn't work)" ?

Comment 16

[Jan Beich]

Due to uncertainty about the desired behavior on B2G neither your fix nor my statu quo seem correct. Given the lack of an explicit OK from you I've tried to err on the side of caution. It seems this was unnecessary.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70bea246ead
Chase Linux scroll wheel behavior on Tier3 desktop Unix. r=smaug

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d70bea246ead

Comment 19

[Jan Beich]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

Approval Request Comment
[Feature/regressing bug #]: bug 1261673 feature parity
[User impact if declined]: Confusingly different input field scrolling between GTK platforms
[Describe test coverage new/current, TreeHerder]: landed on m-c
[Risks and why]: Low, can only accidentally switch other Tier1 platforms to Linux behavior.
[String/UUID change made/needed]:

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8781851 [details]
Bug 1295883 - Chase Linux scroll wheel behavior on Tier3 desktop Unix.

This has been on Nightly for a month, seems safe to uplift to Aurora51, Beta50.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/709ef83b8446

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f719285cd16