From fb9a7be2bdba0ccc8b10806a247f8989dc343e04 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Fri, 20 Mar 2020 18:43:54 -0700 Subject: [PATCH] Fix TextInput left/right padding Summary: This fixes two things: 1) Currently it only respects Start and End padding, and if there's a Theme default, it will override Left/Right padding. Whoops. 2) Currently it doesn't respect when a TextInput starts with padding, but then is removed. This resolves both. It still does not account for RTL support. Changelog: [Internal] Fix AndroidTextInput padding Reviewed By: mdvacca Differential Revision: D20573151 fbshipit-source-id: e89791641b6699e728cde9dbd661a8c21485fbc8 --- .../AndroidTextInputComponentDescriptor.h | 39 +++++++- .../AndroidTextInputProps.cpp | 94 +++++++++++++------ .../androidtextinput/AndroidTextInputProps.h | 3 + 3 files changed, 103 insertions(+), 33 deletions(-) diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h index 318955cb1eb..0b083e5f619 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h @@ -107,27 +107,56 @@ class AndroidTextInputComponentDescriptor final // Override padding // Node is still unsealed during adoption, before layout is complete // TODO: T62959168 account for RTL and paddingLeft when setting default - // paddingStart, and vice-versa with paddingRight/paddingEnd + // paddingStart, and vice-versa with paddingRight/paddingEnd. + // For now this assumes no RTL. YGStyle::Edges result = textInputShadowNode->getConcreteProps().yogaStyle.padding(); bool changedPadding = false; - if (!textInputShadowNode->getConcreteProps().hasPaddingStart) { + if (!textInputShadowNode->getConcreteProps().hasPadding && + !textInputShadowNode->getConcreteProps().hasPaddingStart && + !textInputShadowNode->getConcreteProps().hasPaddingLeft && + !textInputShadowNode->getConcreteProps().hasPaddingHorizontal) { changedPadding = true; result[YGEdgeStart] = theme[YGEdgeStart]; } - if (!textInputShadowNode->getConcreteProps().hasPaddingEnd) { + if (!textInputShadowNode->getConcreteProps().hasPadding && + !textInputShadowNode->getConcreteProps().hasPaddingEnd && + !textInputShadowNode->getConcreteProps().hasPaddingRight && + !textInputShadowNode->getConcreteProps().hasPaddingHorizontal) { changedPadding = true; result[YGEdgeEnd] = theme[YGEdgeEnd]; } - if (!textInputShadowNode->getConcreteProps().hasPaddingTop) { + if (!textInputShadowNode->getConcreteProps().hasPadding && + !textInputShadowNode->getConcreteProps().hasPaddingTop && + !textInputShadowNode->getConcreteProps().hasPaddingVertical) { changedPadding = true; result[YGEdgeTop] = theme[YGEdgeTop]; } - if (!textInputShadowNode->getConcreteProps().hasPaddingBottom) { + if (!textInputShadowNode->getConcreteProps().hasPadding && + !textInputShadowNode->getConcreteProps().hasPaddingBottom && + !textInputShadowNode->getConcreteProps().hasPaddingVertical) { changedPadding = true; result[YGEdgeBottom] = theme[YGEdgeBottom]; } + // If the TextInput initially does not have paddingLeft or paddingStart, a + // paddingStart may be set from the theme. If that happens, when there's a + // paddingLeft update, we must explicitly unset paddingStart... (same with + // paddingEnd) + // TODO: support RTL + if ((textInputShadowNode->getConcreteProps().hasPadding || + textInputShadowNode->getConcreteProps().hasPaddingLeft || + textInputShadowNode->getConcreteProps().hasPaddingHorizontal) && + !textInputShadowNode->getConcreteProps().hasPaddingStart) { + result[YGEdgeStart] = YGValueUndefined; + } + if ((textInputShadowNode->getConcreteProps().hasPadding || + textInputShadowNode->getConcreteProps().hasPaddingRight || + textInputShadowNode->getConcreteProps().hasPaddingHorizontal) && + !textInputShadowNode->getConcreteProps().hasPaddingEnd) { + result[YGEdgeEnd] = YGValueUndefined; + } + // Note that this is expensive: on every adopt, we need to set the Yoga // props again, which normally only happens during prop parsing. Every // commit, state update, etc, will incur this cost. diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp index 78f933af107..670bbffa36e 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp @@ -13,6 +13,29 @@ namespace facebook { namespace react { +static bool hasValue( + const RawProps &rawProps, + bool defaultValue, + const char *name, + const char *prefix, + const char *suffix) { + auto rawValue = rawProps.at(name, prefix, suffix); + + // No change to prop - use default + if (rawValue == nullptr) { + return defaultValue; + } + + // Value passed from JS + if (rawValue->hasValue()) { + return true; + } + + // Null/undefined passed in, indicating that we should use the default + // platform value - thereby resetting this + return false; +} + AndroidTextInputProps::AndroidTextInputProps( const AndroidTextInputProps &sourceProps, const RawProps &rawProps) @@ -225,36 +248,48 @@ AndroidTextInputProps::AndroidTextInputProps( convertRawProp(rawProps, sourceProps.paragraphAttributes, {})), // See AndroidTextInputComponentDescriptor for usage // TODO T63008435: can these, and this feature, be removed entirely? - hasPaddingLeft( - sourceProps.hasPaddingLeft || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("Left", "padding", "") != nullptr || - rawProps.at("Horizontal", "padding", "") != nullptr), + hasPadding(hasValue(rawProps, sourceProps.hasPadding, "", "padding", "")), + hasPaddingHorizontal(hasValue( + rawProps, + sourceProps.hasPaddingHorizontal, + "Horizontal", + "padding", + "")), + hasPaddingVertical(hasValue( + rawProps, + sourceProps.hasPaddingVertical, + "Vertical", + "padding", + "")), + hasPaddingLeft(hasValue( + rawProps, + sourceProps.hasPaddingLeft, + "Left", + "padding", + "")), hasPaddingTop( - sourceProps.hasPaddingTop || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("Top", "padding", "") != nullptr || - rawProps.at("Vertical", "padding", "") != nullptr), - hasPaddingRight( - sourceProps.hasPaddingRight || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("Right", "padding", "") != nullptr || - rawProps.at("Horizontal", "padding", "") != nullptr), - hasPaddingBottom( - sourceProps.hasPaddingBottom || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("Bottom", "padding", "") != nullptr || - rawProps.at("Vertical", "padding", "") != nullptr), - hasPaddingStart( - sourceProps.hasPaddingStart || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("Start", "padding", "") != nullptr || - rawProps.at("Horizontal", "padding", "") != nullptr), + hasValue(rawProps, sourceProps.hasPaddingTop, "Top", "padding", "")), + hasPaddingRight(hasValue( + rawProps, + sourceProps.hasPaddingRight, + "Right", + "padding", + "")), + hasPaddingBottom(hasValue( + rawProps, + sourceProps.hasPaddingBottom, + "Bottom", + "padding", + "")), + hasPaddingStart(hasValue( + rawProps, + sourceProps.hasPaddingStart, + "Start", + "padding", + "")), hasPaddingEnd( - sourceProps.hasPaddingEnd || - rawProps.at("", "padding", "") != nullptr || - rawProps.at("End", "padding", "") != nullptr || - rawProps.at("Horizontal", "padding", "") != nullptr) {} + hasValue(rawProps, sourceProps.hasPaddingEnd, "End", "padding", "")) { +} // TODO T53300085: support this in codegen; this was hand-written folly::dynamic AndroidTextInputProps::getDynamic() const { @@ -309,6 +344,9 @@ folly::dynamic AndroidTextInputProps::getDynamic() const { props["mostRecentEventCount"] = mostRecentEventCount; props["text"] = text; + props["hasPadding"] = hasPadding; + props["hasPaddingHorizontal"] = hasPaddingHorizontal; + props["hasPaddingVertical"] = hasPaddingVertical; props["hasPaddingStart"] = hasPaddingStart; props["hasPaddingEnd"] = hasPaddingEnd; props["hasPaddingLeft"] = hasPaddingLeft; diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.h b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.h index 68430021892..acd185b0d85 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.h +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.h @@ -164,6 +164,9 @@ class AndroidTextInputProps final : public ViewProps, public BaseTextProps { * See AndroidTextInputComponentDescriptor for usage. * TODO T63008435: can these, and this feature, be removed entirely? */ + const bool hasPadding{}; + const bool hasPaddingHorizontal{}; + const bool hasPaddingVertical{}; const bool hasPaddingLeft{}; const bool hasPaddingTop{}; const bool hasPaddingRight{};