Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133753002:
🏠Delay the URL focus effects if they happen before NTP is loaded. (Closed)
Description[NTP] Delay the URL focus effects if they happen before NTP is loaded.
When the URL bar is focused before the NTP is loaded, the UI misses
the events and renders in the incorrect state. This change just runs
the effects of the event later.
BUG=617582
Committed: https://crrev.com/d2ea385339e75be9f8d8edcfa600273fd4db7a54
Cr-Commit-Position: refs/heads/master@{#404634}
Patch Set 1 #Patch Set 2 : add comment #
Total comments: 6
Patch Set 3 : use onLayoutChange #
Total comments: 1
Messages
Total messages: 16 (6 generated)
Description was changed from ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 ========== to ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:723: if (((LocationBarLayout) fakeboxDelegate).isUrlBarFocused()) { Can you make this part of the FakeboxDelegate interface? https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1105: * Schedules the URL focus translation to be ran at a later time, when the layout is stable. Nit: "to be run" https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1113: getViewTreeObserver().addOnGlobalLayoutListener(new OnGlobalLayoutListener() { OnGlobalLayoutListener seems like a very big hammer... If we would just set the focus change percentage here directly, would it get overwritten later?
https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1113: getViewTreeObserver().addOnGlobalLayoutListener(new OnGlobalLayoutListener() { On 2016/07/08 15:28:19, Bernhard Bauer wrote: > OnGlobalLayoutListener seems like a very big hammer... > > If we would just set the focus change percentage here directly, would it get > overwritten later? The elements don't have the right size yet. the space has not been distributed, etc. We also unregister the listener right after it runs, so it's not a problem later on.
https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1113: getViewTreeObserver().addOnGlobalLayoutListener(new OnGlobalLayoutListener() { On 2016/07/08 15:30:07, dgn wrote: > On 2016/07/08 15:28:19, Bernhard Bauer wrote: > > OnGlobalLayoutListener seems like a very big hammer... > > > > If we would just set the focus change percentage here directly, would it get > > overwritten later? > > The elements don't have the right size yet. the space has not been distributed, > etc. We also unregister the listener right after it runs, so it's not a problem > later on. I'm not sure whether a *global* layout listener wouldn't be a problem in, say, multiwindow mode; and even if it isn't, it just seems super ugly to me... Couldn't we just rerun onUrlFocusAnimationChanged() if our elements change size?
dgn@chromium.org changed reviewers: + dfalcantara@chromium.org
PTAL https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2133753002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1113: getViewTreeObserver().addOnGlobalLayoutListener(new OnGlobalLayoutListener() { On 2016/07/08 15:35:31, Bernhard Bauer wrote: > On 2016/07/08 15:30:07, dgn wrote: > > On 2016/07/08 15:28:19, Bernhard Bauer wrote: > > > OnGlobalLayoutListener seems like a very big hammer... > > > > > > If we would just set the focus change percentage here directly, would it get > > > overwritten later? > > > > The elements don't have the right size yet. the space has not been > distributed, > > etc. We also unregister the listener right after it runs, so it's not a > problem > > later on. > > I'm not sure whether a *global* layout listener wouldn't be a problem in, say, > multiwindow mode; and even if it isn't, it just seems super ugly to me... > Ah right I expected it to listen only to changes in its view tree. I'll have to look for an exact explanation of what this does. > Couldn't we just rerun onUrlFocusAnimationChanged() if our elements change size? Done. https://codereview.chromium.org/2133753002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2133753002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:812: int oldHeight = oldBottom - oldTop; Using height should achieve the same effect as width to detect orientation changes, but is also useful at the initialization, to run code when the layout is not stable yet Also note: we now look at the dimensions of the above-the-fold, NTPLayout, not NTPView. Not sure if using the width of the NTPView had another purpose. +dfalcantara@ any idea?
LGTM, thanks!
The CQ bit was checked by dgn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 ========== to ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 ========== to ========== [NTP] Delay the URL focus effects if they happen before NTP is loaded. When the URL bar is focused before the NTP is loaded, the UI misses the events and renders in the incorrect state. This change just runs the effects of the event later. BUG=617582 Committed: https://crrev.com/d2ea385339e75be9f8d8edcfa600273fd4db7a54 Cr-Commit-Position: refs/heads/master@{#404634} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d2ea385339e75be9f8d8edcfa600273fd4db7a54 Cr-Commit-Position: refs/heads/master@{#404634} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
