Don't fire change event during mutations affecting disabledness [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
bool ancestor_disabled_state_change_due_to_mutation_ = false;Joey Arharnit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming `ancestor_disabled_state_change_due_to_mutation_` to `is_ancestor_disabled_state_change_due_to_mutation_` or `was_ancestor_disabled_state_changed_due_to_mutation_` to match the style guide and other boolean members in this class.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Some alternate things I could do with this patch:
- queue the events to fire after a microtask or something, but this seems like such an edge case that it isn't necessary
- pass the flag around in a lot of methods instead of using a member variable with AutoReset. ai convinced me that it would be more ugly, but im happy to actually implement it to see if it would really be worse or not
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 4 comments![]( "Open in Gerrit")
Some alternate things I could do with this patch:
- queue the events to fire after a microtask or something, but this seems like such an edge case that it isn't necessary
- pass the flag around in a lot of methods instead of using a member variable with AutoReset. ai convinced me that it would be more ugly, but im happy to actually implement it to see if it would really be worse or not
I'm not particularly bothered by the second one. I have mixed feelings about the first one, though. I agree it's an edge case, but I also wonder if it's important to callers to maintain these sorts of invariants. That said, these are extreme edge cases and it's probably fine.
(The one other comment is that I don't think microtask timing would be the right thing since I think that should be specific to things that involve promises.)
Fixed: 523756329, 523748081, 523737685is it worth adding a test for https://issues.chromium.org/523737685 as well?
};I don't like "mutation" as a name for this because both the things that you're counting "as a mutation" and the things that you're not are, I think, really all different kinds of mutations.
I think the basic distinction you're trying to make here is whether the change is due to elements moving or whether it's due to attributes changing. (Is that correct?) Maybe that could help with figuring out a new name?
It's possible that it would end up being more self documenting if it were an `enum class` rather than a `bool`, since then the type name could be `...Reason` and the values could be self explanatory and not need comments.
// the descendants.Is it possible that you could `CHECK()` that this doesn't change a `true` to `false`? (Same for `ListedElement` -- the override here just to call the base class `DisabledAttributeChanged` instead of the fieldset implementation is starting to get annoying!)
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 4 comments![]( "Open in Gerrit")
David BaronSome alternate things I could do with this patch:
- queue the events to fire after a microtask or something, but this seems like such an edge case that it isn't necessary
- pass the flag around in a lot of methods instead of using a member variable with AutoReset. ai convinced me that it would be more ugly, but im happy to actually implement it to see if it would really be worse or not
I'm not particularly bothered by the second one. I have mixed feelings about the first one, though. I agree it's an edge case, but I also wonder if it's important to callers to maintain these sorts of invariants. That said, these are extreme edge cases and it's probably fine.
(The one other comment is that I don't think microtask timing would be the right thing since I think that should be specific to things that involve promises.)
I changed it to pass the flag around. How does it look?
Fixed: 523756329, 523748081, 523737685is it worth adding a test for https://issues.chromium.org/523737685 as well?
That bug is about doing a dom mutation inside the change event of an <input type=number>, right? Isn't that case already covered by the new moveBefore-iframe-crash.html and moveBefore-legend-input-crash.html tests?
I don't like "mutation" as a name for this because both the things that you're counting "as a mutation" and the things that you're not are, I think, really all different kinds of mutations.
I think the basic distinction you're trying to make here is whether the change is due to elements moving or whether it's due to attributes changing. (Is that correct?) Maybe that could help with figuring out a new name?
It's possible that it would end up being more self documenting if it were an `enum class` rather than a `bool`, since then the type name could be `...Reason` and the values could be self explanatory and not need comments.
i changed the flag to an enum and called the "mutation" case "kFieldsetChildrenChanged"
// the descendants.Is it possible that you could `CHECK()` that this doesn't change a `true` to `false`? (Same for `ListedElement` -- the override here just to call the base class `DisabledAttributeChanged` instead of the fieldset implementation is starting to get annoying!)
I added a check that the flag is false at the start of both methods. Does that address your concern?
(this is obsolete if we go with the latest patchset, which removes the autoreset)
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 4 comments![]( "Open in Gerrit")
Answers to all the open questions below though I didn't quite do a full re-review of all the changes since my last review.
David BaronSome alternate things I could do with this patch:
- queue the events to fire after a microtask or something, but this seems like such an edge case that it isn't necessary
- pass the flag around in a lot of methods instead of using a member variable with AutoReset. ai convinced me that it would be more ugly, but im happy to actually implement it to see if it would really be worse or not
Joey ArharI'm not particularly bothered by the second one. I have mixed feelings about the first one, though. I agree it's an edge case, but I also wonder if it's important to callers to maintain these sorts of invariants. That said, these are extreme edge cases and it's probably fine.
(The one other comment is that I don't think microtask timing would be the right thing since I think that should be specific to things that involve promises.)
I changed it to pass the flag around. How does it look?
Oh, I thought the old way was fine (though I guess "not particularly bothered" wasn't the clearest way to say that), but this way seems fine too.
Fixed: 523756329, 523748081, 523737685Joey Arharis it worth adding a test for https://issues.chromium.org/523737685 as well?
That bug is about doing a dom mutation inside the change event of an <input type=number>, right? Isn't that case already covered by the new moveBefore-iframe-crash.html and moveBefore-legend-input-crash.html tests?
That one looks like it has something to do with temporal (date-time) inputs such as `<input type=date>`. I didn't see any tests for those.
// the descendants.Joey ArharIs it possible that you could `CHECK()` that this doesn't change a `true` to `false`? (Same for `ListedElement` -- the override here just to call the base class `DisabledAttributeChanged` instead of the fieldset implementation is starting to get annoying!)
I added a check that the flag is false at the start of both methods. Does that address your concern?
(this is obsolete if we go with the latest patchset, which removes the autoreset)
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Fixed: 523756329, 523748081, 523737685Joey Arharis it worth adding a test for https://issues.chromium.org/523737685 as well?
David BaronThat bug is about doing a dom mutation inside the change event of an <input type=number>, right? Isn't that case already covered by the new moveBefore-iframe-crash.html and moveBefore-legend-input-crash.html tests?
That one looks like it has something to do with temporal (date-time) inputs such as `<input type=date>`. I didn't see any tests for those.
Ah ok so yes that report does talk about temporal inputs, but the provided PoC actually uses <input type=number> instead of a temporal input type. It explains in the first comment after the OP:
Note on the original report: the report names MultipleFieldsTemporalInputTypeView::DisabledAttributeChanged() (<input type=date>). That method does call ReleaseCapture(), but the multiple-fields shadow tree never instantiates a SpinButtonElement (CreateShadowSubtree() only adds DateTimeEditElement and PickerIndicatorElement), so GetSpinButtonElement() is always nullptr there. The same bug class is reachable via the sibling TextFieldInputType::DisabledAttributeChanged() path used by <input type=number>, which is what this POC exercises.
with this in mind, I will replace the call to ReleaseCapture with a CHECK that there is no spin button element in a followup: https://chromium-review.googlesource.com/c/chromium/src/+/7985070
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
Fixed: 523756329, 523748081, 523737685Joey Arharis it worth adding a test for https://issues.chromium.org/523737685 as well?
David BaronThat bug is about doing a dom mutation inside the change event of an <input type=number>, right? Isn't that case already covered by the new moveBefore-iframe-crash.html and moveBefore-legend-input-crash.html tests?
Joey ArharThat one looks like it has something to do with temporal (date-time) inputs such as `<input type=date>`. I didn't see any tests for those.
Ah ok so yes that report does talk about temporal inputs, but the provided PoC actually uses <input type=number> instead of a temporal input type. It explains in the first comment after the OP:
Note on the original report: the report names MultipleFieldsTemporalInputTypeView::DisabledAttributeChanged() (<input type=date>). That method does call ReleaseCapture(), but the multiple-fields shadow tree never instantiates a SpinButtonElement (CreateShadowSubtree() only adds DateTimeEditElement and PickerIndicatorElement), so GetSpinButtonElement() is always nullptr there. The same bug class is reachable via the sibling TextFieldInputType::DisabledAttributeChanged() path used by <input type=number>, which is what this POC exercises.
with this in mind, I will replace the call to ReleaseCapture with a CHECK that there is no spin button element in a followup: https://chromium-review.googlesource.com/c/chromium/src/+/7985070
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/60867.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
Hmmm, it looks like my Code-Review+1 got reset because https://crrev.com/c/7921190 got included into this CL.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Hmmm, it looks like my Code-Review+1 got reset because https://crrev.com/c/7921190 got included into this CL.
This patch had a merge conflict with that one, which I resolved by adding the new parameter to the disabled changed method on the select element.
I don't think this patch "includes" the other one by duplicating its changes or something, does it?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
Joey ArharHmmm, it looks like my Code-Review+1 got reset because https://crrev.com/c/7921190 got included into this CL.
This patch had a merge conflict with that one, which I resolved by adding the new parameter to the disabled changed method on the select element.
I don't think this patch "includes" the other one by duplicating its changes or something, does it?
ok, I guess I shouldn't trust Gerrit's diff between revisions. (I usually don't, but I did today and it was a mistake.)
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |