Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
275483005:
Fix initial focus, and the way elements are disabled on the 'Clear browsing data' dialog. (Closed)
|
Created:
6 years, 7 months ago by engedy Modified:
6 years, 6 months ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix initial focus, and the way elements are disabled on the 'Clear browsing data' dialog.
* Fix a regression so that once again the time range selector combo-box is in focus initially.
* Clean up the code that determines which controls should be enabled/disabled/hidden on the dialog.
* Reduce the number of controls that flicker (i.e. change state from disabled->enabled visibly) on opening the dialog in the most likely use-case, i.e., instead of starting with everything disabled, only have the commit button initially disabled.
* Fix a regression so that once again some checkboxes can be disabled by policies. (I think this was also broken.)
BUG=365206
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274090
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Addressed comments. #
Total comments: 12
Patch Set 3 : Fix typo. #Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Fixed OR condition, added test. #
Total comments: 8
Patch Set 6 : Addressed final round of comments. #
Messages
Total messages: 20 (0 generated)
@Dan, please take a look. This is fixing an M36 release blocker, plus some flickering I have discovered during working on the fix for the former.
On 2014/05/13 17:56:13, engedy wrote: > @Dan, please take a look. > > This is fixing an M36 release blocker, plus some flickering I have discovered > during working on the fix for the former. can you split the part of the fix that will be merged out separately?
On 2014/05/13 22:11:58, Dan Beam wrote: > On 2014/05/13 17:56:13, engedy wrote: > > @Dan, please take a look. > > > > This is fixing an M36 release blocker, plus some flickering I have discovered > > during working on the fix for the former. > > can you split the part of the fix that will be merged out separately? Sorry, what I wanted to say is that I think the additional 2 issues are actually more severe than the original one; and the release should not go out with those unfixed. Given that we are still so close to branch point, I was thinking about fixing them properly and merging a bigger change. The only other real alternative is reverting my previous CL that lead to this, but then we will have another bug again.
On 2014/05/13 22:53:31, engedy wrote: > On 2014/05/13 22:11:58, Dan Beam wrote: > > On 2014/05/13 17:56:13, engedy wrote: > > > @Dan, please take a look. > > > > > > This is fixing an M36 release blocker, plus some flickering I have > discovered > > > during working on the fix for the former. > > > > can you split the part of the fix that will be merged out separately? > > Sorry, what I wanted to say is that I think the additional 2 issues are actually > more severe than the original one; and the release should not go out with those > unfixed. Given that we are still so close to branch point, I was thinking about > fixing them properly and merging a bigger change. > > The only other real alternative is reverting my previous CL that lead to this, > but then we will have another bug again. @Dan, as per our agreement with the release managers, the CL causing all these issues was reverted on the M36 branch. On trunk, however, it was not reverted, but instead, this CL will be landed as a follow-up to address the regression. Could you please proceed with the review?
looks pretty good... https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:42: isClearingInProgress_: null, overall I think it's pretty dubious to have a variable that could be null or false... https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:86: } can this be in |updateStateOfControls_|? https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:157: setAllowDeletingHistory: function(allowed) { nit: setAllowDeletingHistory_ + @private if only used in |ClearBrowserDataOverlay.setAllowDeletingHistory| https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:163: * Updates the enabled/disabled/hidden status of all controls on the dialog. @private https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:167: // cleared, and if we are not already in the process of clearing. can we do the |isClearingInProgress_| check first? https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:230: ClearBrowserDataOverlay.getInstance().setClearing(false); nit: ClearBrowserDataOverlay.setClearing(false);
One more question: given that I am proposing to have a single monolithic update methods: there is no signficant performance implication in setting an already disabled control disabled again, right? https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:42: isClearingInProgress_: null, On 2014/05/19 17:39:07, Dan Beam wrote: > overall I think it's pretty dubious to have a variable that could be null or > false... I have introduces a third boolean that indicates whether or not we have already finished initializing. WDYT? https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:86: } On 2014/05/19 17:39:07, Dan Beam wrote: > can this be in |updateStateOfControls_|? I would prefer keeping this separate. This is a load time, permanent hiding of these controls, which is quite different than how updateStateOfControls_() operates. https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:157: setAllowDeletingHistory: function(allowed) { On 2014/05/19 17:39:07, Dan Beam wrote: > nit: setAllowDeletingHistory_ + @private if only used in > |ClearBrowserDataOverlay.setAllowDeletingHistory| Done. https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:163: * Updates the enabled/disabled/hidden status of all controls on the dialog. On 2014/05/19 17:39:07, Dan Beam wrote: > @private Done. https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:167: // cleared, and if we are not already in the process of clearing. On 2014/05/19 17:39:07, Dan Beam wrote: > can we do the |isClearingInProgress_| check first? Done. https://codereview.chromium.org/275483005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:230: ClearBrowserDataOverlay.getInstance().setClearing(false); On 2014/05/19 17:39:07, Dan Beam wrote: > nit: ClearBrowserDataOverlay.setClearing(false); Done.
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:190: if (this.isClearingInProgress_ || !this.isInitializationComplete) { isInitializationComplete -> isInitializationComplete_ https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; just remove isCheck and inline $('clear-browser-data-commit').disabled = true; here, IMO https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:209: var enabled = this.allowDeletingHistory_ && !this.isClearingInProgress_; don't we need to check isInitializationComplete_ here?
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; On 2014/05/19 21:41:16, Dan Beam wrote: > just remove isCheck and inline > > $('clear-browser-data-commit').disabled = true; > > here, IMO isChecked**
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:190: if (this.isClearingInProgress_ || !this.isInitializationComplete) { On 2014/05/19 21:41:16, Dan Beam wrote: > isInitializationComplete -> isInitializationComplete_ Done. Thanks, sorry, working through SSH... https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; On 2014/05/19 21:41:34, Dan Beam wrote: > On 2014/05/19 21:41:16, Dan Beam wrote: > > just remove isCheck and inline > > > > $('clear-browser-data-commit').disabled = true; > > > > here, IMO > > isChecked** In that case we would need a "disabled = false" call in the beginning. Are visual styles possibly multiple times *during* running the JS, or only once afterwards? (I do not want to introduce flickering by this.) https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:209: var enabled = this.allowDeletingHistory_ && !this.isClearingInProgress_; On 2014/05/19 21:41:16, Dan Beam wrote: > don't we need to check isInitializationComplete_ here? To avoid all the controls becoming disabled then enabled when the dialog is first shown (looks really ugly), I would just leave them enabled in the primary use case. There is no harm even if the user can toggle their state. Should I add more detail to the comment above?
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; ... Are visual styles *applied* possibly... On 2014/05/19 23:19:22, engedy wrote: > On 2014/05/19 21:41:34, Dan Beam wrote: > > On 2014/05/19 21:41:16, Dan Beam wrote: > > > just remove isCheck and inline > > > > > > $('clear-browser-data-commit').disabled = true; > > > > > > here, IMO > > > > isChecked** > > In that case we would need a "disabled = false" call in the beginning. Are > visual styles possibly multiple times *during* running the JS, or only once > afterwards? (I do not want to introduce flickering by this.)
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; On 2014/05/19 23:20:20, engedy wrote: > ... Are visual styles *applied* possibly... > > On 2014/05/19 23:19:22, engedy wrote: > > On 2014/05/19 21:41:34, Dan Beam wrote: > > > On 2014/05/19 21:41:16, Dan Beam wrote: > > > > just remove isCheck and inline > > > > > > > > $('clear-browser-data-commit').disabled = true; > > > > > > > > here, IMO > > > > > > isChecked** > > > > In that case we would need a "disabled = false" call in the beginning. Are > > visual styles possibly multiple times *during* running the JS, or only once > > afterwards? (I do not want to introduce flickering by this.) > that seems highly unlikely but here's an alternative: var enabled = this.isInitializationComplete_ && !this.isClearingInProgress_; if (enabled) { var checkboxes = document.querySelectorAll( '#cbd-content-area input[type=checkbox]'); for (var i = 0; i < checkboxes.length; i++) { if (checkboxes[i].checked) break; } // Disable the checkbox if there's nothing to clear. enabled = i == checkboxes.length; } $('clear-browser-data-commit').disabled = !enabled; https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:209: var enabled = this.allowDeletingHistory_ && !this.isClearingInProgress_; On 2014/05/19 23:19:22, engedy wrote: > On 2014/05/19 21:41:16, Dan Beam wrote: > > don't we need to check isInitializationComplete_ here? > > To avoid all the controls becoming disabled then enabled when the dialog is > first shown (looks really ugly), I would just leave them enabled in the primary > use case. There is no harm even if the user can toggle their state. > > Should I add more detail to the comment above? can we make them readonly instead of disabled in this case?
https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:198: isChecked = true; On 2014/05/20 00:07:49, Dan Beam wrote: > On 2014/05/19 23:20:20, engedy wrote: > > ... Are visual styles *applied* possibly... > > > > On 2014/05/19 23:19:22, engedy wrote: > > > On 2014/05/19 21:41:34, Dan Beam wrote: > > > > On 2014/05/19 21:41:16, Dan Beam wrote: > > > > > just remove isCheck and inline > > > > > > > > > > $('clear-browser-data-commit').disabled = true; > > > > > > > > > > here, IMO > > > > > > > > isChecked** > > > > > > In that case we would need a "disabled = false" call in the beginning. Are > > > visual styles possibly multiple times *during* running the JS, or only once > > > afterwards? (I do not want to introduce flickering by this.) > > > > that seems highly unlikely but here's an alternative: > > var enabled = this.isInitializationComplete_ && !this.isClearingInProgress_; > if (enabled) { > var checkboxes = document.querySelectorAll( > '#cbd-content-area input[type=checkbox]'); > for (var i = 0; i < checkboxes.length; i++) { > if (checkboxes[i].checked) > break; > } > // Disable the checkbox if there's nothing to clear. > enabled = i == checkboxes.length; > } > $('clear-browser-data-commit').disabled = !enabled; Done something along these lines. https://codereview.chromium.org/275483005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/clear_browser_data_overlay.js:209: var enabled = this.allowDeletingHistory_ && !this.isClearingInProgress_; On 2014/05/20 00:07:49, Dan Beam wrote: > On 2014/05/19 23:19:22, engedy wrote: > > On 2014/05/19 21:41:16, Dan Beam wrote: > > > don't we need to check isInitializationComplete_ here? > > > > To avoid all the controls becoming disabled then enabled when the dialog is > > first shown (looks really ugly), I would just leave them enabled in the > primary > > use case. There is no harm even if the user can toggle their state. > > > > Should I add more detail to the comment above? > > can we make them readonly instead of disabled in this case? As discussed off-line, there is no risk in leaving them enabled for the initial transient period.
https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:195: var isChecked = false; unused https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:198: enabled = false; how does this make sense? if there's data to clear (e.g. a checkbox is checked), *disable* the commit button? can you write some tests for this?
On 2014/05/20 01:16:24, Dan Beam wrote: > https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... > File chrome/browser/resources/options/clear_browser_data_overlay.js (right): > > https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... > chrome/browser/resources/options/clear_browser_data_overlay.js:195: var > isChecked = false; > unused > > https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... > chrome/browser/resources/options/clear_browser_data_overlay.js:198: enabled = > false; > how does this make sense? if there's data to clear (e.g. a checkbox is > checked), *disable* the commit button? can you write some tests for this? Sorry, that is complete nonsense on my part -- I better hold off on this CL until I get back to my home office next week and can actually use a workstation :-).
Please take another look. https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:195: var isChecked = false; On 2014/05/20 01:16:24, Dan Beam wrote: > unused Done. https://codereview.chromium.org/275483005/diff/120001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:198: enabled = false; On 2014/05/20 01:16:24, Dan Beam wrote: > how does this make sense? if there's data to clear (e.g. a checkbox is > checked), *disable* the commit button? can you write some tests for this? Done.
lgtm w/nits https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:40: * Whether or not the WebUI handler have completed initialization. s/have/has/ https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:191: this.isInitializationComplete_; I think this is more sane: var enabled = false; if (this.isInitializationComplete_ && !this.isClearingInProgress_) { var checkboxes = document.querySelectorAll( '#cbd-content-area input[type=checkbox]'); for (var i = 0; i < checkboxes.length; i++) { if (checkboxes[i].checked) { enabled = true; break; } } } https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc (right): https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc:97: for (size_t i = 0; i < arraysize(kDataTypes); ++i) nit: curlies https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc:98: prefs->SetBoolean(kDataTypes[i], false); nit: \n
https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.js (right): https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:40: * Whether or not the WebUI handler have completed initialization. On 2014/05/30 21:11:07, Dan Beam wrote: > s/have/has/ Done. https://codereview.chromium.org/275483005/diff/160001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.js:191: this.isInitializationComplete_; On 2014/05/30 21:11:07, Dan Beam wrote: > I think this is more sane: > > var enabled = false; > if (this.isInitializationComplete_ && !this.isClearingInProgress_) { > var checkboxes = document.querySelectorAll( > '#cbd-content-area input[type=checkbox]'); > for (var i = 0; i < checkboxes.length; i++) { > if (checkboxes[i].checked) { > enabled = true; > break; > } > } > } Done. https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc (right): https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc:97: for (size_t i = 0; i < arraysize(kDataTypes); ++i) On 2014/05/30 21:11:07, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/275483005/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc:98: prefs->SetBoolean(kDataTypes[i], false); On 2014/05/30 21:11:07, Dan Beam wrote: > nit: \n Done.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/engedy@chromium.org/275483005/180001
Message was sent while issue was closed.
Change committed as 274090 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
