Bug: Expander Events Naming

[robloo]

I noticed that expander has two events: Expanding and Collapsed that are named differently.

microsoft-ui-xaml/dev/Expander/Expander.idl

Lines 30 to 31 in 4208a01

	event Windows.Foundation.TypedEventHandler<Expander, ExpanderExpandingEventArgs> Expanding;
	event Windows.Foundation.TypedEventHandler<Expander, ExpanderCollapsedEventArgs> Collapsed;

Expanding implies the event is fired before the control is expanded and Collapsed implies after. Therefore, a difference in naming here concerned me so I went to check the code. Sure enough, there seems to be a mismatch. According to the code below all events are fired BEFORE the is expanded state is applied meaning both events should be named with a 'ing' suffix. Is my understanding correct here? If so this is actually a bug.

microsoft-ui-xaml/dev/Expander/Expander.cpp

Lines 104 to 115 in 4208a01

	void Expander::OnIsExpandedPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& /*args*/)
	{
	if (IsExpanded())
	{
	RaiseExpandingEvent(*this);
	}
	else
	{
	RaiseCollapsedEvent(*this);
	}
	UpdateExpandState(true);
	}

The differences in naming is actually quite important as in the future four events should be added to cancel expanding/collapsing. #3279 (comment). There should be four total events in the end:

1. Expanding <- Can cancel
2. Expanded
3. Collapsing <- Can cancel
4. Collapsed

[marcelwgn]

This looks like a bug to me. @ejserna Can you confirm? @kat-y FYI

[robloo]

We might want to rename both to Expanded and Collapsed and move raising of the events until after the state is applied then.

Another wrench in the naming convention is the 'Preview' prefix. That may be needed instead as it seems to be the more recent convention but I can't find the docs at the moment. Edit: This doesn't seem to be the convention PreviewKeyDown is an example but that is a special case.

1. PreviewExpanded
2. Expanded
3. PreviewCollapsed
4. Collapsed

[michael-hawker]

Could be interesting to have a Expanding/Collapsing in the future, but we haven't seen a need from folks to support pro-actively cancelling the user trying to expand/collapse something. That seems a bit odd of a scenario for this type of UI. It's not something we supported in the Toolkit, we used Expanded and Collapsed.

So, I agree this should be Expanded and Collapsed and they should occur after the state updates.

[robloo]

I have a need for the Expanding and Collapsing events which is what caused me to write the original comment.

When there are many properties in an editor it is common to group them and put them in an accordion-style layout with Expanders stacked on top of one another. In my case, to save space, only one Expander should be opened at a time in the accordion. However, at least one expander should always be opened to show something to the user.

The closing event with cancelation is necessary to ensure the currently expanded expander in the accordion is never closed. It is only closed when another expander is expanded.

[robloo]

I should add there are many 3rd party expander controls that have the preview events as well.

https://docs.telerik.com/devtools/wpf/api/telerik.windows.controls.radexpander

Microsoft should not artificially constrain what controls can do just because they can't imagine what may be possible. I like to leave what is possible and what makes sense for their unique use cases up to app developers and their design teams.

[StephenLPeters]

Microsoft should not artificially constrain what controls can do just because they can't imagine what may be possible. I like to leave what is possible and what makes sense for their unique use cases up to app developers and their design teams.

The problem is that API's have costs, not only in terms of not only implementing, maintaining, and documenting, but also package size as well as a tiny bit of performance, additionally every added api makes the process of a developer learning the proper way to use a control a little bit harder. So we don't want to add APIs that could go unused, luckily we are developing in the open and can see hugely valuable feedback, like the expanding and collapsing events which you've outlined above, that will point out our mistakes and let us update our controls to best serve the community :)

[robloo]

@StephenLPeters Thanks. I should add as I've mentioned before I'm not asking for Microsoft to invest in this sort of thing. As it is an open-source project I would be happy to implement it myself in the future -- I only want to make sure Microsoft keeps an open mind which has been a struggle in the past. I'll open a separate feature proposal about this topic. It's mentioned a few separate places now.

[robloo]

Here is the discussion that took place on PR #4394. It seems the event naming, while not ideal for a few different reasons, is Microsoft's intent.

#4394 (comment)

[robloo]

I'm keeping this open as the sequence events are fired still was not correct.

[michael-hawker]

@robloo your scenario of preventing the control from being collapsed is still not addressed if there's no Collapsing event.

Both pre and post event events are helpful. That's why there's so many sets of 4 events patterned throughout the controls.

It's very odd to make the effort to have a pre and post event, but then not implement the other 2 events in the common 4 pattern.

Events are pretty straight-forward to add usually, so I don't see why there's resistance to following the established patterns that developers rely on. We spend more time discussing the merits of why these things exist over and over again than just implementing the other set of events.

