fluentui-apple icon indicating copy to clipboard operation
fluentui-apple copied to clipboard

Tooltip could crash during runtime because of `preconditionFailure()`

Open imWildCat opened this issue 4 years ago • 2 comments

Environment Information

  • Platform:
    • [x] iOS: any
    • [x] macOS: any
  • Package version(s): (fill this out, include which package manager you're using): latest
  • Xcode and OS versions: (fill this out if relevant): any version supported

Please provide a reproduction of the bug:

The anchorView's window can be nil when it got removed from the view hierarchy. In this situation, preconditionFailure() will lead to runtime crash.

Since Tooltip isn't a critical UI for most business logic, should we crash the app when this exception happens?

I searched for preconditionFailure() in this repo (https://github.com/microsoft/fluentui-apple/search?p=1&q=preconditionFailure) and found that most of its use is for unimplemented initializers.

Actual behavior:

Tooltip could crash the app.

Expected behavior:

This UI component shouldn't crash app. Or at least, we can have an alternative to crash (runtime error log?)

Priorities and help requested:

Are you willing to submit a PR to fix? No. Or yes if we have a clear path to fix it.

Requested priority: Normal

imWildCat avatar Jun 20 '21 13:06 imWildCat

Thanks for the feedback. runtime error log is bit messy to do as a shared UI library because each client team might have different way of error logging and most of the time developers tend to ignore logs on the console. I think author's thoughts were to make sure clients understand the tooltip is actually showing on a valid anchor view and crashing might be the best way to debug user scenarios if things are not shown in intended way.

harrieshin avatar Jun 22 '21 20:06 harrieshin

Thanks @harrieshin!

Yeah, I agree the anchorView should be in the view hierarchy (and the window of it shouldn't be nil) while showing Tooltip. This should be a precondition.

Could we consider other alternative approach? Like returning a Result<Void, Error> enum to make it more flexible?

In large apps, the view hierarchy could be complex. As a result, when Tooltip.show() is executed by the runloop, the anchorView.window might be nil even if the developer is very careful about it.

imWildCat avatar Jun 23 '21 02:06 imWildCat