Closed Bug 1295883 Opened 8 years ago Closed 8 years ago

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

Categories

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

Unspecified
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file)

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.
Hi Jan, thanks for the patch. I am assigning this bug to you since you are working on it. Please feel free to reset.
Assignee: nobody → jbeich
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.
Attachment #8781851 - Flags: review?(sshih)
Attachment #8781851 - Flags: review?(bugs)
(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 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.
Attachment #8781851 - Flags: review?(bugs) → review+
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 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.)
Attachment #8781851 - Flags: review+ → review?(bugs)
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 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)
Attachment #8781851 - Flags: review?(bugs) → review+
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
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.
Flags: needinfo?(bugs)
this has r+.
"So, with that, r+ (or explain why it doesn't work)"
Flags: needinfo?(bugs)
(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.
How do you disagree with "(or explain why it doesn't work)" ?
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70bea246ead
Chase Linux scroll wheel behavior on Tier3 desktop Unix. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d70bea246ead
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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]:
Attachment #8781851 - Flags: approval-mozilla-beta?
Attachment #8781851 - Flags: approval-mozilla-aurora?
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.
Attachment #8781851 - Flags: approval-mozilla-beta?
Attachment #8781851 - Flags: approval-mozilla-beta+
Attachment #8781851 - Flags: approval-mozilla-aurora?
Attachment #8781851 - Flags: approval-mozilla-aurora+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.