Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[iOS] Fix usage conflict of Title and AttributedTitle on ButtonRenderer #6944

Merged
merged 4 commits into from
Jul 30, 2019

Conversation

rmarinho
Copy link
Member

Description of Change

PR #5167 introduced a bug with iOS button text where it was setting a AttributtedTitle and the ButtonLayoutManager.cs was setting just the control Title when the text was changed.

API Changes

  • iOS

Behavioral/Visual Changes

When changing the Text button it should work, you should see the new text

Testing Procedure

There are several tests on ControlGallery that should pass with this fix, iOS should be green.
Can tests for example with test case Bugzilla46458 after clicking the button it should read Clicked.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@PureWeen
Copy link
Contributor

it looks like there's an infinite ping pong happening

If you run the label gallery the bounds on the label just keeps changing back and forth

If you change the LabelRenderer calls back to

Control.AttributedText = newAttributedText;

it no longer does this

@PureWeen
Copy link
Contributor

@rmarinho take a look at the tests and let me know what you think. It's only the last set of tests that are failing and when I run them locally they run mostly fine

  • if I run the material set before the picker ones the completed test fails with an odd NRE
  • If I run each of the sets that failed in appcenter locally the whole set passes. Though I do have to try to kick them off a few times before they actually run.
  • if you check appcenter the iOS device is just a black screen for the whole last part of the run which seems odd
  • I saw this same behavior when running locally against 4.1.0 as well so it didn't seem special to this PR

I'll play with it a bit more tomorrow see if I can get them running better locally

@PureWeen PureWeen force-pushed the fix-buttontitle-ios branch from aef84f7 to 768b0f2 Compare July 25, 2019 16:39
@PureWeen
Copy link
Contributor

@rmarinho all these same tests are failing on master

https://appcenter.ms/orgs/xtc-Xamarin-Forms/apps/XamControl/test/series/master/runs/12859e81-f366-4311-bdbd-69edb73728a8

But with this PR a lot less are failing :-) so I'd say this one is good to merge

@PureWeen PureWeen added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Jul 26, 2019
@PureWeen PureWeen removed their assignment Jul 29, 2019
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

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

Oops. Tests still failing.

@PureWeen
Copy link
Contributor

PureWeen commented Jul 30, 2019

@samhouts the iOS tests that are failing are failing on the revert as well so we're fine as far as iOS is concerned so now just need to wait for Android tests to finish

Here's the PR with the char spacing reverted

https://devdiv.visualstudio.com/DevDiv/_releaseProgress?_a=release-pipeline-progress&releaseId=398317

Same test is failing on iOS

Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

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

Ok, I will trust you.

iOS should be green.

_glares suspiciously at @PureWeen _

@rmarinho
Copy link
Member Author

Just 1 test failing on iOS not related

@rmarinho rmarinho merged commit 5d01051 into master Jul 30, 2019
@PureWeen PureWeen deleted the fix-buttontitle-ios branch July 30, 2019 15:16
@samhouts samhouts added this to the 4.3.0 milestone Sep 11, 2019
felipebaltazar pushed a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
…er (xamarin#6944)

* [iOS] Fix usage conflict of Title and AttributedTitle on ButtonRenderer

* if CharacterSpacing isn't set then don't set it

* remove comments

* check for null
felipebaltazar pushed a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
…er (xamarin#6944)

* [iOS] Fix usage conflict of Title and AttributedTitle on ButtonRenderer

* if CharacterSpacing isn't set then don't set it

* remove comments

* check for null
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/shell 🐚 a/visual blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. p/iOS 🍎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants