J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Description • 8 years ago

The current version of APZ (async pan zoom) can cause tearing effects in the source editor where the line numbers float around disconnected from the code.

I think we should disable this behavior for source editors until it improves.  :kats told me how to do this in bug 1246290 comment 12.

See the recording as an example of the effect.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 1 • 8 years ago

[Tracking Requested - why for this release]:

APZ is enabled for 46 and later, so this affects Dev. Ed. too.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 2 • 8 years ago

Here's what you see with the patch applied to disable APZ for our editors.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 3 • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 4 • 8 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=476db9f206ed

	Gabriel [:gl] (ΦωΦ)
	Comment 5 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

https://reviewboard.mozilla.org/r/35997/#review32631

LGTM. Let's remove the double spacing between the sentences in the comment to be consistent with the formatting in the file.

	Markus Stange [:mstange]
	Comment 6 • 8 years ago

You need to handle all deltaMode values. deltaX/deltaY are only in pixels if you use high precision scroll devices, but not if you scroll with a traditional mouse wheel, for example.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 7 • 8 years ago

(In reply to Markus Stange [:mstange] from comment #6)
> You need to handle all deltaMode values. deltaX/deltaY are only in pixels if
> you use high precision scroll devices, but not if you scroll with a
> traditional mouse wheel, for example.

What's a good way to handle line and page mode?  I noticed a test that sends fake events to measure how many pixels each one would have moved[1].  Or we could an arbitrary amount.  Perhaps there is a better way?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/events/test/test_dom_wheel_event.html#58-99

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 8 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Or we could an arbitrary amount.

We could *choose* an arbitrary amount.

	Markus Stange [:mstange]
	Comment 9 • 8 years ago

Ideally you'd ask CodeMirror for the size of a row or page and use that. I don't know if it has useful APIs for that. If not, choosing an arbitrary amount should be fine.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 10 • 8 years ago

Okay, I have updated the patch to account for line and page modes.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 11 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 12 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

Probably worth another look, just to double check.

	Gabriel [:gl] (ΦωΦ)
	Comment 13 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

https://reviewboard.mozilla.org/r/35997/#review33273

LGTM - scrolling isn't quite as smooth but that is because we are using a fixed.

::: devtools/client/sourceeditor/editor.js:306
(Diff revision 2)
> +      // Disable APZ for source editors.  It currently causes the line numbers

Can we remove the double space after each sentence to be consistent with the current file format?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 14 • 8 years ago

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #13)
> ::: devtools/client/sourceeditor/editor.js:306
> (Diff revision 2)
> > +      // Disable APZ for source editors.  It currently causes the line numbers
> 
> Can we remove the double space after each sentence to be consistent with the
> current file format?

Sorry, I've fixed that now.

	Pulsebot
	Comment 15 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/684054d29f9b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/684054d29f9b

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 17 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 18 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

Approval Request Comment
[Feature/regressing bug #]: APZ was enabled on desktop for 46 in bug 1178298.  This causes poor UX with DevTools code editors at the moment.
[User impact if declined]:  If declined, the line number gutter on the DevTools code editor will "swim around" when scrolling, which looks quite strange and broken.  This patch takes over wheel scrolling for DevTools editors to work around the issue for now.
[Describe test coverage new/current, TreeHerder]: No specific test coverage, but tested manually locally, and landed on m-c.
[Risks and why]: Low risk, only affects scrolling in DevTools code editors.
[String/UUID change made/needed]: No string changes.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 8 years ago

Regression from 46 (from apz), tracking.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 8 years ago

Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

Workaround for dev tools/apz regressions. Please uplift to aurora since we plan to enable APZ for 46.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 21 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c159bd99b25

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 22 • 8 years ago

Is this broken again? I just tried in Nightly on OS X and the gutter is lagging behind the scroll in the devtools debugger pane.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 23 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22)
> Is this broken again? I just tried in Nightly on OS X and the gutter is
> lagging behind the scroll in the devtools debugger pane.

Hm, this is bug 1256727. Setting apz.paint_skipping.enabled to false fixes it.

	Mayur Patil
	Comment 24 • 8 years ago

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected