[<install> Element] Surface new invalidReason for data errors [chromium/src : main]
Thomas Nguyen (Gerrit)
Thomas Nguyen added 3 comments![]( "Open in Gerrit")
It seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
kInstallDataError,It's the best if you can move the <install> related errors out of the generic DisableReason enum (which is used for general disable reasons)
DisableClickingIndefinitely(DisableReason::kInstallDataError);This will indefinitely disable clicking and the button will not available anymore.
Related details
- Kristin Lee
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. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
It seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
.
Lia Hiscock (Gerrit)
Lia Hiscock added 3 comments![]( "Open in Gerrit")
Thomas NguyenIt seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
.
Thanks for the thoughts!
We're definitely aware that this is likely not the long-term solution, but I believe we're on the same page with mkwst/dmurph that we're just looking for *somewhere* to surface the DataError so that developers can mess around with it during OT, while we gather feedback and continue to discuss the right longer-term solution.
We've touched on this a bit in our syncs with mike/dan, notes are below [1] if you're curious (they're a bit chaotic though)
This kind of touches on your other comment (about disabling indefinitely) - IIUC "indefinitely" just means until page reload. You're correct that DataError requires the user to click before we surface it, however it's not something that's fixable by users - the developer must go in and update the element's attributes, so that's why we felt disabling "indefinitely" fit OK (for now).
I do hear you though that it's a bit odd under the current definitions of disabling, I'm curious though if you're aware of any functional blockers to this invalidReason approach? (We're definitely open to pivoting, but just trying to avoid scope creep as we try to get this OT out the door)
kInstallDataError,It's the best if you can move the <install> related errors out of the generic DisableReason enum (which is used for general disable reasons)
Acknowledged
DisableClickingIndefinitely(DisableReason::kInstallDataError);This will indefinitely disable clicking and the button will not available anymore.
- Kristin Lee
- Thomas Nguyen
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Rob on cc as an FYI
Related details
- Kristin Lee
- Thomas Nguyen
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. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Thomas NguyenIt seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
Lia Hiscock.
Thanks for the thoughts!
We're definitely aware that this is likely not the long-term solution, but I believe we're on the same page with mkwst/dmurph that we're just looking for *somewhere* to surface the DataError so that developers can mess around with it during OT, while we gather feedback and continue to discuss the right longer-term solution.
We've touched on this a bit in our syncs with mike/dan, notes are below [1] if you're curious (they're a bit chaotic though)
This kind of touches on your other comment (about disabling indefinitely) - IIUC "indefinitely" just means until page reload. You're correct that DataError requires the user to click before we surface it, however it's not something that's fixable by users - the developer must go in and update the element's attributes, so that's why we felt disabling "indefinitely" fit OK (for now).
I do hear you though that it's a bit odd under the current definitions of disabling, I'm curious though if you're aware of any functional blockers to this invalidReason approach? (We're definitely open to pivoting, but just trying to avoid scope creep as we try to get this OT out the door)
I don't have a strong objection if there is consensus on "temporarily" putting the error in the DisabledReason of the generic <permission> class (note that we plan to phase out this class shortly).
That said, I do not see this as a viable long-term solution. Please include a TODO comment here and ensure this is addressed later.
For your information, we have explorered other members of the <permission> family; they are expected to follow a error handler pattern similar like:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_geolocation_element.idl;l=31
Related details
- Kristin Lee
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. |
Kristin Lee (Gerrit)
Kristin Lee added 3 comments![]( "Open in Gerrit")
Thomas NguyenIt seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
Lia Hiscock.
Thomas NguyenThanks for the thoughts!
We're definitely aware that this is likely not the long-term solution, but I believe we're on the same page with mkwst/dmurph that we're just looking for *somewhere* to surface the DataError so that developers can mess around with it during OT, while we gather feedback and continue to discuss the right longer-term solution.
We've touched on this a bit in our syncs with mike/dan, notes are below [1] if you're curious (they're a bit chaotic though)
This kind of touches on your other comment (about disabling indefinitely) - IIUC "indefinitely" just means until page reload. You're correct that DataError requires the user to click before we surface it, however it's not something that's fixable by users - the developer must go in and update the element's attributes, so that's why we felt disabling "indefinitely" fit OK (for now).
I do hear you though that it's a bit odd under the current definitions of disabling, I'm curious though if you're aware of any functional blockers to this invalidReason approach? (We're definitely open to pivoting, but just trying to avoid scope creep as we try to get this OT out the door)
I don't have a strong objection if there is consensus on "temporarily" putting the error in the DisabledReason of the generic <permission> class (note that we plan to phase out this class shortly).
That said, I do not see this as a viable long-term solution. Please include a TODO comment here and ensure this is addressed later.
For your information, we have explorered other members of the <permission> family; they are expected to follow a error handler pattern similar like:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_geolocation_element.idl;l=31
Thank you all for the comments and consensus here. It seems we're all on the same page about moving forward with this approach as a temporary solution for OT. I will add a TODO comment to follow up on updating this to a more appropriate solution after OT: https://issues.chromium.org/issues/481519343.
I will polish this and add tests before requesting another review. Thanks!
if (!is_registered_in_browser_process_) {
return {false, AtomicString("unsuccessful_registration")};
}Kristin LeeA working option that is unclear if recommended or not:
I've attempted to add
```
if (is_install_data_error_) {
return {false, AtomicString("install_data_error")};
}
```and in `DidInstallFailWithDataError()`, `is_install_data_error_` is set to true which works, but not sure if right approach.
No longer relevant.
// If there's an "indefinitely disabling" for any reason, return that reason.
// Otherwise, we will look into the reason of the current active timer.
for (const auto& it : clicking_disabled_reasons_) {
if (it.value == base::TimeTicks::Max()) {
return {false, DisableReasonToInvalidReasonString(it.key)};
}
}Kristin LeeNote: we're aware this is where the invalidReason is checked and surfaced.
Done
Related details
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. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Hi Thomas, I added a blink unit test and made a few small changes. If you could help with another early review, that'd be great. Thanks!
Related details
- Lia Hiscock
- Thomas Nguyen
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 3 comments![]( "Open in Gerrit")
HandleInstallDataError();can we add an actual comment here to? something like
// Disable the element to prevent future activations and
// inform the developer.
kInstallDataError,nit: add the followup TODO here as well
// TODO(crbug.com/481519343): Move DataError out of invalidReason. Revisit
// how to best surface this for <install>.
// Called when an attempted install via <install> element fails with data
// error.nit: Can we make this a bit more descriptive. maybe something like
```
// Called on activation of an <install> element with attributes
// that fail installability checks.`
```
Related details
- Kristin Lee
- Thomas Nguyen
Kristin Lee (Gerrit)
Kristin Lee added 3 comments![]( "Open in Gerrit")
can we add an actual comment here to? something like
// Disable the element to prevent future activations and
// inform the developer.
Yes, we can. Done and thanks!
nit: add the followup TODO here as well
// TODO(crbug.com/481519343): Move DataError out of invalidReason. Revisit
// how to best surface this for <install>.
Done
// Called when an attempted install via <install> element fails with data
// error.nit: Can we make this a bit more descriptive. maybe something like
```
// Called on activation of an <install> element with attributes
// that fail installability checks.`
```
- Lia Hiscock
- Thomas Nguyen
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. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Thomas NguyenIt seems your error handling refers specifically to installation failures occurring after a user initiates the process via a button click. In this scenario, we assume users will resolve the underlying issue and attempt the installation again by re-clicking the button.
If this is the case, our current approach may not be the right one. The "DisabledReason" is intended to address "pre-click" errors, determining whether an element should be enabled or disabled before any user interaction takes place.
I think, a better approach would be to introduce a separate "error" attribute linked to the "install result" event. However, implementing this change might necessitate modifications to the web API's surface.
Lia Hiscock.
Thomas NguyenThanks for the thoughts!
We're definitely aware that this is likely not the long-term solution, but I believe we're on the same page with mkwst/dmurph that we're just looking for *somewhere* to surface the DataError so that developers can mess around with it during OT, while we gather feedback and continue to discuss the right longer-term solution.
We've touched on this a bit in our syncs with mike/dan, notes are below [1] if you're curious (they're a bit chaotic though)
This kind of touches on your other comment (about disabling indefinitely) - IIUC "indefinitely" just means until page reload. You're correct that DataError requires the user to click before we surface it, however it's not something that's fixable by users - the developer must go in and update the element's attributes, so that's why we felt disabling "indefinitely" fit OK (for now).
I do hear you though that it's a bit odd under the current definitions of disabling, I'm curious though if you're aware of any functional blockers to this invalidReason approach? (We're definitely open to pivoting, but just trying to avoid scope creep as we try to get this OT out the door)
Kristin LeeI don't have a strong objection if there is consensus on "temporarily" putting the error in the DisabledReason of the generic <permission> class (note that we plan to phase out this class shortly).
That said, I do not see this as a viable long-term solution. Please include a TODO comment here and ensure this is addressed later.
For your information, we have explorered other members of the <permission> family; they are expected to follow a error handler pattern similar like:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_geolocation_element.idl;l=31
Thank you all for the comments and consensus here. It seems we're all on the same page about moving forward with this approach as a temporary solution for OT. I will add a TODO comment to follow up on updating this to a more appropriate solution after OT: https://issues.chromium.org/issues/481519343.
I will polish this and add tests before requesting another review. Thanks!
Closing this thread.
Related details
- Lia Hiscock
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Hi Thomas, I added a blink unit test and made a few small changes. If you could help with another early review, that'd be great. Thanks!
Friendly bump here @tun...@chromium.org, I'm assuming Mike is waiting for your LGTM before signing off here as well :)
Related details
- Thomas Nguyen
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. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Nguyen
| Code-Review | +1 |
1 comment
MaybeDispatchValidationChangeEvent();Please also add todo here (TODO(crbug.com/481519343) as the validationChange is not what we expected with after interaction error.
Related details
- Kristin Lee
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. |
Kristin Lee (Gerrit)
Kristin Lee added 2 comments![]( "Open in Gerrit")
Lia HiscockHi Thomas, I added a blink unit test and made a few small changes. If you could help with another early review, that'd be great. Thanks!
Friendly bump here @tun...@chromium.org, I'm assuming Mike is waiting for your LGTM before signing off here as well :)
Thanks again, Thomas!
@mk...@chromium.org - Thomas has approved. Waiting next for your approval, thanks!
Please also add todo here (TODO(crbug.com/481519343) as the validationChange is not what we expected with after interaction error.
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Lia HiscockHi Thomas, I added a blink unit test and made a few small changes. If you could help with another early review, that'd be great. Thanks!
Kristin LeeFriendly bump here @tun...@chromium.org, I'm assuming Mike is waiting for your LGTM before signing off here as well :)
Thanks again, Thomas!
@mk...@chromium.org - Thomas has approved. Waiting next for your approval, thanks!
@mk...@chromium.org, friendly bump here since Thomas has given his LGTM. Thanks!