Closed
Bug 1319950
Opened 6 years ago
Closed 6 years ago
Enabling touch simulation then resizing the viewport should not disable touch simulation
Categories
(DevTools :: Responsive Design Mode, defect, P1)
DevTools
Responsive Design Mode
Tracking
(firefox51 unaffected, firefox52 verified, firefox53 verified, firefox54 verified)
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | verified |
firefox53 | --- | verified |
firefox54 | --- | verified |
People
(Reporter: ntim, Assigned: jryans)
References
Details
(Whiteboard: [rdm-v2])
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
jryans
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
gl
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
gl
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
gl
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
1.75 MB,
video/mp4
|
Details |
STR: - Open RDM - Enable touch - Resize viewport Actual result: - touch gets disabled ER: - touch should stay enabled
Assignee | ||
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [triage][rdm-v2] → [rdm-v2]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35fd6863b3112432b20b7e2581adc532efb4940d
Reporter | ||
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8829022 [details] Bug 1319950 - DPR watching should move to actor. https://reviewboard.mozilla.org/r/106232/#review107252 ::: devtools/client/responsive.html/index.js:104 (Diff revision 1) > // Dispatch a `changeDisplayPixelRatio` action when the browser's pixel ratio is changing. > // This is usually triggered when the user changes the monitor resolution, or when the > // browser's window is dragged to a different display with a different pixel ratio. > +// TODO: It would be better to move this watching into the actor, so that it can be > +// better synchronized with any overrides that might be applied. Also, reading a single > +// value like this makes less sense with multiple viewports. nit: reading a value
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. https://reviewboard.mozilla.org/r/106236/#review107254 Is "viewport" really redundant in the names? I expect we'll likely re-introduce it with multiple viewports, so I see no point of removing viewport now.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8829023 [details] Bug 1319950 - Wrap dPR state in object for consistency. https://reviewboard.mozilla.org/r/106234/#review107288 ::: devtools/client/responsive.html/types.js:100 (Diff revision 1) > - * The location of the document displayed in the viewport(s). > - */ > -exports.location = PropTypes.string; > > /** > - * The progression of the screenshot > + * devicePixelRatio for a given viewport. s/devicePixelRatio/Device Pixel Ratio ::: devtools/client/responsive.html/types.js:102 (Diff revision 1) > -exports.location = PropTypes.string; > > /** > - * The progression of the screenshot > + * devicePixelRatio for a given viewport. > */ > -exports.screenshot = { > +const pixelRatio = exports.pixelRatio = { Let's move pixelRatio below networkThrottling to maintain an alphabetical order for the VIEWPORT types. ::: devtools/client/responsive.html/types.js:104 (Diff revision 1) > /** > - * The progression of the screenshot > + * devicePixelRatio for a given viewport. > */ > -exports.screenshot = { > +const pixelRatio = exports.pixelRatio = { > > - isCapturing: PropTypes.bool, > + // The devicePixelRatio value s/devicePixelRatio/Device Pixel Ratio ::: devtools/client/responsive.html/types.js:154 (Diff revision 1) > + // The devicePixelRatio of the viewport > + pixelRatio: PropTypes.shape(pixelRatio), > + > +}; > + > +/* ACTION */ I like to keep things sorted in alphabetical order. Perhaps consider moving these ACTION types above GLOBAL.
Attachment #8829023 -
Flags: review?(gl) → review+
Reporter | ||
Comment 9•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #6) > Comment on attachment 8829022 [details] > Bug 1319950 - DPR watching should move to actor. > > https://reviewboard.mozilla.org/r/106232/#review107252 > > ::: devtools/client/responsive.html/index.js:104 > (Diff revision 1) > > // Dispatch a `changeDisplayPixelRatio` action when the browser's pixel ratio is changing. > > // This is usually triggered when the user changes the monitor resolution, or when the > > // browser's window is dragged to a different display with a different pixel ratio. > > +// TODO: It would be better to move this watching into the actor, so that it can be > > +// better synchronized with any overrides that might be applied. Also, reading a single > > +// value like this makes less sense with multiple viewports. > > nit: reading a value Hmm, Mozreview cut off "a single" form the viewport. I guess the comment is fine then.
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. https://reviewboard.mozilla.org/r/106236/#review107398 ::: devtools/client/responsive.html/app.js:51 (Diff revision 1) > onBrowserMounted() { > window.postMessage({ type: "browser-mounted" }, "*"); > }, > > + onChangeDevice(id, device) { > + // XXX: Get rid of this extra message? Do we want to keep these comments? ::: devtools/client/responsive.html/manager.js:455 (Diff revision 1) > > switch (event.data.type) { > + case "change-device": > + this.onChangeDevice(event); > + break; > + case "change-pixel-ratio": Move "change-pixel-ratio" below "change-network-throtting" case to keep the switch block alphabetically ordered.
Attachment #8829024 -
Flags: review?(gl) → review+
Comment 11•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. https://reviewboard.mozilla.org/r/106236/#review107398 > Do we want to keep these comments? Dropping this issue since it is removed in the next patch
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8829025 [details] Bug 1319950 - Only clear properties on resize if device is active. https://reviewboard.mozilla.org/r/106238/#review107414
Attachment #8829025 -
Flags: review?(gl) → review+
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. https://reviewboard.mozilla.org/r/106236/#review107398 > Dropping this issue since it is removed in the next patch I removed them anyway, there were WIP notes not worth keeping. > Move "change-pixel-ratio" below "change-network-throtting" case to keep the switch block alphabetically ordered. Fixed.
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. https://reviewboard.mozilla.org/r/106236/#review107254 In my view, it will remain redundant even with multiple viewports. Nearly everything in RDM is about a viewport, so it seems unnecessary to include that word all over the place.
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8829023 [details] Bug 1319950 - Wrap dPR state in object for consistency. https://reviewboard.mozilla.org/r/106234/#review107288 > s/devicePixelRatio/Device Pixel Ratio I don't think "Device Pixel Ratio" is a good spelling of the name, since it's not really a proper noun... I would think "devicePixelRatio" (the name of the window property for this value) or "device pixel ratio" are good spellings. We're not very consistent right now, so I filed bug 1333254 about this. In this case, I guess I'll use "Device pixel ratio". > Let's move pixelRatio below networkThrottling to maintain an alphabetical order for the VIEWPORT types. Seems reasonable, fixed. > s/devicePixelRatio/Device Pixel Ratio Fixed as above. > I like to keep things sorted in alphabetical order. Perhaps consider moving these ACTION types above GLOBAL. Yeah... I am not sure how to balance the order of this file yet... I have a mental model of "overall app state" that is something like "global, then viewports, then actions", but it's pretty nebulous. I think I'll drop this one for now and keep thinking about it as I continue to make changes. I want something richer like several levels of an outline, or maybe some way to list out the full app state, but I don't see clean way to do it yet.
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7654d2dbc5c772f75f4c97f045e477b722fc891
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8829022 [details] Bug 1319950 - DPR watching should move to actor. https://reviewboard.mozilla.org/r/106232/#review107948
Attachment #8829022 -
Flags: review+
Comment 21•6 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6f1cefc5b510 DPR watching should move to actor. r=jryans https://hg.mozilla.org/integration/autoland/rev/b6076330933b Wrap dPR state in object for consistency. r=gl https://hg.mozilla.org/integration/autoland/rev/a0c4fc1d8ae7 Use consistent naming with viewport properties. r=gl https://hg.mozilla.org/integration/autoland/rev/54350550ed2c Only clear properties on resize if device is active. r=gl
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f1cefc5b510 https://hg.mozilla.org/mozilla-central/rev/b6076330933b https://hg.mozilla.org/mozilla-central/rev/a0c4fc1d8ae7 https://hg.mozilla.org/mozilla-central/rev/54350550ed2c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Updated•6 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 23•6 years ago
|
||
Comment on attachment 8829023 [details] Bug 1319950 - Wrap dPR state in object for consistency. Approval Request Comment [Feature/Bug causing the regression]: Touch simulation in DevTools RDM [User impact if declined]: Touch simulation becomes disabled if you resize the viewport, which is expected to be a frequent activity. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: QE is flagged for testing, but I don't think it needs to block uplift. [List of other uplifts needed for the feature/fix]: 3 patches in this bug are needed (another patch in this bug only changes comments and can be ignored). [Is the change risky?]: No [Why is the change risky/not risky?]: Only affects touch in DevTools RDM. [String changes made/needed]: None
Attachment #8829023 -
Flags: approval-mozilla-beta?
Attachment #8829023 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•6 years ago
|
||
Comment on attachment 8829024 [details] Bug 1319950 - Use consistent naming with viewport properties. See comment 23.
Attachment #8829024 -
Flags: approval-mozilla-beta?
Attachment #8829024 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 25•6 years ago
|
||
Comment on attachment 8829025 [details] Bug 1319950 - Only clear properties on resize if device is active. See comment 23.
Attachment #8829025 -
Flags: approval-mozilla-beta?
Attachment #8829025 -
Flags: approval-mozilla-aurora?
Comment 26•6 years ago
|
||
Comment on attachment 8829023 [details] Bug 1319950 - Wrap dPR state in object for consistency. devtools RDM fix for aurora53 and beta52, should be in b2
Attachment #8829023 -
Flags: approval-mozilla-beta?
Attachment #8829023 -
Flags: approval-mozilla-beta+
Attachment #8829023 -
Flags: approval-mozilla-aurora?
Attachment #8829023 -
Flags: approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8829024 -
Flags: approval-mozilla-beta?
Attachment #8829024 -
Flags: approval-mozilla-beta+
Attachment #8829024 -
Flags: approval-mozilla-aurora?
Attachment #8829024 -
Flags: approval-mozilla-aurora+
Updated•6 years ago
|
Attachment #8829025 -
Flags: approval-mozilla-beta?
Attachment #8829025 -
Flags: approval-mozilla-beta+
Attachment #8829025 -
Flags: approval-mozilla-aurora?
Attachment #8829025 -
Flags: approval-mozilla-aurora+
Comment 27•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/227831ce1a7b https://hg.mozilla.org/releases/mozilla-aurora/rev/b48c0364b756 https://hg.mozilla.org/releases/mozilla-aurora/rev/c933e719a567
Flags: in-testsuite+
Comment 28•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a6cb4268d7b4 https://hg.mozilla.org/releases/mozilla-beta/rev/7ea353eafc60 https://hg.mozilla.org/releases/mozilla-beta/rev/bf69066e6917
Comment 29•6 years ago
|
||
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170201030207 When trying to verify this, I noticed that the touch simulation is still lost when resizing the viewport if the touch simulation is enabled by selecting the device. Please see the screen cast for more detail. Ryan, is this expected in any way?
Flags: needinfo?(jryans)
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Simona B [:simonab ] from comment #29) > Created attachment 8832836 [details] > LostTouchSimulation.mp4 > > Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 > Firefox/54.0 > Build ID: 20170201030207 > > When trying to verify this, I noticed that the touch simulation is still > lost when resizing the viewport if the touch simulation is enabled by > selecting the device. > > Please see the screen cast for more detail. > > Ryan, is this expected in any way? Yes, that's "expected", at least from the way the code is written today. It's quite hard to guess what would be most obvious to users... But for the moment anyway, if you select a device and then resize manually, all device parameters are cleared including touch. If however no device is selected and you enable touch, resizing should keep it enabled (that's what this bug has fixed). We have some ideas to improve the UX of changing away from a device like you mention in bug 1329843, so hopefully it can be improved.
Flags: needinfo?(jryans)
Comment 31•6 years ago
|
||
Thank you Ryan for all the additional information. Verified as fixed on the latest Nightly 54.0a1 (Build ID: 20170201030207) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
Comment 32•6 years ago
|
||
I managed to reproduce this issue on Firefox 52.0a1 (2016-11-09), under Windows 10x64. The issue is no longer reproducible on Firefox 52.0b2 (64-bit), or on Firefox 53.0a2 (2017-02-03). Tests were performed under Windows 10x64, Mac OS X 10.12.1, Ubuntu 16.04x64
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•