Zoom Level doesn't increase/decrease with fingers touch (with e10s on)

RESOLVED FIXED in Firefox 52

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Kanchan Kumari QA, Assigned: kats)

Tracking

51 Branch
mozilla52
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(e10s+, firefox50 unaffected, firefox51 unaffected, firefox52 fixed)

Details

(Whiteboard: [gfx-noted][nightly only])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
[ Tested Version]: 
Build Id 20160829030202
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[Tested touch enabled platforms]: 
Surface Pro 4  
Aspire SW3-013 
ASUS TP500L    

[Pre-requisite]: 
javascript.options.asyncstack; false
browser.tabs.remote.force-enable; true
browser.tabs.remote.autostart.2; true 
layers.async-pan-zoom.enabled; true 
About:support
Multiprocess Windows 1/1 (Enabled by user)
Asynchronous Pan/Zoom wheel input enabled;

[Steps to reproduce]:
1.Launch Firefox on Nightly 51.0a1 by double tapping the icon with  finger	
2.Tap the Location bar and use the onscreen keyboard to type 
  https://en.wikipedia.org/wiki/Peach. 
3.Apply your fingers to increase the zoom level
4.Apply your fingers to decrease the zoom level	

[Expected result]:
Zoom level is increased/decreased smoothly.

[Actual result]:
With e10s enabled: Zoom level doesn’t get increased or decreased, page scrolls down when fingers are used to change the zoom level.

[Note]:
It works fine with e10s disabled.
(Reporter)

Updated

a year ago
tracking-e10s: --- → ?
Blocks: 1244402
status-firefox50: --- → unaffected
OS: Unspecified → Windows
Priority: -- → P3
Whiteboard: [gfx-noted][nightly only]

Updated

a year ago
tracking-e10s: ? → +
status-firefox51: affected → unaffected
status-firefox52: --- → affected
See Also: → bug 1304729
Looking into fixing this.
Assignee: nobody → bugmail
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cfc99723923
Now with modifier support:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f7bd2d8a6b
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
mozreview-review
Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79516

::: gfx/layers/apz/util/APZCCallbackHelper.h:195
(Diff revision 1)
>      IsScrollInProgress(nsIScrollableFrame* aFrame);
> +
> +    /* Notify content of the progress of a pinch gesture that APZ won't do
> +     * zooming for (because the apz.allow_zooming pref is false). This function
> +     * will dispatch appropriate WidgetSimpleGestureEvent events to gecko.
> +     * The widget provided must be non-null.

Actually the widget can be null, and the code will handle it fine. I'll remove this line from the comment block.

Comment 7

a year ago
mozreview-review
Comment on attachment 8794357 [details]
Bug 1298886 - Convert the PinchGestureInput span fields from float to ParentLayerCoord.

https://reviewboard.mozilla.org/r/80844/#review79552

::: gfx/layers/apz/src/GestureEventListener.cpp:52
(Diff revision 1)
> -float GetCurrentSpan(const MultiTouchInput& aEvent)
> +ParentLayerCoord GetCurrentSpan(const MultiTouchInput& aEvent)
>  {
>    const ParentLayerPoint& firstTouch = aEvent.mTouches[0].mLocalScreenPoint;
>    const ParentLayerPoint& secondTouch = aEvent.mTouches[1].mLocalScreenPoint;
>    ParentLayerPoint delta = secondTouch - firstTouch;
>    return delta.Length();

Can we change BasePoint::Length() to return Coord, while we're at it?

(I originally didn't do that for the reason mentioned in bug 1055741 comment 11, but I've since been convinced otherwise (bug 1055741 comment 12)).
Attachment #8794357 - Flags: review?(botond) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79556

r+ on the APZ parts. Going to defer to :dvander on the IPC-related parts.

::: gfx/layers/apz/public/GeckoContentController.h:69
(Diff revision 1)
>                           uint64_t aInputBlockId) = 0;
>  
>    /**
> +   * When the apz.allow_zooming pref is set to false, the APZ will not
> +   * translate pinch gestures to actual zooming. Instead, it will call this
> +   * method to notify gecko of the pinch gesture, and allow it to deal with

deal with it

::: gfx/layers/apz/public/GeckoContentController.h:83
(Diff revision 1)
> +   *        For a SCALE event, this is the difference in span between the
> +   *        previous state and the new state.
> +   * @param aModifiers The keyboard modifiers depressed during the pinch.
> +   */
> +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> +                                  const ScrollableLayerGuid& aGuid,

