[iOS][Android] fix scrollToElement performance incorrect offset #2198
Conversation
win80540
requested review from
Darin726,
gubaojian,
wqyfavor and
YorkShen
as code owners
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
I think code change in WXScroller.initComponentHostView may cause potential problem.
Your PR removes the code of @lucky-chen , you might as well contact him.
| scrollView.post(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| if (mIslastDirectionRTL != null && isLayoutRTL() != mIslastDirectionRTL.booleanValue()) { |
There was a problem hiding this comment.
As mIslastDirectionRTL is Boolean, which could cause null pointer exception here,
Why don't use boolean mIslastDirectionRTL directly?
There was a problem hiding this comment.
if use boolean, default is false, is not what i want.
I need record direction when last layout change occur, it not always false, and i need distinct is layout change trigger first or not.
| int width = right - left; | ||
| int changedWidth = width - oldWidth; | ||
| if (changedWidth != 0) { | ||
| scrollView.scrollBy(changedWidth, component.getScrollY()); |
There was a problem hiding this comment.
You are missing an else case, which could be a problem here.
There was a problem hiding this comment.
If changedWidth == 0, we should do nothing
| scrollView.scrollTo(mw, component.getScrollY()); | ||
| } else { | ||
| boolean scrollToBegin = true; | ||
| Object scrollToBeginOnLTR = getAttrs().get("scrollToBegin"); |
There was a problem hiding this comment.
ScrollBegin is added in #2165
Is there an reason to remove?
There was a problem hiding this comment.
Because it do not scroll to begin anymore.
| int totalWidth = getInnerView().getChildAt(0).getWidth(); | ||
| int displayWidth = getInnerView().getMeasuredWidth(); | ||
| viewXInScroller = totalWidth - (component.getAbsoluteX() - getAbsoluteX()) - displayWidth; | ||
| if (component.getParent() != null && component.getParent() == this) { |
There was a problem hiding this comment.
Any reason to add this if?
There was a problem hiding this comment.
If hit this if, it mean's scroll to indirect child element, then the offsetX calculate will different from direct child element
Since i start use Rax UI Framework - Nuke, i found i can't scroll to correct element on RTL layout.
Because Nuke will add a container div between scroller and scroller's child element.
ScrollToElement only can scroll to the direct child element on RTL direction in the online version，
now i fix the problem，scrollToElement can scroll to indirect child element.
BTW. i found scroll to begin let front-end engineer confused, now i found a better way to key offset when layout direction changed.
Nuke DEMO： https://jsplayground.taobao.org/raxplayground/5bf4b469-8eb1-4f5d-91df-912cd085ab7b
Direction chang DEMO：http://dotwe.org/vue/09cb6e35412f96c25c423fd50a929597