Closed Bug 1250398 Opened 6 years ago Closed 6 years ago

Disable APZ for source editor

Categories

(DevTools :: Source Editor, defect)

defect
Not set
normal

Tracking

(firefox45 unaffected, firefox46+ fixed, firefox47+ fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Attached image Scrolling (APZ enabled)
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.
[Tracking Requested - why for this release]:

APZ is enabled for 46 and later, so this affects Dev. Ed. too.
Here's what you see with the patch applied to disable APZ for our editors.
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.
Attachment #8722374 - Flags: review?(gl) → review+
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.
(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
Flags: needinfo?(mstange)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Or we could an arbitrary amount.

We could *choose* an arbitrary amount.
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.
Flags: needinfo?(mstange)
Okay, I have updated the patch to account for line and page modes.
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/
Comment on attachment 8722374 [details]
MozReview Request: Bug 1250398 - Disable APZ for source editors. r=gl

Probably worth another look, just to double check.
Attachment #8722374 - Flags: review+ → review?(gl)
Attachment #8722374 - Flags: review?(gl) → review+
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/684054d29f9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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/
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.
Attachment #8722374 - Flags: approval-mozilla-aurora?
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.
Attachment #8722374 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
[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
Depends on: 1252983
Depends on: 1327066
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.