Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861443003:
MD Settings: Fix subpage visibility and add appearance page tests (Closed)
|
Created:
3 years, 7 months ago by stevenjb Modified:
3 years, 7 months 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix subpage visibility and add appearance page tests
BUG=717663
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix default subpage visibility and CrSettingsMainPageTest #Patch Set 3 : Revert to use hidden and improve tests #Patch Set 4 : Rebase #
Total comments: 10
Patch Set 5 : Fix first-of-type selector #Patch Set 6 : Remove cleanup #
Total comments: 2
Patch Set 7 : Restore showPage_ #Messages
Total messages: 32 (14 generated)
Description was changed from ========== MD Settings: Fix subpage visibility and add appearance page tests BUG=717663 ========== to ========== MD Settings: Fix subpage visibility and add appearance page tests BUG=717663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
I forgot to include these changes in the first CL. There were some conflicts and I wanted to added some simple tests, so it took a bit longer than I hoped to get this up. Apologies for the poor diff for appearance_page.html, I couldn't figure out a way to make it better.
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.html:42: <template is="dom-if" if="[[showPage_(pageVisibility.googleDrive)]]"> Is it necessary to switch a div with |hidden| to dom-if (here and elsewhere in this CL, I think I see such cases in appearance_page.js too).
https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.html:42: <template is="dom-if" if="[[showPage_(pageVisibility.googleDrive)]]"> On 2017/05/02 23:32:00, dpapad wrote: > Is it necessary to switch a div with |hidden| to dom-if (here and elsewhere in > this CL, I think I see such cases in appearance_page.js too). Necessary? No. I thought it was preferred? We couldn't use dom-if before because previously pageVisibility might not have been defined.
https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.html:42: <template is="dom-if" if="[[showPage_(pageVisibility.googleDrive)]]"> On 2017/05/02 at 23:36:02, stevenjb wrote: > On 2017/05/02 23:32:00, dpapad wrote: > > Is it necessary to switch a div with |hidden| to dom-if (here and elsewhere in > > this CL, I think I see such cases in appearance_page.js too). > > Necessary? No. I thought it was preferred? > > We couldn't use dom-if before because previously pageVisibility might not have been defined. It is preferable when the dom-if wraps a large DOM subtree or expensive UI components. Wrapping a fairly small subtree (imagine a single div with a dom-if) can actually make things slower as opposed to using |hidden|. Having said that, I have not audited all such changes in this CL, just mentioning the overall principle on hidden vs dom-if I am aware of. I am leaning towards preferring to keep the diff as minimal as possible for this regression fix and consider changing hidden to dom-if as a separate optimization CL if data supports it. Do you think it is too much extra work to revert those in this CL? If not, I am fine with it, but it will make reviewing this CL a bit longer (since there is more to review than just the fix). https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.js (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.js:38: value: function() { Can we follow the approach of the previous CL, where in prod code all elements expect pageVisibility to be passed down from settings-ui? In unit test code (where no settings-ui exists) we explicitly initialize pageVisibility when the element is constructed.
https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.html:42: <template is="dom-if" if="[[showPage_(pageVisibility.googleDrive)]]"> On 2017/05/02 23:45:52, dpapad wrote: > On 2017/05/02 at 23:36:02, stevenjb wrote: > > On 2017/05/02 23:32:00, dpapad wrote: > > > Is it necessary to switch a div with |hidden| to dom-if (here and elsewhere > in > > > this CL, I think I see such cases in appearance_page.js too). > > > > Necessary? No. I thought it was preferred? > > > > We couldn't use dom-if before because previously pageVisibility might not have > been defined. > > It is preferable when the dom-if wraps a large DOM subtree or expensive UI > components. Wrapping a fairly small subtree (imagine a single div with a dom-if) > can actually make things slower as opposed to using |hidden|. > > Having said that, I have not audited all such changes in this CL, just > mentioning the overall principle on hidden vs dom-if I am aware of. > > I am leaning towards preferring to keep the diff as minimal as possible for this > regression fix and consider changing hidden to dom-if as a separate optimization > CL if data supports it. > > Do you think it is too much extra work to revert those in this CL? If not, I am > fine with it, but it will make reviewing this CL a bit longer (since there is > more to review than just the fix). I guess "large" is subjective. I personally prefer consistency over pretty much everything, and some of the sections (e.g. in the appearance page) are large. That said, it's not that big a deal to switch back to 'hidden' everywhere if you prefer. I don't feel strongly either way. https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.js (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.js:38: value: function() { On 2017/05/02 23:45:52, dpapad wrote: > Can we follow the approach of the previous CL, where in prod code all elements > expect pageVisibility to be passed down from settings-ui? In unit test code > (where no settings-ui exists) we explicitly initialize pageVisibility when the > element is constructed. So, if we don't do this, we need to ensure that any subpage that adds a pageVisibilty property needs to also set it to an empty object in settings-ui. I'd prefer not to require that. The reason I changed the other pageVisibility properties to default to undefined is that they are all bound to the one in settings-ui, which is now always defined, so I think that would be more clear. Does that make sense?
https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.js (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.js:38: value: function() { On 2017/05/02 at 23:56:17, stevenjb wrote: > On 2017/05/02 23:45:52, dpapad wrote: > > Can we follow the approach of the previous CL, where in prod code all elements > > expect pageVisibility to be passed down from settings-ui? In unit test code > > (where no settings-ui exists) we explicitly initialize pageVisibility when the > > element is constructed. > > So, if we don't do this, we need to ensure that any subpage that adds a pageVisibilty property needs to also set it to an empty object in settings-ui. I'd prefer not to require that. > > The reason I changed the other pageVisibility properties to default to undefined is that they are all bound to the one in settings-ui, which is now always defined, so I think that would be more clear. > > Does that make sense? Hm, I think it actually made me realize that I should have reviewed the previous CL a bit more thoroughly. I am now seeing a problem at [1] and similar places where we call basic-page's getPageVisibility_. The binding does not depend on this.pageVisibility, which means that the binding triggers only once and never re-triggers if |this.pageVisibility| is set to a different value in basic-page. In other words it seems we are breaking the flow of the top-level settings-ui pageVisibility such that is not guaranteed to reach all children which is not equivalent to what has happening before [2]. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/basic_... [2] https://codereview.chromium.org/2852433003/diff/20001/chrome/browser/resource...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/downloads_page/downloads_page.js (right): https://codereview.chromium.org/2861443003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/downloads_page/downloads_page.js:38: value: function() { On 2017/05/03 00:18:48, dpapad wrote: > On 2017/05/02 at 23:56:17, stevenjb wrote: > > On 2017/05/02 23:45:52, dpapad wrote: > > > Can we follow the approach of the previous CL, where in prod code all > elements > > > expect pageVisibility to be passed down from settings-ui? In unit test code > > > (where no settings-ui exists) we explicitly initialize pageVisibility when > the > > > element is constructed. > > > > So, if we don't do this, we need to ensure that any subpage that adds a > pageVisibilty property needs to also set it to an empty object in settings-ui. > I'd prefer not to require that. > > > > The reason I changed the other pageVisibility properties to default to > undefined is that they are all bound to the one in settings-ui, which is now > always defined, so I think that would be more clear. > > > > Does that make sense? > > Hm, I think it actually made me realize that I should have reviewed the previous > CL a bit more thoroughly. I am now seeing a problem at [1] and similar places > where we call basic-page's getPageVisibility_. > > The binding does not depend on this.pageVisibility, which means that the binding > triggers only once and never re-triggers if |this.pageVisibility| is set to a > different value in basic-page. In other words it seems we are breaking the flow > of the top-level settings-ui pageVisibility such that is not guaranteed to reach > all children which is not equivalent to what has happening before [2]. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/basic_... > [2] > https://codereview.chromium.org/2852433003/diff/20001/chrome/browser/resource... Ah, yes, you are correct, that's wrong, and fixing that eliminates the need to initialize the subpage pageVisibility, so I think all is well.
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Diff from master should be reasonable now.
https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (left): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:154: <div class$="settings-box [[getFirst_(pageVisibility.bookmarksBar)]]"> Isn't this addressing the problem of determining which settings-box should have |first|? I don't think this is being addressed anymore, erroneously. https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:27: settings-box:first-of-type { I don't think this works at all. :first-of-type can only be used with valid HTML Types (div, span etc). It can't even be used with classes, so even .settings-box:first-of-type {...} would do nothing AFAIK. Are you trying to fix a bug that already exists in the code? Meaning that the <div class="settings-box first...> on the left exists only on CrOS, which means that on non-ChromeOS there is no settings-box marked with |first|? https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:52: hidden="[[!showPage_(pageVisibility.setWallpaper)]]"> Isn't this equivalent to what is already on the left? hidden="[[!pageVisibility.setWallpaper]]"> Essentially the showPage_() wrapper only made sense when we were switching from hidden to dom-if. Does it add any value anymore (here and elsewhere)? https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:128: page-visibility="[[getPageVisibility_(pageVisibility, IIUC, only the changes in this file are necessary to fix the regression (missing sections). All other changes (except for the tests), can be reverted, no?
https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (left): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:154: <div class$="settings-box [[getFirst_(pageVisibility.bookmarksBar)]]"> On 2017/05/03 23:02:24, dpapad wrote: > Isn't this addressing the problem of determining which settings-box should have > |first|? I don't think this is being addressed anymore, erroneously. The new CSS does the right thing now, always, regardless of which sections we decide to hide, and with better readability. I *hate* this pattern, especially the mess around line 60. https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:27: settings-box:first-of-type { On 2017/05/03 23:02:24, dpapad wrote: > I don't think this works at all. :first-of-type can only be used with valid HTML > Types (div, span etc). It can't even be used with classes, so even > > .settings-box:first-of-type {...} would do nothing AFAIK. > > > Are you trying to fix a bug that already exists in the code? Meaning that the > <div class="settings-box first...> on the left exists only on CrOS, which means > that on non-ChromeOS there is no settings-box marked with |first|? Ugh, you're right. It's super subtle when at the top of a page/section so I didn't notice. It's supposed to be .settings-box:first-of-type, which does work and which we use elsewhere. :first-of-type "represents the first sibling of its type in the list of children of its parent element". So in this case, for anything matching .settings-box (which will be a div), if it is the first div of its siblings we apply the styling (which is exactly what we want). Did you take a look at the <if> hackery that this had before? It was super challenging to parse, and could still be wrong if we changed what we hide e.g. in Guest mode. https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:52: hidden="[[!showPage_(pageVisibility.setWallpaper)]]"> On 2017/05/03 23:02:24, dpapad wrote: > Isn't this equivalent to what is already on the left? > > hidden="[[!pageVisibility.setWallpaper]]"> > > Essentially the showPage_() wrapper only made sense when we were switching from > hidden to dom-if. Does it add any value anymore (here and elsewhere)? The change makes this more robust and, imho, more readable. I really didn't like that using hidden *happens* to work when the property was undefined. It requires a pretty deep understanding of Polymer to recognize that when setWallpaper is 'undefined' (which is usually falsy) hidden never gets set and therefore does not get initialized to false. (The first time I looked at this code I was surprised by this. and theoretically I have a pretty decent understanding of Polymer at this point...) Plus now if someone comes along and switches this to use dom-if, it will still work. https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:128: page-visibility="[[getPageVisibility_(pageVisibility, On 2017/05/03 23:02:25, dpapad wrote: > IIUC, only the changes in this file are necessary to fix the regression (missing > sections). All other changes (except for the tests), can be reverted, no? Theoretically yes, but I really don't want to abandon those changes.
https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:27: settings-box:first-of-type { On 2017/05/03 23:31:43, stevenjb wrote: > On 2017/05/03 23:02:24, dpapad wrote: > > I don't think this works at all. :first-of-type can only be used with valid > HTML > > Types (div, span etc). It can't even be used with classes, so even > > > > .settings-box:first-of-type {...} would do nothing AFAIK. > > > > > > Are you trying to fix a bug that already exists in the code? Meaning that the > > <div class="settings-box first...> on the left exists only on CrOS, which > means > > that on non-ChromeOS there is no settings-box marked with |first|? > > Ugh, you're right. It's super subtle when at the top of a page/section so I > didn't notice. > > It's supposed to be .settings-box:first-of-type, which does work and which we > use elsewhere. :first-of-type "represents the first sibling of its type in the > list of children of its parent element". So in this case, for anything matching > .settings-box (which will be a div), if it is the first div of its siblings we > apply the styling (which is exactly what we want). > > Did you take a look at the <if> hackery that this had before? It was super > challenging to parse, and could still be wrong if we changed what we hide e.g. > in Guest mode. > > > Oh, ugh, I need to check [hidden] also... one moment...
https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2861443003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:27: settings-box:first-of-type { On 2017/05/03 23:33:16, stevenjb wrote: > On 2017/05/03 23:31:43, stevenjb wrote: > > On 2017/05/03 23:02:24, dpapad wrote: > > > I don't think this works at all. :first-of-type can only be used with valid > > HTML > > > Types (div, span etc). It can't even be used with classes, so even > > > > > > .settings-box:first-of-type {...} would do nothing AFAIK. > > > > > > > > > Are you trying to fix a bug that already exists in the code? Meaning that > the > > > <div class="settings-box first...> on the left exists only on CrOS, which > > means > > > that on non-ChromeOS there is no settings-box marked with |first|? > > > > Ugh, you're right. It's super subtle when at the top of a page/section so I > > didn't notice. > > > > It's supposed to be .settings-box:first-of-type, which does work and which we > > use elsewhere. :first-of-type "represents the first sibling of its type in the > > list of children of its parent element". So in this case, for anything > matching > > .settings-box (which will be a div), if it is the first div of its siblings we > > apply the styling (which is exactly what we want). > > > > Did you take a look at the <if> hackery that this had before? It was super > > challenging to parse, and could still be wrong if we changed what we hide e.g. > > in Guest mode. > > > > > > > > Oh, ugh, I need to check [hidden] also... one moment... aaaaand this only works with dom-if, right. Which is why I switched to dom-if. Sigh. Guess I'll just upload the minimal patch to fix this, I'm sick of it.
PTAL
https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/basic_page/basic_page.js:150: return /** @type {Object} */ (this.get(subpage, visibility)) || {}; I tried the CL locally and I still see the sections hidden as in the bug report. Played with it locally and it seems that this line needs to be as follows for it to work: return /** @type {Object} */ (this.get(subpage, visibility)); Essentially, now that we are using |hidden|, we should not be sending an empty object to the the other pages (like appearance), because bindings like hidden="[[!pageVisibility.homeButton]]" evaluate to true, if pageVisibility is an empty object. In retrospect, I think in the original CL we should 1) not have changed the default value of settings-ui's pageVisibility 2) have kept pageVisibility only used for "guestMode" and not bundle the "show-android-apps" functionality into it (which is the reason we changed the default value). I hope this makes sense (just trying to help, given the complications that were created by the original CL, which I should have been more thoroughly reviewing.). https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/appearance_page_test.js (right): https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/appearance_page_test.js:145: assertTrue(!!appearancePage.$$('#homeButton')); Now that we don't use dom-if, these tests are insufficient (they check existence which is always true, and therefore did not catch the problem mentioned in previous comment).
On 2017/05/04 01:03:24, dpapad wrote: > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/settings/basic_page/basic_page.js (right): > > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/basic_page/basic_page.js:150: return /** @type > {Object} */ (this.get(subpage, visibility)) || {}; > I tried the CL locally and I still see the sections hidden as in the bug report. > Played with it locally and it seems that this line needs to be as follows for it > to work: > > return /** @type {Object} */ (this.get(subpage, visibility)); > > Essentially, now that we are using |hidden|, we should not be sending an empty > object to the the other pages (like appearance), because bindings like > hidden="[[!pageVisibility.homeButton]]" > > evaluate to true, if pageVisibility is an empty object. > > In retrospect, I think in the original CL we should > 1) not have changed the default value of settings-ui's pageVisibility > 2) have kept pageVisibility only used for "guestMode" and not bundle the > "show-android-apps" functionality into it (which is the reason we changed the > default value). > > I hope this makes sense (just trying to help, given the complications that were > created by the original CL, which I should have been more thoroughly > reviewing.). > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > File chrome/test/data/webui/settings/appearance_page_test.js (right): > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > chrome/test/data/webui/settings/appearance_page_test.js:145: > assertTrue(!!appearancePage.$$('#homeButton')); > Now that we don't use dom-if, these tests are insufficient (they check existence > which is always true, and therefore did not catch the problem mentioned in > previous comment). Good grief, have I mentioned recently how much I despise Javascript? I was in too much of a rush last night. I actually had tests for hidden but didn't save/test that version, and clearly didn't manually test it. I added back showPage_ which IMHO is the better approach when we are using undefined == visible == truthy. If you are unhappy with this approach I can revert my original "cleanup" CL and we can let this code rot.
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
PTAL & LMKWYT
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/04 15:41:28, stevenjb wrote: > On 2017/05/04 01:03:24, dpapad wrote: > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > > File chrome/browser/resources/settings/basic_page/basic_page.js (right): > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > > chrome/browser/resources/settings/basic_page/basic_page.js:150: return /** > @type > > {Object} */ (this.get(subpage, visibility)) || {}; > > I tried the CL locally and I still see the sections hidden as in the bug > report. > > Played with it locally and it seems that this line needs to be as follows for > it > > to work: > > > > return /** @type {Object} */ (this.get(subpage, visibility)); > > > > Essentially, now that we are using |hidden|, we should not be sending an empty > > object to the the other pages (like appearance), because bindings like > > hidden="[[!pageVisibility.homeButton]]" > > > > evaluate to true, if pageVisibility is an empty object. > > > > In retrospect, I think in the original CL we should > > 1) not have changed the default value of settings-ui's pageVisibility > > 2) have kept pageVisibility only used for "guestMode" and not bundle the > > "show-android-apps" functionality into it (which is the reason we changed the > > default value). > > > > I hope this makes sense (just trying to help, given the complications that > were > > created by the original CL, which I should have been more thoroughly > > reviewing.). > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > > File chrome/test/data/webui/settings/appearance_page_test.js (right): > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > > chrome/test/data/webui/settings/appearance_page_test.js:145: > > assertTrue(!!appearancePage.$$('#homeButton')); > > Now that we don't use dom-if, these tests are insufficient (they check > existence > > which is always true, and therefore did not catch the problem mentioned in > > previous comment). > > Good grief, have I mentioned recently how much I despise Javascript? > > I was in too much of a rush last night. I actually had tests for hidden but > didn't save/test that version, and clearly didn't manually test it. > > I added back showPage_ which IMHO is the better approach when we are using > undefined == visible == truthy. > > If you are unhappy with this approach I can revert my original "cleanup" CL and > we can let this code rot. i'd be happy to revert the original CL that caused these breaks
On 2017/05/04 19:13:10, Dan Beam wrote: > On 2017/05/04 15:41:28, stevenjb wrote: > > On 2017/05/04 01:03:24, dpapad wrote: > > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > > > File chrome/browser/resources/settings/basic_page/basic_page.js (right): > > > > > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/browser/resourc... > > > chrome/browser/resources/settings/basic_page/basic_page.js:150: return /** > > @type > > > {Object} */ (this.get(subpage, visibility)) || {}; > > > I tried the CL locally and I still see the sections hidden as in the bug > > report. > > > Played with it locally and it seems that this line needs to be as follows > for > > it > > > to work: > > > > > > return /** @type {Object} */ (this.get(subpage, visibility)); > > > > > > Essentially, now that we are using |hidden|, we should not be sending an > empty > > > object to the the other pages (like appearance), because bindings like > > > hidden="[[!pageVisibility.homeButton]]" > > > > > > evaluate to true, if pageVisibility is an empty object. > > > > > > In retrospect, I think in the original CL we should > > > 1) not have changed the default value of settings-ui's pageVisibility > > > 2) have kept pageVisibility only used for "guestMode" and not bundle the > > > "show-android-apps" functionality into it (which is the reason we changed > the > > > default value). > > > > > > I hope this makes sense (just trying to help, given the complications that > > were > > > created by the original CL, which I should have been more thoroughly > > > reviewing.). > > > > > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > > > File chrome/test/data/webui/settings/appearance_page_test.js (right): > > > > > > > > > https://codereview.chromium.org/2861443003/diff/100001/chrome/test/data/webui... > > > chrome/test/data/webui/settings/appearance_page_test.js:145: > > > assertTrue(!!appearancePage.$$('#homeButton')); > > > Now that we don't use dom-if, these tests are insufficient (they check > > existence > > > which is always true, and therefore did not catch the problem mentioned in > > > previous comment). > > > > Good grief, have I mentioned recently how much I despise Javascript? > > > > I was in too much of a rush last night. I actually had tests for hidden but > > didn't save/test that version, and clearly didn't manually test it. > > > > I added back showPage_ which IMHO is the better approach when we are using > > undefined == visible == truthy. > > > > If you are unhappy with this approach I can revert my original "cleanup" CL > and > > we can let this code rot. > > i'd be happy to revert the original CL that caused these breaks ah, just saw this was already done here https://codereview.chromium.org/2866463002/ |
