Skip to content

Conversation

@ddalp
Copy link
Contributor

@ddalp ddalp commented Dec 17, 2019

#3737 To fix borderRadius for TextInput, we should support updating properties for borderRadius in ControlViewManager class.

-Added a E2D test case to cover style properties related to border and color for TextInput (regular and secure text entry), using the newly created tree dump utility.
-Fixed some bugs in tree dump utility and updated the scripts so multiple tests can include TreeDump view more efficiently.

Microsoft Reviewers: Open in CodeFlow

@ddalp ddalp requested a review from a team as a code owner December 17, 2019 21:13
@ddalp ddalp changed the title Textborder Bunch of style properties fixes and TreeDump utility updates Dec 18, 2019
}

void ControlViewManager::InitializeDefaultProperties(XamlView view) {
// Set the default cornerRadius to 2 for controls, since WinUI usually default template the cornerRadius to 2
Copy link
Contributor Author

@ddalp ddalp Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 [](start = 37, length = 1)

0 #Closed

Super::UpdateProperties(nodeToUpdate, reactDiffMap);

if (finalizeBorderRadius)
UpdateCornerRadiusOnElement(nodeToUpdate, control);
Copy link
Contributor Author

@ddalp ddalp Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateCornerRadiusOnElement [](start = 4, length = 27)

Check IControl7 #Closed

<Assembly Name="*Application*" Dynamic="Required All" />
<Namespace Name="Windows.UI.Xaml" Serialize="All"/>
<Namespace Name="Windows.UI.Xaml" Serialize ="Required All"/>
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required [](start = 50, length = 8)

Add comments #Closed

<Content Include="Assets\TreeDump\ControlStyleRoundBorder.txt" />
<Content Include="Assets\TreeDump\ImageWithBorder.txt" />
<Content Include="Assets\TreeDump\ImageWithoutBorder-Subsequent.txt" />
<Content Include="Assets\TreeDump\ImageWithoutBorder.txt" />
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to update csproj everytime #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!


In reply to: 359599465 [](ancestors = 359599465)

},
});

