-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Use a proper tool bar in Document Outline #68094
Conversation
Approving, predicated reasonable smoke-testing. @ryzngard to take a peek when he has time, to see if there's any questions/concerns on his part. |
Can you link to the tracking issue about document outline behavior? |
Looking. Did you run https://accessibilityinsights.io/ on the window? |
@ryzngard I haven't run the tool. Last time I tried it was a resounding failure (provided no usable information good or bad). |
It's a requirement https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/11451/Accessibility?anchor=local-testing. If you need help understanding the tool or the results I'm available. |
_viewModel.SearchText = ""; | ||
} | ||
|
||
void IVsWindowSearch.ProvideSearchSettings(IVsUIDataSource pSearchSettings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, I've never seen this before.
I was able to run the tool and inspect the Document Outline window. Prior to this change, there are 13 failures. After this change, there are 6 failures, with all failures in the tool bar and search box being resolved. |
Great! Thank you for checking and fixing the issues. My hope was embedded would do the right thing, but needed to verify that there was no oddities with the approach |
Fixed the remaining items (which were unrelated to this PR but might as well fix them all at once). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you want for me to do a second pass. i would like to see how all the specific issues were addressed.
Thanks for looking at this!
@CyrusNajmabadi No problem, I'll keep auto-merge enabled so it will merge as soon as you complete the review. |
This comment was marked as resolved.
This comment was marked as resolved.
InternalUtilities.SetValue(pSearchSettings, SearchSettingsDataSource.PropertyNames.PrefixFilterMRUItems, false); | ||
InternalUtilities.SetValue(pSearchSettings, SearchSettingsDataSource.PropertyNames.MaximumMRUItems, (uint)25); | ||
InternalUtilities.SetValue(pSearchSettings, SearchSettingsDataSource.PropertyNames.SearchWatermark, ServicesVSResources.Document_Outline_Search); | ||
InternalUtilities.SetValue(pSearchSettings, SearchSettingsDataSource.PropertyNames.SearchPopupAutoDropdown, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this one do?
{ | ||
// Construct a rectangle at the left of the item to avoid horizontal scrolling when the items is longer than | ||
// fits in the view. We make the rectangle 25% the width of the containing tree view to ensure at least some | ||
// of the text is visible for deeply nested items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nifty.
renderHeight = item.RenderSize.Height; | ||
} | ||
|
||
var croppedRenderWidth = Math.Min(item.RenderSize.Width, SymbolTree.RenderSize.Width / 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any concern about this ending up 0? or is that ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that to be fine. The default value here if you just call BringIntoView()
is Rect.Empty
.
Old behavior:
New behavior after fixing tool bar:
New behavior after fixing tool bar and search box:
Fixes #63859
Closes #67264
Fixes #68096
Fixes #68098
Fixes #68100
Fixes #68102
Fixes #68103
Fixes AB#1680146 (internally-filed accessibility bug related to search box theming)