Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-25301]:iPhone X: Need to be able to control the Insets / Layout Margins #9475

Merged
merged 29 commits into from Nov 14, 2017

Conversation

vijaysingh-axway
Copy link
Contributor

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Left a few comments about general proxy-handling. Besides that, I'm wondering if we need a wrapper-view to achieve the safeAreaInsets changes, although I can see this might be the only way to solve it with the current layout-system.

I also tested the built SDK with this change and the whole view does not seem to respond anymore. I tried with https://github.com/hansemannn/studentenfutter-app and changed the SDK to 7.0.0 which is the latest master this PR is being filed against. Please check it out as well to ensure it's something with my env.

@@ -27,6 +27,8 @@
- (void)refreshBackButton;
- (void)updateNavBar;
- (void)boot:(BOOL)timeout args:(id)args;
@property (nonatomic, assign) TiViewProxy *safeAreaViewProxy;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add one tmpty line for readability. Also ensure to format everything with our clang-format

@@ -937,6 +940,49 @@ - (void)cleanupWindowDecorations
[barImageView removeFromSuperview];
}
}

- (void)processForSafeArea
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be compiled out by #if IS_XCODE_9 to decrease the code-size for Xcode < 9.

bottom = rootView.safeAreaInsets.bottom;
top = rootView.safeAreaInsets.top;
}
[self.safeAreaViewProxy setTop:[NSNumber numberWithFloat:top]];
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Use NUMFLOAT for all 4 sizes
  • Access safeAreaViewProxy via [self safeAreaViewProxy]

@@ -20,6 +20,8 @@
#import <QuartzCore/QuartzCore.h>
#import <libkern/OSAtomic.h>
#import <pthread.h>
#import "TiUIWindowProxy.h"
#import "TiUIViewProxy.h"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Compile out for Xcode < 9

windowProxy.shouldExtendSafeArea = [TiUtils boolValue:[self valueForUndefinedKey:@"extendSafeArea"]
def:NO];
if (!windowProxy.safeAreaViewProxy && !windowProxy.shouldExtendSafeArea) {
windowProxy.safeAreaViewProxy = [[TiUIViewProxy alloc] _initWithPageContext:[self pageContext] args:nil];
Copy link
Collaborator

Choose a reason for hiding this comment

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

May need to be [[TiUIViewProxy alloc] _initWithPageContext:[self pageContext]] autorelease]

[windowProxy.safeAreaViewProxy add:a];
} else {
[self add:a];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Compile out for Xcode < 9, change the access of safeAreaViewProxy as well.

#endif
[self add:a];
}
#if IS_XCODE_9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this bracket belong to? Maybe this is more readable (untested!):

  // allow either an array of arrays or an array of single proxy
  if ([arg isKindOfClass:[NSArray class]]) {
    for (id a in arg) {
#if IS_XCODE_9
      if (windowProxy.safeAreaViewProxy) {
        [windowProxy.safeAreaViewProxy add:a];
	continue;
      }
#endif
      [self add:a];
    }
    return;
  }

@vijaysingh-axway
Copy link
Contributor Author

@hansemannn , I tried your app with extendSafeArea = false , UI is not responding . But if set extendSafeArea = true, its working fine. KitchenSink- V2 is also working fine. Tried to create a similar Test case but it is working properly.
var win = Ti.UI.createWindow({
backgroundColor: 'white',
title: 'Shopping List'});

var control = Ti.UI.createRefreshControl({
tintColor:'red'
});
var listView = Ti.UI.createListView({
refreshControl:control
});

// Left navigation button to toggle between editing and selection mode
var button = Ti.UI.createButton({title: 'Edit'});
win.leftNavButton = button;

var sections = [];
var fruitSection = Ti.UI.createListSection({ headerTitle: 'Fruits'});
var fruitDataSet = [
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}}
];
fruitSection.setItems(fruitDataSet);
sections.push(fruitSection);

var vegSection = Ti.UI.createListSection({ headerTitle: 'Vegetables'});
var vegDataSet = [
{properties: { title: 'Carrots'}},
{properties: { title: 'Potatoes'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}},
{properties: { title: 'Apple'}},
{properties: { title: 'Banana'}}
];
vegSection.setItems(vegDataSet);
sections.push(vegSection);

listView.sections = sections;

var footerView = Ti.UI.createView({
backgroundColor:'gray' ,
height:50,
bottom:0
});
var footerLabel = Ti.UI.createLabel({
text: 'Footer View',
});
footerView.add(footerLabel);
win.add(listView);
win.add(footerView);

var navWin = Ti.UI.iOS.createNavigationWindow({window: win});
navWin.open();

Will check again. Can you please try to create a simple test case from your app, which I can use in debug . Thanks!

