Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585573002:
Allow navigation to chrome://md-settings/pointer-overlay (Closed)
|
Created:
4 years ago by michaelpg Modified:
4 years ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow navigation to chrome://md-settings/pointer-overlay
The pointers subpage has some clever logic to close the subpage if all
connected touchpads and mice are detached. But it breaks loading the subpage
directly, because we don't receive the device status synchronously.
Prevent the auto-close logic running too early by not initializing the mouse and
touchpad Polymer properties.
BUG=674729
R=dpapad@chromium.org
Committed: https://crrev.com/ec33688c7d81592695a5ecc92b198d6883d2a39f
Cr-Commit-Position: refs/heads/master@{#439318}
Patch Set 1 #
Total comments: 6
Patch Set 2 : closure fixes #Patch Set 3 : rebase #
Messages
Total messages: 20 (9 generated)
Description was changed from ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org ========== to ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad, mind taking a look at this brief CL?
LGTM. https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:53: attached: function() { Nit: @override https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:128: this.checkPointerSubpage_(); Is this call necessary? IIUC sometimes the desired behavior when visiting chrome://md-settings/pointer-overlay is to immediately navigate elsewhere? From a UX perspective, I would expect to stay in whatever page I explicitly navigated (even if everything on that page is disabled).
On 2016/12/16 01:20:53, dpapad wrote: > LGTM. > > https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/device_page/device_page.js (right): > > https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/device_page/device_page.js:53: attached: > function() { > Nit: @override > > https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/device_page/device_page.js:128: > this.checkPointerSubpage_(); > Is this call necessary? IIUC sometimes the desired behavior when visiting > chrome://md-settings/pointer-overlay is to immediately navigate elsewhere? From > a UX perspective, I would expect to stay in whatever page I explicitly navigated > (even if everything on that page is disabled). The page is entirely blank in that case, not even a title. We only show mouse settings if you have a mouse; we only show touchpad settings if you have a touchpad. (So, for example, a Chromebox would never see the touchpad settings.) If we have neither, why display anything -- and which sections would we display? both? We pretty much never want to show touchpad on a Chromebox unless you actually attach a touchpad. And we shouldn't show Mouse on a convertible or something with just a touchscreen. I can't really imagine why you would navigate to this page at all if you didn't have pointers -- maybe it's bookmarked or in your history -- but I think going back to Device is a reasonable solution.
https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:53: attached: function() { On 2016/12/16 01:20:53, dpapad wrote: > Nit: @override Done + some other minor closure cleanup. https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:128: this.checkPointerSubpage_(); On 2016/12/16 01:20:53, dpapad wrote: > Is this call necessary? IIUC sometimes the desired behavior when visiting > chrome://md-settings/pointer-overlay is to immediately navigate elsewhere? From > a UX perspective, I would expect to stay in whatever page I explicitly navigated > (even if everything on that page is disabled). Oops, I replied to this as a message.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2585573002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:128: this.checkPointerSubpage_(); >If we have neither, why display anything -- and which sections would we display? both? Preferably there would be a message "Nothing to see here", or "No devices", so we wouldn't actually display nothing, but also would not surprise the user by magically navigating to the parent page.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2585573002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:128: this.checkPointerSubpage_(); On 2016/12/17 01:05:08, dpapad wrote: > >If we have neither, why display anything -- and which sections would we > display? both? > > Preferably there would be a message "Nothing to see here", or "No devices", so > we wouldn't actually display nothing, but also would not surprise the user by > magically navigating to the parent page. Sure, that's doable - "patches welcome" :-) This CL is fixing objectively broken behavior (loading chrome://md-settings/pointer-overlay directly always redirects to the device section, 100% of the time). Removing the existing behavior of closing the subpage when there are no pointer devices, by keeping the page open with a "You can't do anything here, go away" message, should be a separate CL. We'd also need a new string or two at least.
Description was changed from ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org ==========
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481943908932130,
"parent_rev": "050d84f1d9a5742d48259f0da331a997b695b209", "commit_rev":
"d89328fb427834e600ac61d91809c70ff11ede29"}
Message was sent while issue was closed.
Description was changed from ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org ========== to ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org Review-Url: https://codereview.chromium.org/2585573002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org Review-Url: https://codereview.chromium.org/2585573002 ========== to ========== Allow navigation to chrome://md-settings/pointer-overlay The pointers subpage has some clever logic to close the subpage if all connected touchpads and mice are detached. But it breaks loading the subpage directly, because we don't receive the device status synchronously. Prevent the auto-close logic running too early by not initializing the mouse and touchpad Polymer properties. BUG=674729 R=dpapad@chromium.org Committed: https://crrev.com/ec33688c7d81592695a5ecc92b198d6883d2a39f Cr-Commit-Position: refs/heads/master@{#439318} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ec33688c7d81592695a5ecc92b198d6883d2a39f Cr-Commit-Position: refs/heads/master@{#439318} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