@ranjeshj @ryandemopoulos I know more spec work has been done in the open on the repo, but I feel like having a set of guidelines for control creation in the repo would be useful. Things like which things should be template bound, what types of properties should have static resources, naming of styles, when adding events how should they be named, what event pairs should be added, etc...

All these things which seem to come up as bugs after things have shipped over and over again that should just be part of the best-practices for creating a control. It'd help streamline the process for everyone. Happy to work together on this too, as I've been wanting to put a blog together too about all the nitpicky things we do for Templated Controls in the Toolkit.

[robloo]

Events are pretty straight-forward to add usually, so I don't see why there's resistance to following the established patterns that developers rely on. We spend more time discussing the merits of why these things exist over and over again than just implementing the other set of events.

@michael-hawker I agree with your comments and am still against the IsOpen/Opening convention as I mentioned in the PR. I think it's a bit shortsighted and doesn't match with historic precedents. That said, this was opened as a 'bug' with a mismatch between the intent of the events and the actual implementation (naming). @ranjeshj Confirmed the naming actually matches the intent. However much I disagree, this isn't a bug in that case. (only the sequence of invoking the events needs to be fixed now)

I opened the other issue #4407 to address adding the other two missing events (which only needs to be slightly adjusted now). I still intend to add this in another PR (as requested) and hope there isn't further push back there.

All these things which seem to come up as bugs after things have shipped over and over again that should just be part of the best-practices for creating a control.

Yes, I've requested that in the past as well. We constantly have issues with text scaling, etc. because there is no checklist or standardized test sequence. The fundamental problem seems to be the individuals implementing controls now don't have a strong background in XAML. I'm at the point now where I'm just choosing my battles and fixing things myself when it directly affects apps I'm responsible for. I would like to emphasize again that the developers consuming the framework seem to have much more knowledge of the framework than most at Microsoft these days (obviously you are one of a handful not in this category). I don't think a lot of the problems you mentioned come from community contributions.

@ranjeshj @ryandemopoulos This is another example of Microsoft discouraging the 'Why Not' mentality. You are putting the burden on individuals to prove why something is needed. It should be obvious if a feature is directionally correct and a 'why not' mentality should be encouraged to facilitate innovation. As I've said before, you will end up killing future great ideas simply because you refused to implement some stepping stones.

#913 (comment)
#913 (comment)

Who can know what future features will exist? Lots of good ideas are only possible because of the smaller ideas and features that came before. If we must defend each small but good idea some future high-level features might never be possible. They won’t be possible simply because they couldn’t be envisioned without the smaller features and ideas leading up to them. Those smaller features and ideas never saw the light of day because they could not be defended as necessary without the future high-level feature in mind. A real catch 22. This is the most fundamental reason we need to keep an open mind and adopt a “why not” approach to framework development.

[StephenLPeters]

@robloo I'm a bit confused what is still broken, when you say " the sequence of invoking the events needs to be fixed now" Can you help me understand?

[robloo]

@StephenLPeters

Current code is below.

microsoft-ui-xaml/dev/Expander/Expander.cpp

Lines 104 to 115 in 4208a01

	void Expander::OnIsExpandedPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& /*args*/)
	{
	if (IsExpanded())
	{
	RaiseExpandingEvent(*this);
	}
	else
	{
	RaiseCollapsedEvent(*this);
	}
	UpdateExpandState(true);
	}

It will invoke Collapsed BEFORE the state is applied with UpdateExpandState(true); . The code as it is now would instead be for a non-existent Collapsing event. I will update code to fire the Collapsed event after state is applied.

[StephenLPeters]

Ah, yes. I agree. I think we should even wait until the storyboard has concluded to raise the event?

[robloo]

@StephenLPeters

I think we should even wait until the storyboard has concluded to raise the event?

That's certainly the most correct solution. However, it was a larger tear-up than I wanted to do so decided against it. Not only are you connecting and managing event handlers to the four storyboards (more template parts and boilerplate), but the Collapsed event would easily not get fired if the control was re-templated without the specific storyboard named template part. I'm not sure any other controls have such an error-prone restriction that could cause a fundamental event not to fire? (I don't follow the code that closely to know. I also avoid C++/WinRT whenever possible.)

Also don't forget AutomationPeer should be updated at the same time we raise the event; however, the AutomationPeer is currently handled inside UpdateExpandState().

microsoft-ui-xaml/dev/Expander/Expander.cpp

Lines 164 to 172 in 4208a01

	if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this))
	{
	auto expanderPeer = peer.as<ExpanderAutomationPeer>();
	expanderPeer->RaiseExpandCollapseAutomationEvent(
	isExpanded ?
	winrt::ExpandCollapseState::Expanded :
	winrt::ExpandCollapseState::Collapsed
	);
	}