Closed Bug 1138252 Opened 5 years ago Closed 5 years ago

Load BrowserElementPanning.js only if touch events are enabled

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: paul, Assigned: paul)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

On Desktop, we can pan (click, hold, move) even if the touch events are explicitly disabled. BrowserElementPanning falls back to mouse events in this case.
Attached patch v1Splinter Review
Fabrice, do I need a review from a DOM peer for this patch?
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8571113 - Flags: review?(fabrice)
Comment on attachment 8571113 [details] [diff] [review]
v1

Review of attachment 8571113 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.
Please land on m-c, not larch.
Attachment #8571113 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Hi, this failed to apply:

Hunk #1 FAILED at 36
1 out of 1 hunks FAILED -- saving rejects to file dom/browser-element/BrowserElementChild.js.rej
patching file dom/ipc/preload.js
Hunk #1 FAILED at 85
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/preload.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh patch.diff

could you take a look, thanks!
Flags: needinfo?(paul)
Keywords: checkin-needed
Blocks: graphene
Flags: needinfo?(paul)
Attached patch v2Splinter Review
Things have changed a bit since bug 995394. Re-asking for a review.
Attachment #8571971 - Flags: review?(botond)
Comment on attachment 8571971 [details] [diff] [review]
v2

Review of attachment 8571971 [details] [diff] [review]:
-----------------------------------------------------------------

BrowserElementPanningAPZDisabled.js has a codepath for handling mouse events when touch events are not supported [1]. Are we sure this patch doesn't disable it on some platforms where we want that codepath?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanningAPZDisabled.js#45
(In reply to Botond Ballo [:botond] from comment #6)
> Comment on attachment 8571971 [details] [diff] [review]
> v2
> 
> Review of attachment 8571971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BrowserElementPanningAPZDisabled.js has a codepath for handling mouse events
> when touch events are not supported [1]. Are we sure this patch doesn't
> disable it on some platforms where we want that codepath?

As long as the touch events pref is enabled, it's not a problem.

B2G desktop: http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#340

We are working on another runtime for desktop where we don't want panning with the mouse.
Comment on attachment 8571971 [details] [diff] [review]
v2

Review of attachment 8571971 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I guess no one has been using the non-touch codepaths in BrowserElementPanning.js, then.

Could you please file a follow-up to remove these dead codepaths?
Attachment #8571971 - Flags: review?(botond) → review+
Blocks: 1139727
Attached patch v2 to landSplinter Review
(updated commit message)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb1d40720855
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.