Closed
Bug 1359211
Opened 7 years ago
Closed 7 years ago
Handle touch-scrolling of XUL <listbox> with APZ enabled
Categories
(Core :: Panning and Zooming, enhancement, P3)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
Details
(Keywords: correctness, Whiteboard: [gfx-noted])
Attachments
(1 file)
XUL <listbox> is a special snowflake in that it builds an nsIScrollableFrame (which makes it use APZ scrolling by default), but doesn't play nicely with APZ scrolling (doesn't respect the displayport, and is designed to scroll by whole-line increments only). Wheel-scrolling it works fine because it listens for preventDefault()s wheel events, so APZ doesn't scroll in response to them, and scrolls itself instead. Scrollbar dragging also works fine: it uses a custom scrollbar mediator, and, as of bug 1355376, opts out of the APZ drag codepath. For touch events, though, it doesn't currently do anything, causing APZ to try to touch-scroll it as if it were a normal scroll frame, resulting in bad behaviour (like unpainted regions being brought into view and then never being painted). <listbox> needs to handle touch events similar to how it handles wheel events: preventDefault() them and scroll itself. We did this in bug 1302736 for <tree>, so it should just be a matter of adapting that code for <listbox>.
Assignee | ||
Comment 1•7 years ago
|
||
(Every platform is an APZ platform now :)).
Summary: Handle touch-scrolling of XUL <listbox> on APZ platforms → Handle touch-scrolling of XUL <listbox> with APZ enabled
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Here's what I'm asking for reviews on: dao: the change to listbox.xml kats: the C++ changes bz: WebIDL approval
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8861180 [details] Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS. https://reviewboard.mozilla.org/r/133144/#review135946 Looks good to me, but please get :dao or one of the other toolkit peers to sign off on this, and you'll need a DOM peer to sign off on the .webidl change, I think. ::: toolkit/content/widgets/listbox.xml:84 (Diff revision 1) > extends="chrome://global/content/bindings/general.xml#basecontrol"> > > <implementation implements="nsIDOMXULMultiSelectControlElement"> > <field name="_lastKeyTime">0</field> > <field name="_incrementalString">""</field> > - > + looks like whitespace was added, please remove ::: toolkit/content/widgets/listbox.xml:875 (Diff revision 1) > this.ensureIndexIsVisible(targetIndex); > break; > } > ]]> > </handler> > + nit: trailing ws ::: toolkit/content/widgets/listbox.xml:910 (Diff revision 1) > + <handler event="touchend"> > + <![CDATA[ > + this._touchY = -1; > + ]]> > + </handler> > + more trailing ws
Attachment #8861180 -
Flags: review?(bugmail) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8861180 [details] Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS. https://reviewboard.mozilla.org/r/133144/#review136430 r=me on the webidl bits
Attachment #8861180 -
Flags: review?(bzbarsky) → review+
Updated•7 years ago
|
status-firefox55:
--- → affected
status-firefox57:
affected → ---
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8861180 [details] Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS. https://reviewboard.mozilla.org/r/133144/#review137240 ::: toolkit/content/widgets/listbox.xml:84 (Diff revision 2) > extends="chrome://global/content/bindings/general.xml#basecontrol"> > > <implementation implements="nsIDOMXULMultiSelectControlElement"> > <field name="_lastKeyTime">0</field> > <field name="_incrementalString">""</field> > - > + please drop this change ::: toolkit/content/widgets/listbox.xml:875 (Diff revision 2) > this.ensureIndexIsVisible(targetIndex); > break; > } > ]]> > </handler> > + trailing spaces ::: toolkit/content/widgets/listbox.xml:895 (Diff revision 2) > + <handler event="touchmove"> > + <![CDATA[ > + if (event.touches.length == 1 && > + this._touchY >= 0) { > + var deltaY = this._touchY - event.touches[0].screenY; > + var lines = Math.trunc(deltaY / this.listBoxObject.getRowHeight()); please use let instead of var ::: toolkit/content/widgets/listbox.xml:910 (Diff revision 2) > + <handler event="touchend"> > + <![CDATA[ > + this._touchY = -1; > + ]]> > + </handler> > + trailing spaces again
Attachment #8861180 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Addressed review comments and autolanded.
Comment 10•7 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/76bcb2239fb0 Handle touch-scrolling of XUL <listbox> in JS. r=bz,dao,kats
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/76bcb2239fb0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•