xamarin / Xamarin.PropertyEditing

MIT License
24 stars 16 forks source link

[Mac] Make Brush Tabs Navigable. Centralise Tabs code under TabButton… #672

Closed CartBlanche closed 4 years ago

CartBlanche commented 5 years ago

… class.

Fixes https://github.com/xamarin/Xamarin.PropertyEditing/issues/654

Fixes AB#1001604

CartBlanche commented 5 years ago

@ermau I'm not seeing a white background post keyboard selection. Only as an indiction that the button has been "clicked" when the space bar is used to fire the onClick/Activated event. I've had a look through XCode and it seems to also show a button "flash" when a button is selected by the keyboard.

CartBlanche commented 4 years ago

@ermau The functionality you speak of is correct when using NSSegmentedControls, like in MacOS System Preference screen etc, but when using NSTabViewController or similar, like XCode does in this video and as we do in proppy, the expected behaviour is to be able to tab between the options and selecting them. In that situation it does NOT allow the left and right arrow keys. https://youtu.be/IYsFq70dboQ XCode also highlights the button when mouse clicked, so developers on Mac will be used to that behaviour too.

ermau commented 4 years ago

The functionality you speak of is correct when using NSSegmentedControls, like in MacOS System Preference screen etc, but when using NSTabViewController or similar, like XCode does in this video and as we do in proppy, the expected behaviour is to be able to tab between the options and selecting them. In that situation it does NOT allow the left and right arrow keys.

👍

XCode also highlights the button when mouse clicked, so developers on Mac will be used to that behaviour too.

It does not, it focus-highlights it once you've hit tab at some point. Ours focus-highlights as soon as it's open, not based on a click. There's three different "highlights" at play here. Xcode does not use underlines to show which tab is active, they use a colored image, so ignore that one. Our icon based tabs (top of the control) in this branch are highlighting based on focus around the edge icon itself, but our text-based tabs highlight focus in a rectangle around the whole tab like Xcode does.

BretJohnson commented 4 years ago

@CartBlanche From me checking out the video earlier, I do think we'll want to make a few more tweaks here, as Eric suggests.

Also feel free to ping, @vancura, say in Slack, for any UX questions on this. Your time zones match up, which should help. And Vaclav can help confirm exactly what the UX should be, answering any other outstanding questions there. Just a thought, which may help.

CartBlanche commented 4 years ago

@ermau @BretJohnson I've made a change so by default it focuses on a control inside the tabview. This works for Material and Resource tabs, but I was unable to get it to work in the Solid Brush. In Solid Brush i've left it to focus on the Tab button itself and the user can still tab into the items from there. My suggestion is that this at least gives user the ability to tab around. With the caveat that the we need to raise an issue to fix the Solid Brush focus issue. So I'd suggest if this latest fix works in terms of accessibility we pull it in and raise the aforementioned issue to fix Solid Brush.

CartBlanche commented 4 years ago

@chkn I have pushed a fix for 1, 2 and 3.

With 3 though, once they tab into the NSOutlineView there is no visual cue they are there, until they navigate up/down using the arrow keys. Then the highlight appears. We can't auto highlight as that would automagically change the colour. I did look to see if there was a way to have FocusRight around the NSOutlineViews, but I'm yet to find a solution.

BretJohnson commented 4 years ago

I did some more testing here, with the latest code. On the whole, it looks good. Though I wasn't able to navigate through the Material palette colors with the keyboard, unless there's some magic key combination I missed here.

chkn commented 4 years ago

The fixes for 1 and 2 look great!

For 3:

once they tab into the NSOutlineView there is no visual cue they are there, until they navigate up/down using the arrow keys.

This seems to be the default behavior on macOS, so I wouldn't worry about it for now. We can revisit later if it's an issue for people

However, I'm still seeing this on the Resource brush page:

focus still seems to be in a black hole when the popover is subsequently popped into that tab. Setting focus on the "Search resources" field instead might be more reliable.

Let's just fix that and get this puppy merged!

BretJohnson commented 4 years ago

For the most part, this all looked quite good to me. When a tab is selected, I do see the initial control on that tab selected, which (for the most part) looks good.

The only issue I see now is that it may work a little too well, since when selecting the "No brush" tab (the first one), because there aren't any controls on the tab itself, it actually selects the next tab when you hit space bar. In that case, it's likely better to not change the selection at all when hitting space bar - just leave it on the "No brush" tab.

chkn commented 4 years ago

@BretJohnson Looks like my changes had been accidentally reverted. They've been restored now, if you have a sec to give it one more go

BretJohnson commented 4 years ago

I tested again. I still the issue below, but otherwise it seems fine. I'm fine with us merging even with that issue, if we want.

The only issue I see now is that it may work a little too well, since when selecting the "No brush" tab (the first one), because there aren't any controls on the tab itself, it actually selects the next tab when you hit space bar. In that case, it's likely better to not change the selection at all when hitting space bar - just leave it on the "No brush" tab.