Support right-clicking on iPadOS #27019
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
moffatman
force-pushed
the
ipad_rightclick
branch
from
921da7c to
90034c0
Compare
moffatman
changed the title
|
cc @gaaclarke |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Outstanding contribution, thanks! I just have a few nits/questions, otherwise LGTM.
| self.continueAfterFailure = NO; | ||
| } | ||
|
|
||
| #ifdef __IPHONE_15_0 |
There was a problem hiding this comment.
I don't think you want conditional compilation here, you want runtime checks like you have below.
There was a problem hiding this comment.
I'm not sure this will compile on Xcode 12 without this conditional, since supportsPointerInteraction was only introduced in the Xcode 13 beta.
There was a problem hiding this comment.
If you want to do that use #if (defined (__IPHONE_15_0) && __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_15_0) then. We don't just care that the SDK has it, but that the target platform has it.
edit: nevermind, LGTM, sdk version is the same thing as running environment for tests.
There was a problem hiding this comment.
A comment wouldn't hurt, a runtime check with respondsToSelector would be a bit more apparent and easier to maintain, but a comment will be good enough.
|
|
||
| #ifdef __IPHONE_15_0 | ||
| - (void)testPointerButtons { | ||
| if (@available(iOS 15, *)) { |
There was a problem hiding this comment.
How come this is asking for iOS 15 when the implementation check is for iOS 13.4?
There was a problem hiding this comment.
While the pointer/trackpad features on iPad came out in iPadOS 13.4, synthesizing those events in XCTest wasn't supported until this year's new Xcode/iPadOS beta releases.
There was a problem hiding this comment.
You aren't synthesizing any events here though, right? At the least please add a comment, preferably one that points to the API that is being used that requires ios 15.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I'm going to approve this and trust you to put the comments in the code so someone else can just hit the merge button. I'll be out of office for awhile and don't want you to get blocked or have to go through review again.
|
Or better yet, I have a spare moment. I'll land this and just do the quick clean up in a follow up PR so that you're off the hook. Thanks for the contribution. |
* e94ed1c [web] skip overlay test on Safari (flutter/engine#27114) * 6b92842 Roll Skia from 1c467774e56e to e9ab391765c7 (1 revision) (flutter/engine#27143) * 0977906 Update goldens test for Thai. (flutter/engine#27144) * 4ec5781 [web] replace browser-related conditional logic with BrowserEnvironment (flutter/engine#27084) * 05ce70e enable DisplayList by default (flutter/engine#27130) * e3357d2 Roll Fuchsia Mac SDK from jzKy-rCeR... to oiyYFMOd3... (flutter/engine#27145) * cb1c312 Support right-clicking on iPadOS (flutter/engine#27019) * 4ac4e5c Roll Fuchsia Linux SDK from 4MLcvcjCH... to QbIpQIqxK... (flutter/engine#27146) * e6250f7 Roll Skia from e9ab391765c7 to 04d79fc59488 (1 revision) (flutter/engine#27148) * 2dacffa Revert "enable DisplayList by default (#27130)" (flutter/engine#27153)
4 participants
iPadOS 13.4 introduced improved cursor support, including native support for more pointer "buttons" than the default. There is also a change needed to the framework to avoid adding the default button to iPad right-clicks (link tba).
flutter/flutter#55809