@build
Copy link
Contributor

build commented Sep 25, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

@vijaysingh-axway That's exactly what I would have done to test it, hehe. Maybe the overlay (loader) influences it. I'll try removing UI-parts incrementally today to track it down. The other changes look good so far!

@jquick-axway
Copy link
Contributor

Side Comment:
If we ever decide to support tvOS or Android TV in the future, we can use an API like this to determine the overscan insets/margins.

And for Android phones/tablets that display an overlaid/translucent status bar and navigation bar, we can bind this new JavaScript API to the Java WindowInsets API, but this is only available on Android 4.4 and higher. But we could use it in the future when dropping older OS versions. (The older Android API View.getWindowDisplayVisibleDisplayFrame() for doing this is unreliable and usually provides the wrong results. Google's own comments in their code here admits to this.)

@hansemannn
Copy link
Collaborator

@ewieberappc Build now passing on this ones :-) Getting back the green arrows, feeling like inside a DC-comic!


Read more about the safe-area layout-guide in the [Human Interface Guidelines](https://developer.apple.com/ios/human-interface-guidelines/overview/iphone-x/).

Note: The default value is `true`, so content will extend the safe-are for backward compatibility. In Titanium SDK 7.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. Change "safe-are" to "safe-area".

Note: The default value is `true`, so content will extend the safe-are for backward compatibility. In Titanium SDK 7.0.0
and later, this property will be `false` by default to automatically adjust the layout. This will be a breaking
change for windows without a navigation-bar, as the status-bar is part of the safe-area, so subviews that are positioned
relatively to the status-bar do not need to include the height of the status-bar (20px) in the margins anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

The status-bar on iOS is 20dp (ie: Apple points), not 20px.

description: |
If the value is `true`, the content (subviews) of the window will not respect the safe-area, meaning that the content
will fill the whole screen available for rendering. If the value is `false`, the content of the
window will be rendered inside the safe-area (including then status-bar, iPhone X home-indicator & iPhone X notch).
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:
If this is set false, then what will this mean for a translucent status bar? Will the window's background color or image be behind the translucent status bar or not? If yes, then does the window's height property include the height of the status bar then?

I ask because we may want to document these details. This may matter to a developer who is doing absolute positioning via a composite layout that involves subtracting from the window height. Please see the "LayoutTest.js" sample project attached to TIMOB-24972 and try its "Absolute Position" test in the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If false, it will not extend the safe-area (status-bar is part of that), meaning that a translucent status-bar should receive the window-background-color and the height of the window will not be influenced by the status-bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so, this means that window.height and window.width returns the safe area width height in this case, right? We should document this detail.

description: |
If the value is `true`, the content (subviews) of the window will not respect the safe-area, meaning that the content
will fill the whole screen available for rendering. If the value is `false`, the content of the
window will be rendered inside the safe-area (including then status-bar, iPhone X home-indicator & iPhone X notch).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn status-bar, iPhone X home-indicator & iPhone X notch are not part of safe area. Let us remove the statement from doc.
And we have planned to make extendSafeArea property false by default to automatically adjust layout in Titanium SDK 7.0.0. Do you really think we need to change it. In my view default property should be true. So that when developer change this property they will be aware of consequences of property and will change this wherever they require and verify the same. What you think ?

Let us discuss on this. Thanks.

@vijaysingh-axway
Copy link
Contributor Author

@ QE android test is getting failed. So it is not getting built.

Note: The default value is `true`, so content will extend the safe-area for backward compatibility.
Please note that status-bar, which have height (20pt), is outside safe-area. So any view added on window with
this property 'true', will start rendering below to status-bar. In other words, subviews that are positioned
relatively to the status-bar do not need to include the height of the status-bar (20pt) in the margins anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

The above description has grammar issues. How about we rewrite it to the following?...


description:
If set true (the default), then the content of the window will be extended to fill the whole screen and under the system's UI elements (such as the top status-bar) and physical obstructions (such as the iPhone X rounded corners and top sensor housing). In this case, it is the app developer's responsibility to position views so that they're unobstructed.

If set false, then the window's content will be laid out within the safe-area and its child views will be unobstructed. For example, you will not need to position a view below the top status-bar.

Read more about the safe-area layout-guide in the Human Interface Guidelines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Josh! We should consider having a grammar review for all docs changes due to an international team 🙂.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to help! :)
And my grammar isn't perfect either. I just made a couple edits to my text above just now.

@hansemannn
Copy link
Collaborator

@jquick-axway PR updated. @ewieberappc Ready to be merged (6_3_X was test & merged earlier already).

@mukherjee2
Copy link
Contributor

@eric34 can you please merge this one? CR and FR passed.

@eric34 eric34 merged commit 4ce7c86 into tidev:master Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants