Update Cupertino desktop text selection toolbar #121829
Conversation
The macOS context menu UI was outdated. With these changes it will be very similar to native.
flutter-dashboard
bot
added
a: text input
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Thanks for updating this! I hadn't noticed that it got out of date, but this version looks much better.
Some questions about light/dark mode to make sure about before merging.
Also, I wonder if there is anything that would be useful to test here?
| color: Color(0xFFECECEC), | ||
| darkColor: Color(0xFF333333), | ||
| ); |
There was a problem hiding this comment.
Nit: I think this should be indented more like this, but not a big deal.
CupertinoDynamicColor.withBrightness(
color: Color(0xFFECECEC),
darkColor: Color(0xFF333333),
);There was a problem hiding this comment.
@justinmc I used dart format, but I agree this is cleaner. Should I disregard the default formatting?
packages/flutter/lib/src/cupertino/desktop_text_selection_toolbar.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/desktop_text_selection_toolbar_button.dart
Show resolved
Hide resolved
|
Nice, much better. I think the text may benefit from being bumped 1-2 pts, but I don't think we'll be able to get true fidelity without using San Francisco |
|
@guidezpl Thanks! But yeah, I was not very sure about decreasing the size to 13, I will be bumping it to 14 again. |
|
@justinmc You're welcome! Flutter is amazing and I'm really enjoying contributing 😄 Regarding the tests, I don't know. I tried to think about it when I was making these changes, but I couldn't come up with anything. Honestly, I don't know much about testing in Flutter at this scale, so what do you think? And what's your opinion on the transparency/blur effect? I thought about adding it but as the current context menu is opaque I ended up opting to leave it like this. |
|
Ah the blur effect would be cool and I imagine would be doable in Flutter. Maybe do it in a separate PR if that's easier, otherwise go for it here. For tests, one thing you could do is to find the BoxDecoration and then check that it now has a BoxShadow. You could also probably check the BorderRadius that's been added to the inner CupertinoButton. |
|
@justinmc I decided to give the blur effect a try and it's really cool. The thing is, macOS applies a saturation boost besides the blur and without it the effect looses its charm. The only way I could find to increase saturation was via I kept the
|
- Transform toolbar boxShadow into a constant - Increase font size to 14
|
@justinmc And about the tests, I'm legitimately curious as to why it would be important to test "static" things like shadow and borderRadius. If someone changes these values in the future, wouldn't the change be deliberate and probably because macOS' native counterparts changed too, thus making the tests just another place to update? |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@justinmc I don't know if you didn't continue reviewing because of problems that I still need to fix, so just asking, is there something that I need to do? I noticed that a test failed too, but I didn't bring it up as it doesn't look like there's anything to do with what I changed, the error is |
That's one reason it could change, and it's true that in that case the test would just add another place to update. That shouldn't be a big burden, though; updating a handful of tests will be a much smaller amount of work than is involved in pinning down what the right new values are in the first place. But the other possibility is a hypothetical future change breaking this code by accident, when the change was aimed at doing something else. For example, someone's adding some other feature, perhaps some new way to customize the appearance of this widget, and by accident they write the logic so that it never invokes some part of this code. That's the case where a test will come in handy. I'm not sure why the you-need-a-test bot didn't comment on this PR; I would have expected it to. If it had, one thing it would have said is: the purpose of a test is to make sure we don't accidentally revert your change. Is there anything in your change where it would be a problem if it accidentally got reverted? |
There was a problem hiding this comment.
Sorry, I just dropped off because this got lost in my GitHub notifications. Feel free to ping me on Discord if I disappear again :)
I'm pretty sure that test failure has nothing to do with this PR. I tried rerunning it but I'm not sure if it will pass. If not, can you try pushing a merge commit?
+1 to @gnprice's explanation about the tests. I try to avoid tests that just stupidly check a value in the UI, but if there isn't a better way to test them, they're better than nothing IMO, to prevent regressions.
Edit: By the way, I think the blur with the contrast boost looks awesome. Thank you for digging into that and finding the thread about how to do it.
packages/flutter/lib/src/cupertino/desktop_text_selection_toolbar.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/desktop_text_selection_toolbar.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/cupertino/desktop_text_selection_toolbar_button.dart
Show resolved
Hide resolved
|
@gnprice Thank you for explaining all this, it makes more sense now :) I will add some tests to make sure the widget stays like this. |
@justinmc No problem :)
Just merged, let's see if the tests pass this time.
Yeah, it makes a lot of sense :)
Thank you! It took some time to understand why a simple blur looked wrong and how to fix it, but I think the result was pretty good too. |
One test was failing because of a trailing space, formatting should fix.
|
There are two tests failing on web:
Canvaskit does not support The only solution that I can think of is to use only blur on web. |
|
I agree. Can you disable the unsupported pieces on web and leave a comment explaining and linking to that GitHub issue? Actually this won't affect many users. By default, Flutter web uses the browser's context menu. I only added support for showing this Flutter-drawn context menu on web in the last few months (#118194). |
|
@justinmc it does appear to blur but I guess it's subtle with the blur amount applied:
Thanks for the PR @luccasclezar! |
|
Ah thanks, you guys are right! I tried it on Mac and Flutter and they're both similarly subtle. 👍 |
|
This made my day! Thank-you so much! |
7 participants
The current look-and-feel of the CupertinoDesktopTextSelectionToolbar is outdated and it doesn't change the text color when hovered, which usually leads to poor contrast (see first image).
Current Flutter context menu (pre-PR):
New Flutter context menu:
Native context menu on macOS 13.2 for reference:
Issue
#121827
Pre-launch Checklist
///).