Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627973008:
Language settings: Wait 100ms to close language action menu on checkbox change (Closed)
|
Created:
3 years, 11 months ago by michaelpg Modified:
3 years, 11 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, Dan Beam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLanguage settings: Wait 100ms to close language action menu on checkbox change
Provide feedback to the user by keeping the language detail menu open briefly
when the user toggles a checkbox inside the menu.
BUG=654964
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2627973008
Cr-Commit-Position: refs/heads/master@{#444254}
Committed: https://chromium.googlesource.com/chromium/src/+/c5bbdca617d68284f396f83f6294179cff4d8854
Patch Set 1 #
Total comments: 3
Messages
Total messages: 12 (5 generated)
Description was changed from ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org ========== to ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL. See bug for link to video showing the difference.
I'm not thrilled about the UI approach of having a checkbox inside of an action menu. Maybe this could be a stop-gap fix, but imo the bug (or a bug) should continue to look at this UI. https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/languages_page_browsertest.js:258: // Guaranteed to run later than the menu close delay. Could you point me to docs or something so I can understand why this would be true? i.e. I would not intuitively expect that setTimeout(..., 8); and setTimeout(..., 9); will necessarily execute that that exact order. I would think that if they get delayed they may pile up. iiuc, the guarantee is that they will not execute sooner than the timeout, but may execute at some arbitrary time after the timeout. BTW, even if this didn't normally guarantee the order, I guess I only really need to know that the order will guaranteed for chromium unit test (which may help make the explanation easier). This seems to reinforce this (unless I'm misunderstanding these docs): "There are a number of reasons why a timeout may take longer to fire than anticipated." From https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/se...
Description was changed from ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/01/14 01:15:36, dschuyler wrote: > I'm not thrilled about the UI approach of having a checkbox inside of an action > menu. Maybe this could be a stop-gap fix, but imo the bug (or a bug) should > continue to look at this UI. Yes, it's a problematic UI. bettes@ is investigating other fixes like moving these settings [back] into a "detail" sub-page. https://crbug.com/654964#c6 So this is a temporary patch that makes things just slightly better. > > https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... > File chrome/test/data/webui/settings/languages_page_browsertest.js (right): > > https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... > chrome/test/data/webui/settings/languages_page_browsertest.js:258: // Guaranteed > to run later than the menu close delay. > Could you point me to docs or something so I can understand why this would be > true? i.e. I would not intuitively expect that > > setTimeout(..., 8); > and > setTimeout(..., 9); > > will necessarily execute that that exact order. I would think that if they get > delayed they may pile up. iiuc, the guarantee is that they will not execute > sooner than the timeout, but may execute at some arbitrary time after the > timeout. > > BTW, even if this didn't normally guarantee the order, I guess I only really > need to know that the order will guaranteed for chromium unit test (which may > help make the explanation easier). > > This seems to reinforce this (unless I'm misunderstanding these docs): > "There are a number of reasons why a timeout may take longer to fire than > anticipated." From > https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/se...
https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/languages_page_browsertest.js:258: // Guaranteed to run later than the menu close delay. On 2017/01/14 01:15:36, dschuyler wrote: > Could you point me to docs or something so I can understand why this would be > true? i.e. I would not intuitively expect that > > setTimeout(..., 8); > and > setTimeout(..., 9); > > will necessarily execute that that exact order. I would think that if they get > delayed they may pile up. iiuc, the guarantee is that they will not execute > sooner than the timeout, but may execute at some arbitrary time after the > timeout. > > BTW, even if this didn't normally guarantee the order, I guess I only really > need to know that the order will guaranteed for chromium unit test (which may > help make the explanation easier). > > This seems to reinforce this (unless I'm misunderstanding these docs): > "There are a number of reasons why a timeout may take longer to fire than > anticipated." From > https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/se... Totally right that the actual timeout is not guaranteed. But the callbacks are well-ordered according to the spec, and the order is what matters. For these two statements: 1. setTimeout(fn1, n1) 2. setTimeout(fn2, n2) if n1 <= n2, fn1 runs before fn2. Thus kMenuCloseDelay + 1 ensures the menu close callback has been called first. Source: https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-s... The task (function) is not queued until after this step: "Wait until any invocations of this algorithm that had the same method context [window], that started before this one, and whose timeout is equal to or less than this one's, have completed." The task must be queued on the same task queue as other tasks from the "timer task source", and tasks on a queue are executed in queue order.
As a 'better than it was temporary patch' this looks good. LGTM https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2627973008/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/languages_page_browsertest.js:258: // Guaranteed to run later than the menu close delay. On 2017/01/14 04:39:13, michaelpg wrote: > On 2017/01/14 01:15:36, dschuyler wrote: > > Could you point me to docs or something so I can understand why this would be > > true? i.e. I would not intuitively expect that > > > > setTimeout(..., 8); > > and > > setTimeout(..., 9); > > > > will necessarily execute that that exact order. I would think that if they get > > delayed they may pile up. iiuc, the guarantee is that they will not execute > > sooner than the timeout, but may execute at some arbitrary time after the > > timeout. > > > > BTW, even if this didn't normally guarantee the order, I guess I only really > > need to know that the order will guaranteed for chromium unit test (which may > > help make the explanation easier). > > > > This seems to reinforce this (unless I'm misunderstanding these docs): > > "There are a number of reasons why a timeout may take longer to fire than > > anticipated." From > > > https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/se... > > Totally right that the actual timeout is not guaranteed. But the callbacks are > well-ordered according to the spec, and the order is what matters. For these two > statements: > > 1. setTimeout(fn1, n1) > 2. setTimeout(fn2, n2) > > if n1 <= n2, fn1 runs before fn2. Thus kMenuCloseDelay + 1 ensures the menu > close callback has been called first. > > Source: > https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-s... > > The task (function) is not queued until after this step: > > "Wait until any invocations of this algorithm that had the same method context > [window], that started before this one, and whose timeout is equal to or less > than this one's, have completed." > > The task must be queued on the same task queue as other tasks from the "timer > task source", and tasks on a queue are executed in queue order. Thanks! I really appreciate the detail provided.
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": 1, "attempt_start_ts": 1484705388537070, "parent_rev":
"271c0525253b60dba0d09d8142cf3706217bea6f", "commit_rev":
"c5bbdca617d68284f396f83f6294179cff4d8854"}
Message was sent while issue was closed.
Description was changed from ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Language settings: Wait 100ms to close language action menu on checkbox change Provide feedback to the user by keeping the language detail menu open briefly when the user toggles a checkbox inside the menu. BUG=654964 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2627973008 Cr-Commit-Position: refs/heads/master@{#444254} Committed: https://chromium.googlesource.com/chromium/src/+/c5bbdca617d68284f396f83f6294... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/c5bbdca617d68284f396f83f6294... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