This alignment, where the second argument type is one column before the first argument type, elsewhere is a result of refactorings that changed method names. Let's please not propagate it as a deliberate style choice :)

Applies to other header files modified in the patch too.

::: gfx/layers/apz/public/GeckoContentController.h:84
(Diff revision 1)
> +   *        previous state and the new state.
> +   * @param aModifiers The keyboard modifiers depressed during the pinch.
> +   */
> +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> +                                  const ScrollableLayerGuid& aGuid,
> +                                  LayoutDeviceCoord aSpan,

I'd call this aSpanChange
Attachment #8794358 - Flags: review?(botond) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79566

Looks good on the IPC side.

::: gfx/layers/apz/util/ContentProcessController.cpp:154
(Diff revision 1)
> +                        const ScrollableLayerGuid& aGuid,
> +                        LayoutDeviceCoord aSpan,
> +                        Modifiers aModifiers)
> +{
> +  // This should never get called
> +  MOZ_ASSERT(false);

nit: MOZ_ASSERT_UNREACHABLE

::: gfx/layers/ipc/RemoteContentController.cpp:101
(Diff revision 1)
> +  // For now we only ever want to handle this NotifyPinchGesture message in
> +  // the parent process, even if the APZ is sending it to a content process.
> +
> +  // If we're in the GPU process, try to find a handle to the parent process
> +  // and send it there.
> +  if (XRE_GetProcessType() == GeckoProcessType_GPU) {

note: We have XRE_IsGPUProcess now
Attachment #8794358 - Flags: review?(dvander) → review+
(In reply to Botond Ballo [:botond] from comment #7)
> Can we change BasePoint::Length() to return Coord, while we're at it?
> 
> (I originally didn't do that for the reason mentioned in bug 1055741 comment
> 11, but I've since been convinced otherwise (bug 1055741 comment 12)).

I filed bug 1305201 for this, in case it snowballs into something larger.

(In reply to Botond Ballo [:botond] from comment #8)
> > +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> > +                                  const ScrollableLayerGuid& aGuid,
> 
> This alignment, where the second argument type is one column before the
> first argument type, elsewhere is a result of refactorings that changed
> method names. Let's please not propagate it as a deliberate style choice :)
> 
> Applies to other header files modified in the patch too.

I'm not sure what you mean by this - the argument types are all lined up on the same column. Maybe the font MozReview is using for you makes this look wrong, or maybe I'm misunderstanding what you mean?

Agreed with the other comments, will fix before landing.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> (In reply to Botond Ballo [:botond] from comment #8)
> > > +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> > > +                                  const ScrollableLayerGuid& aGuid,
> > 
> > This alignment, where the second argument type is one column before the
> > first argument type, elsewhere is a result of refactorings that changed
> > method names. Let's please not propagate it as a deliberate style choice :)
> > 
> > Applies to other header files modified in the patch too.
> 
> I'm not sure what you mean by this - the argument types are all lined up on
> the same column. Maybe the font MozReview is using for you makes this look
> wrong, or maybe I'm misunderstanding what you mean?

Hmm, yeah, looking at the raw diff, the alignment is fine. It must be a bug in MozReview that they don't look aligned there. My bad!

Comment 12

a year ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3437f7cc1db2
Convert the PinchGestureInput span fields from float to ParentLayerCoord. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd20b9ac007d
Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled. r=botond,dvander

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3437f7cc1db2
https://hg.mozilla.org/mozilla-central/rev/bd20b9ac007d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Botond Ballo [:botond] from comment #11)
> Hmm, yeah, looking at the raw diff, the alignment is fine. It must be a bug
> in MozReview that they don't look aligned there.

Filed bug 1305464 about the MozReview bug.
You need to log in before you can comment on or make changes to this bug.