[views-ax] Introducing setters for kExpanded and kCollapsed state [chromium/src : main]
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 1 comment![]( "Open in Gerrit")
I'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
Related details
- Divyansh Mangal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
I'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
I did thought about that but i thought having separate functions also made sense.
I can make use of the existing SetIsCollapse but will it be alright if i make single setter? something like SetIsExpandedOrCollaped state?
Related details
- Benjamin Beaudry
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 1 comment![]( "Open in Gerrit")
Divyansh MangalI'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
I did thought about that but i thought having separate functions also made sense.
I can make use of the existing SetIsCollapse but will it be alright if i make single setter? something like SetIsExpandedOrCollaped state?
That sounds good, yes. With a bool param named `is_expanded`?
Related details
- Divyansh Mangal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 1 comment![]( "Open in Gerrit")
Divyansh MangalI'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
Benjamin BeaudryI did thought about that but i thought having separate functions also made sense.
I can make use of the existing SetIsCollapse but will it be alright if i make single setter? something like SetIsExpandedOrCollaped state?
That sounds good, yes. With a bool param named `is_expanded`?
Javier and I just discussed this and decided that, from a Views author's perspective, it's easier to think about it if they have two separate functions, SetIsCollapsed and SetIsExpanded, but both functions enforce that the other state is set to false when it changes (ie. back to my original comment).
Let's keep two functions, and just enforce it in there. Wdyt?
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Divyansh MangalI'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
Benjamin BeaudryI did thought about that but i thought having separate functions also made sense.
I can make use of the existing SetIsCollapse but will it be alright if i make single setter? something like SetIsExpandedOrCollaped state?
Benjamin BeaudryThat sounds good, yes. With a bool param named `is_expanded`?
Javier and I just discussed this and decided that, from a Views author's perspective, it's easier to think about it if they have two separate functions, SetIsCollapsed and SetIsExpanded, but both functions enforce that the other state is set to false when it changes (ie. back to my original comment).
Let's keep two functions, and just enforce it in there. Wdyt?
Indeed from the views author perspective it makes sense to have separate functions.
and if we make the change in both of the functions (the mutual exclusion functionality) itself authors can simply call one single function(either SetIsCollapsed or SetIsExpanded) and that would still albeit indirectly just a single function call as well.
I am comfortable with having two separate functions as well and I will change the CLs accordingly
Related details
- Benjamin Beaudry
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Divyansh MangalI'm onboard with this interface of (ie. individual SetIsExpanded or SetIsCollapsed) functions, but one thing that is important to understand is that these two states are mutually exclusive. Can we enforce this requirement in the implementation of both functions so that we can when SetIsExpanded is called, we automatically set the collapsed state to the opposite of the boolean parameter? And ditto for SetIsCollapsed.
Thanks!
Benjamin BeaudryI did thought about that but i thought having separate functions also made sense.
I can make use of the existing SetIsCollapse but will it be alright if i make single setter? something like SetIsExpandedOrCollaped state?
Benjamin BeaudryThat sounds good, yes. With a bool param named `is_expanded`?
Divyansh MangalJavier and I just discussed this and decided that, from a Views author's perspective, it's easier to think about it if they have two separate functions, SetIsCollapsed and SetIsExpanded, but both functions enforce that the other state is set to false when it changes (ie. back to my original comment).
Let's keep two functions, and just enforce it in there. Wdyt?
Indeed from the views author perspective it makes sense to have separate functions.
and if we make the change in both of the functions (the mutual exclusion functionality) itself authors can simply call one single function(either SetIsCollapsed or SetIsExpanded) and that would still albeit indirectly just a single function call as well.
I am comfortable with having two separate functions as well and I will change the CLs accordingly
Done
Related details
- Benjamin Beaudry
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 2 comments![]( "Open in Gerrit")
bool GetIsExpanded() const;I don't think the getters are necessary. Are they? I don't see them being used in production code. The only callers I see are in view_accessibility.cc, where we can instead directly ask the cache whether we have the state or not.
void ViewAccessibility::SetIsExpanded(bool is_expanded) {Can we simplify the implementation to:
```
if (data_.HasState(ax::mojom::State::kExpanded) == is_expanded) {
CHECK(data_.HasState(ax::mojom::State::kCollapsed) != is_expanded) << "The expanded and collapsed state must be mutually exclusive.";
return;
}
SetState(ax::mojom::State::kExpanded, is_expanded);
SetState(ax::mojom::State::kCollapsed, !is_expanded); NotifyAccessibilityEvent(ax::mojom::Event::kExpandedChanged, true);
```
and the equivalent for the `SetIsCollapsed` function?
Related details
- Divyansh Mangal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 1 comment![]( "Open in Gerrit")
void SetIsExpanded(bool is_expanded);I just thought of something even better: What if SetIsExpanded and SetIsCollapsed don't take any parameters, and Views call SetIsExpanded when they want to expand it and they call SetIsCollapsed when they want it to be collapsed?
I think this would be an even better interface! Wdyt?
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
void SetIsExpanded(bool is_expanded);I just thought of something even better: What if SetIsExpanded and SetIsCollapsed don't take any parameters, and Views call SetIsExpanded when they want to expand it and they call SetIsCollapsed when they want it to be collapsed?
I think this would be an even better interface! Wdyt?
Thats a good suggestion, that will also help to remove the extra function that we are doing in the other CLs. We can simply call either SetIsExpanded or SetIsCollapsed to get the desired results.
Related details
- Benjamin Beaudry
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 3 comments![]( "Open in Gerrit")
I don't think the getters are necessary. Are they? I don't see them being used in production code. The only callers I see are in view_accessibility.cc, where we can instead directly ask the cache whether we have the state or not.
With the new implementation getters are no longer needed and hence removed the getters altogether.
Divyansh MangalI just thought of something even better: What if SetIsExpanded and SetIsCollapsed don't take any parameters, and Views call SetIsExpanded when they want to expand it and they call SetIsCollapsed when they want it to be collapsed?
I think this would be an even better interface! Wdyt?
Thats a good suggestion, that will also help to remove the extra function that we are doing in the other CLs. We can simply call either SetIsExpanded or SetIsCollapsed to get the desired results.
Implemented the new approach
void ViewAccessibility::SetIsExpanded(bool is_expanded) {Can we simplify the implementation to:
```
if (data_.HasState(ax::mojom::State::kExpanded) == is_expanded) {
CHECK(data_.HasState(ax::mojom::State::kCollapsed) != is_expanded) << "The expanded and collapsed state must be mutually exclusive.";
return;
}SetState(ax::mojom::State::kExpanded, is_expanded);
SetState(ax::mojom::State::kCollapsed, !is_expanded); NotifyAccessibilityEvent(ax::mojom::Event::kExpandedChanged, true);```
and the equivalent for the `SetIsCollapsed` function?
Done
Related details
- Benjamin Beaudry
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras voted and added 1 comment![]( "Open in Gerrit")
Votes added by Javier Contreras
| Code-Review | +1 |
1 comment
Related details
- Benjamin Beaudry
- Divyansh Mangal
- Jacques Newman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras voted and added 1 comment![]( "Open in Gerrit")
Votes added by Javier Contreras
| Code-Review | +0 |
1 comment
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);After talking with Jacques about a concern that was raised about a scenario where we don't yet want to fire the event. For instance, when the setter is called from the constructor of a view then we don't want to send the event because the state hasn't changed from one to the other.
I'm going to be implementing a solution for this in the general case, but while I do that could you add a temporary solution here?
it could look like the following:
// TODO(accessibility): Remove this workaround when the general case solution
// is implemented.
// If we don't already have the opposite state in the cache, then this is
// the initial state and we shouldn't be firing an event.
bool is_initial_state = !data_.HasState(ax::mojom::State::kCollapsed);
SetState(ax::mojom::State::kExpanded, true);
SetState(ax::mojom::State::kCollapsed, false);
if (!is_initial_state) {
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);
}
And then have similar code (including the comment) in `SetIsCollapsed`.
Related details
- Benjamin Beaudry
- Divyansh Mangal
- Jacques Newman
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 1 comment![]( "Open in Gerrit")
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);After talking with Jacques about a concern that was raised about a scenario where we don't yet want to fire the event. For instance, when the setter is called from the constructor of a view then we don't want to send the event because the state hasn't changed from one to the other.
I'm going to be implementing a solution for this in the general case, but while I do that could you add a temporary solution here?
it could look like the following:
// TODO(accessibility): Remove this workaround when the general case solution
// is implemented.
// If we don't already have the opposite state in the cache, then this is
// the initial state and we shouldn't be firing an event.
bool is_initial_state = !data_.HasState(ax::mojom::State::kCollapsed);
SetState(ax::mojom::State::kExpanded, true);
SetState(ax::mojom::State::kCollapsed, false);
if (!is_initial_state) {
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);
}And then have similar code (including the comment) in `SetIsCollapsed`.
Ignore the part about the TODO that after looking at the notification header view CL we will still need this check.
Jacques Newman (Gerrit)
Jacques Newman added 1 comment![]( "Open in Gerrit")
I'll take another look once Javier's comments are addressed.
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);Javier ContrerasAfter talking with Jacques about a concern that was raised about a scenario where we don't yet want to fire the event. For instance, when the setter is called from the constructor of a view then we don't want to send the event because the state hasn't changed from one to the other.
I'm going to be implementing a solution for this in the general case, but while I do that could you add a temporary solution here?
it could look like the following:
// TODO(accessibility): Remove this workaround when the general case solution
// is implemented.
// If we don't already have the opposite state in the cache, then this is
// the initial state and we shouldn't be firing an event.
bool is_initial_state = !data_.HasState(ax::mojom::State::kCollapsed);
SetState(ax::mojom::State::kExpanded, true);
SetState(ax::mojom::State::kCollapsed, false);
if (!is_initial_state) {
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);
}And then have similar code (including the comment) in `SetIsCollapsed`.
Ignore the part about the TODO that after looking at the notification header view CL we will still need this check.
Just to clarify, should i go ahead and add this work around or wait for your solution to get merged??
If I am correct we won't need this change once we have your solution
Related details
- Javier Contreras
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);Javier ContrerasAfter talking with Jacques about a concern that was raised about a scenario where we don't yet want to fire the event. For instance, when the setter is called from the constructor of a view then we don't want to send the event because the state hasn't changed from one to the other.
I'm going to be implementing a solution for this in the general case, but while I do that could you add a temporary solution here?
it could look like the following:
// TODO(accessibility): Remove this workaround when the general case solution
// is implemented.
// If we don't already have the opposite state in the cache, then this is
// the initial state and we shouldn't be firing an event.
bool is_initial_state = !data_.HasState(ax::mojom::State::kCollapsed);
SetState(ax::mojom::State::kExpanded, true);
SetState(ax::mojom::State::kCollapsed, false);
if (!is_initial_state) {
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);
}And then have similar code (including the comment) in `SetIsCollapsed`.
Divyansh MangalIgnore the part about the TODO that after looking at the notification header view CL we will still need this check.
Just to clarify, should i go ahead and add this work around or wait for your solution to get merged??
If I am correct we won't need this change once we have your solution
ohh i see, after seeing the notification header view CL comments I get what u mean. We will require this change.
I will corporate this in my new patchset
Related details
- Javier Contreras
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Also Javier, the not operator (!) was a little confusing in your solution so i removed it in my solution. It should still work 😊
Related details
- Javier Contreras
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 3 comments![]( "Open in Gerrit")
// check to see if the expanded state is already set, if already set no neednit: Check
Your approach without `!` is fine but then we should change the variable name.
With the `!` it used to flow fine because the if statement would be read "if its not initial state then notify", and now it reads "if its initial state notify" which is incorrect, we only need to notify if we are not in intial state.
You can keep your approach and remove the ! but change the var name to maybe "should_notify" and add a comment of why we want to check for this. (If we go from no state to a state we shouldnt notify).
// check to see if the collapsed state is already set, if already set no needditto
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 3 comments![]( "Open in Gerrit")
// check to see if the expanded state is already set, if already set no needDivyansh Mangalnit: Check
Done
NotifyEvent(ax::mojom::Event::kExpandedChanged, true);Done
// check to see if the collapsed state is already set, if already set no needDivyansh Mangalditto
Done
Related details
- Javier Contreras
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras voted and added 1 comment![]( "Open in Gerrit")
Votes added by Javier Contreras
- Divyansh Mangal
- Jacques Newman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
1 comment
lgtm!!
Related details
- Divyansh Mangal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[views-ax] Introducing setters for kExpanded and kCollapsed state
Follow-up CL makes use of these.
This CL is part of the ViewsAX project:
https://docs.google.com/document/d/1Ku7HOyDsiZem1yaV6ccZ-tz3lO2XR2NEcm8HjR6d-VY/edit#heading=h.ke1u3utej413
- M ui/views/accessibility/view_accessibility.cc
- M ui/views/accessibility/view_accessibility.h
Code-Review: +1 by Javier Contreras, +1 by Jacques Newman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |