Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1012233002:
Fixed resizing animation for extension options. (Closed)
|
Created:
5 years, 9 months ago by paulmeyer Modified:
5 years, 9 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed resizing animation for extension options.
The problem was that the height of the header (where the extension name and icon are shown) was not being taken into account by the animation of the options overlay.
BUG=468024
Committed: https://crrev.com/e3f634eab3ce973f8a4316e921ade936e5f25ee4
Cr-Commit-Position: refs/heads/master@{#321273}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comment. #
Total comments: 2
Patch Set 3 : Addressed comments. #
Total comments: 2
Patch Set 4 : Addressed comment. #
Total comments: 1
Messages
Total messages: 18 (4 generated)
paulmeyer@chromium.org changed reviewers: + kalman@chromium.org
Thanks for fixing this. It looks like you've found the problem, that we're conflatin the height of the overlay with the height of the overlay's guest. I wonder if my second comment is a more obvious way to fix this? https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:109: var maxHeight = parseInt(overlay.style.maxHeight, 10) - This is funny, I wonder why it's overlay.style.maxHeight rather than overlayStyle.maxHeight? I wonder if it matters? It should probably be overlayStyle.maxHeight. https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:129: // subtracted to get the true old preferred height. And I wonder if this should use the width/height of overlayGuest rather than the width/height of the overlay (where the width is the same of course)?
ptal https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:109: var maxHeight = parseInt(overlay.style.maxHeight, 10) - On 2015/03/18 15:35:20, kalman wrote: > This is funny, I wonder why it's overlay.style.maxHeight rather than > overlayStyle.maxHeight? I wonder if it matters? It should probably be > overlayStyle.maxHeight. Done. https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:129: // subtracted to get the true old preferred height. On 2015/03/18 15:35:20, kalman wrote: > And I wonder if this should use the width/height of overlayGuest rather than the > width/height of the overlay (where the width is the same of course)? I think the reason that overlayGuest isn't used here is because its width and height are undefined the first time this handler is called. Do you still think its worth using them instead here (and initialize them as 0/0 somewhere else ahead of time)?
Continuing conversation. https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:129: // subtracted to get the true old preferred height. On 2015/03/18 17:37:48, Paul Meyer wrote: > On 2015/03/18 15:35:20, kalman wrote: > > And I wonder if this should use the width/height of overlayGuest rather than > the > > width/height of the overlay (where the width is the same of course)? > > I think the reason that overlayGuest isn't used here is because its width and > height are undefined the first time this handler is called. Do you still think > its worth using them instead here (and initialize them as 0/0 somewhere else > ahead of time)? Ah, that could be it. Once again rendering these offscreen then detach/attaching them to render at the initial correct height would solve this, but well outside the scope of this change. Actually - what do they start off as in this case? Is it 0,0? Btw can we rename these to oldGuestWidth and oldGuestHeight?
https://codereview.chromium.org/1012233002/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:140: var newHeight = Math.min(evt.height + 2, maxHeight); Actually - should we be adding overlayHeader.width/height to these, rather than any of the other changes? It seems fine that we're animating the overlay from the old height to the new height. The problem is that the new width/height doesn't take into account the header. (and deleting the +2, that weird resize bug is still there!)
ptal https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_options_overlay.js:129: // subtracted to get the true old preferred height. On 2015/03/18 19:52:06, kalman wrote: > On 2015/03/18 17:37:48, Paul Meyer wrote: > > On 2015/03/18 15:35:20, kalman wrote: > > > And I wonder if this should use the width/height of overlayGuest rather than > > the > > > width/height of the overlay (where the width is the same of course)? > > > > I think the reason that overlayGuest isn't used here is because its width and > > height are undefined the first time this handler is called. Do you still think > > its worth using them instead here (and initialize them as 0/0 somewhere else > > ahead of time)? > > Ah, that could be it. > > Once again rendering these offscreen then detach/attaching them to render at the > initial correct height would solve this, but well outside the scope of this > change. > > Actually - what do they start off as in this case? Is it 0,0? > > Btw can we rename these to oldGuestWidth and oldGuestHeight? Yes, 0,0. I agree, those names are more meaningful. Done. https://codereview.chromium.org/1012233002/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:140: var newHeight = Math.min(evt.height + 2, maxHeight); On 2015/03/18 20:59:15, kalman wrote: > Actually - should we be adding overlayHeader.width/height to these, rather than > any of the other changes? It seems fine that we're animating the overlay from > the old height to the new height. The problem is that the new width/height > doesn't take into account the header. > > (and deleting the +2, that weird resize bug is still there!) Yes, adding the header height to the newheight could work too, but it would then need to be subtracted again later. It's about the same amount of changes, except I think it might make slightly more sense for the old and new sizes to just be the guest sizes, since that's what the preferred size is for. The +2 will be addressed in a separate patch.
Sorry to push here, I thought we were talking about the other approach? https://codereview.chromium.org/1012233002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:157: {width: oldGuestWidth + 'px', I still think it's weird that the overlay is being animated using the overlayGuest's dimensions (as we chatted about) rather than its own. I'd rather rephrase this in terms of the overlay's dimensions i.e. use oldOverlayWidth/oldOverlayHeight, and the only slightly surprising behavior (requiring a comment) would be on line 174 where you e.g. subtract overlayHeader.offsetHeight from newOverlayHeight.
No problem! I misunderstood. ptal. https://codereview.chromium.org/1012233002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:157: {width: oldGuestWidth + 'px', On 2015/03/18 21:47:39, kalman wrote: > I still think it's weird that the overlay is being animated using the > overlayGuest's dimensions (as we chatted about) rather than its own. I'd rather > rephrase this in terms of the overlay's dimensions i.e. use > oldOverlayWidth/oldOverlayHeight, and the only slightly surprising behavior > (requiring a comment) would be on line 174 where you e.g. subtract > overlayHeader.offsetHeight from newOverlayHeight. Okay, done. The other place I thought a comment was warranted is when the header height is added to the new overlay height.
lgtm https://codereview.chromium.org/1012233002/diff/60001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.js (right): https://codereview.chromium.org/1012233002/diff/60001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.js:155: // heights to get the full overlay heights. This comment no longer relevant.
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1012233002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1012233002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e3f634eab3ce973f8a4316e921ade936e5f25ee4 Cr-Commit-Position: refs/heads/master@{#321273} |
