Closed
Bug 1067128
Opened 10 years ago
Closed 10 years ago
[e10s] _setupSwipeGesture() tries to access content when scrolling down
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
Attachments
(1 file, 1 obsolete file)
1.33 KB,
patch
|
spohl
:
review+
|
Details | Diff | Splinter Review |
.... and throws "TypeError: content is null".
Assignee | ||
Comment 1•10 years ago
|
||
Seized the chance to add a proper uninit() function. I would have not added listeners at all but the comment at the top of the file seems to indicate that we want to cancel events.
Comment 2•10 years ago
|
||
Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch
Review of attachment 8489135 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> + return;
> + }
> +
> + for (let name of this.GESTURE_EVENTS) {
> + addEventListener(`Moz${name}`, this, true);
What's this?
Attachment #8489135 -
Flags: review?(felipc) → review+
Comment 3•10 years ago
|
||
Does this mean that we won't/can't support swipe animations in e10s?
Comment 4•10 years ago
|
||
Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch
Review of attachment 8489135 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry, I have no idea what happened.
::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> + return;
> + }
> +
> + for (let name of this.GESTURE_EVENTS) {
> + addEventListener(`Moz${name}`, this, true);
Oh, http://tc39wiki.calculist.org/es6/template-strings/ I guess.
Attachment #8489135 -
Flags: review+ → review?(felipc)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #2)
> > + for (let name of this.GESTURE_EVENTS) {
> > + addEventListener(`Moz${name}`, this, true);
Yes, those are template strings :)
(In reply to Stephen Pohl [:spohl] from comment #3)
> Does this mean that we won't/can't support swipe animations in e10s?
This means we need to rewrite it to support e10s.
Comment 6•10 years ago
|
||
> (In reply to Stephen Pohl [:spohl] from comment #3)
> > Does this mean that we won't/can't support swipe animations in e10s?
>
> This means we need to rewrite it to support e10s.
I'm assuming that we're going with this workaround because e10s is on a tight deadline? Is there already a bug on file to implement this for e10s? IIRC this also affects history swipe navigation without animations that some people have come to rely on.
Assignee | ||
Comment 7•10 years ago
|
||
Yeah, we're going with lots of workarounds because we want to make basic browsing in e10s work while iteratively fixing all the stuff that hasn't been written with e10s in mind.
Bug 863514 was filed to fix this. Are you talking about the back/fwd navigation with two-finger swipes? I of course don't want that to break and I think I tested that with the given patch applied. I really just want to get rid of that error for now when scrolling.
Comment 8•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #7)
> Bug 863514 was filed to fix this.
Thanks! I wasn't aware of this bug but will make sure to CC myself.
> Are you talking about the back/fwd
> navigation with two-finger swipes? I of course don't want that to break and
> I think I tested that with the given patch applied. I really just want to
> get rid of that error for now when scrolling.
Yes, I was expecting back/forward navigation with two-finger swipes to break with this patch and I still get this impression after quickly reading through browser-gestureSupport.js. It might be better to just leave isVerticalSwipe set to false when gMultiProcessBrowser is true in _setupSwipeGesture. If I'm mistaken and regular two-finger swipe navigation is indeed unbroken, please ignore my comment.
Comment 9•10 years ago
|
||
(In reply to Stephen Pohl [:spohl] from comment #8)
> It might be better to just leave isVerticalSwipe set to false when
> gMultiProcessBrowser is true in _setupSwipeGesture.
... and skip over the part that's accessing 'content', of course.
Comment 10•10 years ago
|
||
I would expect something like this to work:
diff --git a/browser/base/content/browser-gestureSupport.js b/browser/base/content/browser-gestureSupport.js
--- a/browser/base/content/browser-gestureSupport.js
+++ b/browser/base/content/browser-gestureSupport.js
@@ -184,22 +184,22 @@ let gGestureSupport = {
*/
_setupSwipeGesture: function GS__setupSwipeGesture(aEvent) {
if (!this._swipeNavigatesHistory(aEvent)) {
return false;
}
let isVerticalSwipe = false;
if (aEvent.direction == aEvent.DIRECTION_UP) {
- if (content.pageYOffset > 0) {
+ if (gMultiProcessBrowser || content.pageYOffset > 0) {
return false;
}
isVerticalSwipe = true;
} else if (aEvent.direction == aEvent.DIRECTION_DOWN) {
- if (content.pageYOffset < content.scrollMaxY) {
+ if (gMultiProcessBrowser || content.pageYOffset < content.scrollMaxY) {
return false;
}
isVerticalSwipe = true;
}
if (isVerticalSwipe) {
// Vertical overscroll has been temporarily disabled until bug 939480 is
// fixed.
return false;
Assignee | ||
Comment 11•10 years ago
|
||
Yeah that sounds like the right solution for now.
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8489135 -
Attachment is obsolete: true
Attachment #8489135 -
Flags: review?(felipc)
Attachment #8489588 -
Flags: review?(spohl.mozilla.bugs)
Attachment #8489588 -
Flags: review?(felipc)
Comment 13•10 years ago
|
||
Comment on attachment 8489588 [details] [diff] [review]
0001-Bug-1067128-e10s-_setupSwipeGesture-tries-to-access-.patch, v2
Review of attachment 8489588 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8489588 -
Flags: review?(spohl.mozilla.bugs) → review+
Updated•10 years ago
|
Attachment #8489588 -
Flags: review?(felipc)
Assignee | ||
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in
before you can comment on or make changes to this bug.
Description
•