export function ControlStylePage() {
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ControlStylePage [](start = 16, length = 16)

rename this file to ControlStylePage too #Closed

import { TREE_DUMP_RESULT, SHOW_IMAGE_BORDER, IMAGE_CONTAINER } from './Consts';
const TreeDumpControl = requireNativeComponent('TreeDumpControl');
import { SHOW_IMAGE_BORDER, IMAGE_CONTAINER, TREE_DUMP_RESULT } from './Consts';
import {TreeDumpControl} from './TreeDump'
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TreeDumpControl [](start = 8, length = 15)

space around it #Closed

*/

import { Switch, CheckBox, TextInput, View, StyleSheet, Button } from 'react-native';
import {DatePicker, Picker} from 'react-native-windows';
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DatePicker, Picker [](start = 8, length = 18)

Add spaces
{ DatePicker.. }
apply to other import too #Closed

@@ -0,0 +1,2 @@
import { requireNativeComponent } from 'react-native';
export const TreeDumpControl = requireNativeComponent('TreeDumpControl');
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TreeDumpControl [](start = 13, length = 15)

the filename to be TreeDumpControl.ts? #Closed

import { BasePage, By } from './BasePage';
import { SHOWBORDER_ON_CONTROLSTYLE } from '../../app/Consts';

class TextInputStyleTestPage extends BasePage {
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextInputStyleTestPage [](start = 6, length = 22)

rename it to ControlStyleTestPage? #Closed

@@ -0,0 +1,2 @@
import { requireNativeComponent } from 'react-native';
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright #Closed

TransferProperty(oldView, newView, winrt::Control::BackgroundProperty());
TransferProperty(oldView, newView, winrt::Control::BorderBrushProperty());
TransferProperty(oldView, newView, winrt::Control::BorderThicknessProperty());
TransferProperty(oldView, newView, winrt::Control::CornerRadiusProperty());
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CornerRadiusProperty [](start = 53, length = 20)

this property only exist in rs5+ #Closed

XamlView ViewManagerBase::CreateView(int64_t tag) {
XamlView view = CreateViewCore(tag);

InitializeDefaultProperties(view);
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitializeDefaultProperties [](start = 2, length = 27)

It can be a more generic name like:
OnViewCreated or OnViewInitialization #Closed

}

void ControlViewManager::InitializeDefaultProperties(XamlView view) {
// Set the default cornerRadius to 0 for Control: WinUI usually default cornerRadius to 2
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment that customer still see CornerRadius effect on RS5- because WinUI has different implementation and we can't change it #Closed

// Set the default cornerRadius to 0 for Control: WinUI usually default cornerRadius to 2
if (auto control = view.try_as<winrt::Windows::UI::Xaml::Controls::IControl7>()) {
winrt::Windows::UI::Xaml::CornerRadius cornerRadius{0};
control.CornerRadius(cornerRadius);
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

control.CornerRadius(cornerRadius); [](start = 4, length = 35)

I didn't do the testing, maybe control.CornerRadius( {0} ); work #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works!


In reply to: 359606312 [](ancestors = 359606312)


Super::UpdateProperties(nodeToUpdate, reactDiffMap);

if (finalizeBorderRadius) {
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finalizeBorderRadius [](start = 6, length = 20)

if (finalizeBorderRadius && control.try_as...) #Closed

private string m_errString = "";
private string m_uiaID = null;

private DispatcherTimer m_timer = null;
Copy link
Contributor

@licanhua licanhua Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DispatcherTimer [](start = 16, length = 15)

I'd like you to review the native module implementation. We should not define any variable in ViewManager.
ViewManager is shared by multiple instance of a Control, and the data should be put in the shadow node other than ViewManager. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I took the shortcut here as we only need support one TreeDump control instance per test. I do have work item to use native module once the host interface is refactored, we can revisit by then.


In reply to: 359610817 [](ancestors = 359610817)

Copy link
Contributor

@licanhua licanhua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ddalp ddalp merged commit db1c46a into microsoft:master Dec 20, 2019
@ddalp ddalp deleted the textborder branch December 20, 2019 05:14
ghost pushed a commit that referenced this pull request Jan 9, 2020
* GetFacebookReactInstance

* def

* ...

* Add __cdecl explicitly

* Update ReactUWP.vcxproj

* Update E2ETest to use ReactApplication (#3715)

* Update E2ETest to use ReactApplication

* Minor update

* Remove generating pch.pch

* Change files

* Shrink pch file size for Microsfot.ReactNative

* Revert "Remove generating pch.pch"

This reverts commit 39286c8.

* fix build

* Update Timeout

* applying package updates ***NO_CI***

* Update ParityStatus.md (#3555)

Documentation update based on #2852 completion

* Bump @microsoft/api-extractor from 7.6.1 to 7.7.0 (#3717)

Bumps [@microsoft/api-extractor](https://github.com/microsoft/rushstack) from 7.6.1 to 7.7.0.
- [Release notes](https://github.com/microsoft/rushstack/releases)
- [Commits](https://github.com/microsoft/rushstack/compare/@microsoft/api-extractor_v7.6.1...@microsoft/api-extractor_v7.7.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Bump @microsoft/api-documenter from 7.6.1 to 7.7.2 (#3724)

Bumps [@microsoft/api-documenter](https://github.com/microsoft/rushstack) from 7.6.1 to 7.7.2.
- [Release notes](https://github.com/microsoft/rushstack/releases)
- [Commits](https://github.com/microsoft/rushstack/compare/@microsoft/api-documenter_v7.6.1...@microsoft/api-documenter_v7.7.2)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Aligning Microsoft.Windows.CppWinRT Versions (#3733)

* Re-aligned SampleAppCPP project to match the others, #3728
* Updated all projects to 2.0.190730.2

* applying package updates ***NO_CI***

* Bump @types/react-native from 0.60.22 to 0.60.24 (#3740)

Bumps [@types/react-native](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-native) from 0.60.22 to 0.60.24.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-native)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Bump @types/node from 10.17.6 to 10.17.7 (#3741)

Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 10.17.6 to 10.17.7.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Update CONTRIBUTING.md

update instructions on how to install submodules in the case where you started out working on vnext then switched to current.

* Use ReactNative.V8JSI.Windows.0.1.6 and ReactNative.Hermes.Windows.0.1.6 (#3729)

* unify hermes and v8jsi version

* Change files

* use HERMES_Package and V8_Package

* remove them from reactuwp project

* applying package updates ***NO_CI***

* Reduce build time on pipeline (#3734)

* reduce build time

* changes

* fix pipeline failure

* use Add-AppxPackage

* E2E test still use windows-2019 image

* force install vs dependencies on vs2019 image for E2E test

* parameters.forceVSDependencies

* add ../../.ado/variables/vs2017.yml

* Revert "add ../../.ado/variables/vs2017.yml"

This reverts commit b829251.

* revert and force

* Fix pipeline error

* Add react-native-win32 package (#3762)

* Add react-native-win32 package

* Publish packages using access public

* applying package updates ***NO_CI***

* Miscellaneous fixes in ETW tracing and Systrace (#3745)

* Miscellaneous fixes in ETW tracing and Systrace

* Miscellaneous fixes in ETW tracing and Systrace - Adding missing files

* Submitting the ETW schema resouce dll and the register script

* Change files

* applying package updates ***NO_CI***

* Strongly typed value serialization and deserialization using IJSValueReader, JSValue, and IJSValueWriter (#3760)

* Merged implementation of strongly typed value serialization and deserialization using IJSValueReader, JSValue, and IJSValueWriter

* Change files

* Updated CLI template for C++ native modules

* applying package updates ***NO_CI***

* Update to [email protected] (#3769)

* Update to [email protected]

* Change files

* Change files

* applying package updates ***NO_CI***

* Fix toggle debugger setting issue with ReactApplication (#3767)

* Fix toggle debugger setting issue with ReactApplication

* applying package updates ***NO_CI***

* Delete .pch after build on pipeline (#3771)

* delete pch after build

* applying package updates ***NO_CI***

* Redirect build directory to C: on vs2017-win2016 build machine (#3768)

* init

* rollback language to default

* use False

* Fix by comment and enable SampleApp on pipeline

* update

* disable msbuild SampleApp

* Apply suggestions from code review

* applying package updates ***NO_CI***

* ignore Bundle folder in sampleapps (#3778)

* Add tree dump utility to E2E test framework and fix Image border issue (#3754)

Add TreeDump utility to E2E test framework and image border fix with TreeDump tests.

* applying package updates ***NO_CI***

* Update yarn.lock

* Change files

* Added new unit test projects to ReactWindows-Universal solution. (#3775)

* Added new unit test projects to ReactWindows-Universal solution.

* Made C# code compatible with C# 7.0

* Fixed some build breaks found by CI

* Trying to fix the Microsoft.ReactNative.Cxx.UnitTests build in CI loop

* Fixed Microsoft.ReactNative.Cxx.UnitTests project build break in CI and removed AMD64.

* Removed C# unit tests project

* applying package updates ***NO_CI***

* Update document for removing ReleaseBundle and DebugBundle (#3702)

* Update doc to removing DebugBundle and ReleaseBundle

* Change files

* applying package updates ***NO_CI***

* CLI reads name from app.json if it doesn't exist in package.json (#3781)

* read name from app.json

* Change files

* applying package updates ***NO_CI***

* Change CLI to add prompt if no --template parameter is supplied (#3784)

* merge

* add prompt

* Change files

* applying package updates ***NO_CI***

* Conditionally use BitmapImage (#3712)

* Use BitmapImage for cover, contain, and stretch resizeModes

* Fix comments

* timing issues

* wip

* Move 'center' resizeMode to BitmapImage

* code cleanup

* ReactImage->Source() refactor

* Clean up for PR

* Change files

* PR feedback

* only create ImageBrush and BitmapImage is needed

* Remove memory stream cache + flicker workaround

* don't cache availablesize + formatting

* SizeChanged event handler

* Fix dynamic resizeMode switch edge case

* Fix merge conflict + add inline data to image playground

* fix playground buildci

* applying package updates ***NO_CI***

* Bump rnpm-plugin-windows from 0.3.8 to 0.4.0 (#3800)

Bumps [rnpm-plugin-windows](https://github.com/ReactWindows/react-native-windows) from 0.3.8 to 0.4.0.
- [Release notes](https://github.com/ReactWindows/react-native-windows/releases)
- [Commits](rnpm-plugin-windows_v0.3.8...rnpm-plugin-windows_v0.4.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Bump pretty-format from 24.8.0 to 24.9.0 (#3764)

Bumps [pretty-format](https://github.com/facebook/jest/tree/HEAD/packages/pretty-format) from 24.8.0 to 24.9.0.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v24.9.0/packages/pretty-format)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Bump stacktrace-parser from 0.1.6 to 0.1.8 (#3765)

Bumps [stacktrace-parser](https://github.com/errwischt/stacktrace-parser) from 0.1.6 to 0.1.8.
- [Release notes](https://github.com/errwischt/stacktrace-parser/releases)
- [Commits](https://github.com/errwischt/stacktrace-parser/commits)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Bump @types/react-native from 0.60.24 to 0.60.25 (#3757)

Bumps [@types/react-native](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-native) from 0.60.24 to 0.60.25.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-native)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Port windowsbrush code into react-native-windows (#3789)

* merge

* move windowsBrush stuff over from fork

* added RNTester page

* Change files

* CR comments

* applying package updates ***NO_CI***

* Make sure that C# and C++ SampleApp projects identifiers have proper CS and Cpp suffixes to avoid name collisions. (#3802)

* Removed Bridge sub-namespace in favor of Microsoft.ReactNative (#3804)

* Removed Bridge sub-namespace in favor of Microsoft.ReactNative

* Change files

* Fixed E2ETest build break

* applying package updates ***NO_CI***

* fixing case issues (#3806)

* Bump @react-native-community/cli from 2.9.0 to 2.10.0 (#3663)

Bumps [@react-native-community/cli](https://github.com/react-native-community/react-native-cli) from 2.9.0 to 2.10.0.
- [Release notes](https://github.com/react-native-community/react-native-cli/releases)
- [Commits](react-native-community/cli@v2.9.0...v2.10.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* reduce build flavors for RNWUniversalPR  (#3773)

* reduce build flavours

* reenable DesktopPR

* \\

* Update .ado/windows-vs-pr.yml

* Bunch of style properties fixes and TreeDump utility updates (#3793)

* Support CornerRadius for TextInput

* Fix cornerRadius issue for TextInput and some updates to TreeDump, plus new test for control style.

* applying package updates ***NO_CI***

* Get flow clean, and turn on flow-check during build (#3730)

* Get flow check working

* Get flow clean, and turn on flow check during build

* Change files

* fix

* Move RNTester files to matching location from RN\rntester

* PR feedback

* fix

* applying package updates ***NO_CI***

* SourceCode module should provide scriptURL when running livereload without webdebugger (#3803)

* Minor fixups after initial rn-win32 checkin

* Provide source uri in SourceCode module when using livereload

* Provide source uri in SourceCode module when using livereload

* Change files

* build fix

* fix build

* fix build

* applying package updates ***NO_CI***

* Remove remaining need for fork of RN for win32 JS (#3811)

* Remove remaining need for fork of RN for win32 JS

* Change files

* Build fix

* Change files

* applying package updates ***NO_CI***

* Export ability to query names of loaded native modules (master branch) (#3813)

* Export ability to query native module names

This is needed for testability (intenral CR using it out now). It's not ideal to add more exports, but we will always have to have some between instance interfaces.

* Change files

* Fix x86 mangeled name

* applying package updates ***NO_CI***

* Changed Microsoft.ReactNative to be independent from ReactUWP (#3809)

* Changed Microsoft.ReactNative to be independent from ReactUWP

* Removed ReactUWP project from the ReactUWPTestApp to reduce compiled code size.

* Removed commented code from pch.h

* Moved WindowsBrushExample.windows.tsx to fix RNTester bundle building

* Updated TreeDumps to fix test cases.

* An attempt to fix E2ETest

* Changed ViewPanel naemspace in the E2ETest tree dumps

* Changed namespace for ViewPanel in other E2ETest tree dumps

* applying package updates ***NO_CI***

* Allow UAP SDK to be in other folder other than ProgramFiles (#3815)

* check UAP in SDK10 installation folder

* applying package updates ***NO_CI***

* Add InjectBundleContent target (#3821)

* add InjectBundleContent target

* Change files

* format

* applying package updates ***NO_CI***

* Bump @types/react-native from 0.60.25 to 0.60.28 (#3831)

Bumps [@types/react-native](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-native) from 0.60.25 to 0.60.28.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-native)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* [Security] Bump handlebars from 4.1.2 to 4.5.3 (#3818)

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.1.2 to 4.5.3. **This update includes a security fix.**
- [Release notes](https://github.com/wycats/handlebars.js/releases)
- [Changelog](https://github.com/wycats/handlebars.js/blob/master/release-notes.md)
- [Commits](handlebars-lang/handlebars.js@v4.1.2...v4.5.3)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Call StartAnimation on m_scaleCombined for ScaleX / ScaleY animations (#3829)

* Call StartAnimatiom on m_scaleCombined for ScaleX / ScaleY animations

There was a copy-paste error previously that started m_translationCombined instead.

* Change files

* applying package updates ***NO_CI***

* Remove remaining need for fork of RN for win32 JS (#3834)

* Remove remaining need for fork of RN for win32 JS

* Change files

* Build fix

* Change files

* Enable flow type checking in win32

* Fix build

* applying package updates ***NO_CI***

* Rename GetFacebookReactInstance

* Fix code review comment

* Update TurboModuleUtils.cpp

* Fix lint errors

Co-authored-by: Di Da <[email protected]>
Co-authored-by: rnbot <[email protected]>
Co-authored-by: Harini Kannan <[email protected]>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Jon Thysell <[email protected]>
Co-authored-by: kmelmon <[email protected]>
Co-authored-by: Canhua Li <[email protected]>
Co-authored-by: Andrew Coates <[email protected]>
Co-authored-by: Anandraj <[email protected]>
Co-authored-by: Vladimir Morozov <[email protected]>
Co-authored-by: Marlene Cota <[email protected]>
Co-authored-by: Mike Kaufman <[email protected]>
Co-authored-by: Nick Gerleman <[email protected]>
Co-authored-by: Tom Shea <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants