[<login>] Extend the LocalFrame to query <login>s elements [chromium/src : main]
Sam Goto (Gerrit)
Sam Goto added 3 comments![]( "Open in Gerrit")
GetFederations() => (array<Federation> federations);Sam GotoTechnically per https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.jv90dicow9kc, these new methods need to have at least one non-test usage, but they're only really exercised in browser tests and no production path.
https://chromium-review.googlesource.com/c/chromium/src/+/7539587 probably has some "production"-paths. Would it be possible to chain the next CL on top of this one that shows how these are really going to be used? (or point out which parts of that aforementioned CL I should look at to get a feel for how these mojo methods would be used outside of tests)?
Sam GotoI'm working on a CL that branches of this one that can give a concrete sense of how this CL is intended to be used.
I'll report back momentarily when I set that up.
Done.
I rebased this and also kicked off two CLs that branches from this one here to give you a concrete sense of how I'm planning to use this.
Something I've been wondering is if LocalFrame is in fact the right architectural choice, rather than this option I described here:
https://chromium-review.googlesource.com/c/chromium/src/+/7543033/comment/c06762eb_895b0ea9/
WDYT?
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Sam GotoI find it strange to add mojo IPC methods without actual usage. +1 on dom@'s comment that we should see a bit more of the picture and linking to the future CLs in this one is needed.
Sam GotoSGTM
Let me rebase some of these changes so that it gets easier to see how this (specifically, the Mojom browser/renderer API changes) is intended to be used.
I rebased this CL onto another one to decrease its scope to only contain the LocalFrame IPC change.
As requested, the way we are planning to use this is here:
https://chromium-review.googlesource.com/c/chromium/src/+/7563244
And as a consequence here too:
https://chromium-review.googlesource.com/c/chromium/src/+/7563245
Does that help give you a sense of how we are planning to use this?
On a related note, another architectural option that I was considering was to expose the <login> elements via the GetAIPageContent method (rather than the LocalFrame mojom) which is implemented by the AIPageContentAgent via the ai_page_content.mojom.
Because we are only planning to use this for agentic browsing, I think it probably fits there better, than the more broadly available LocalFrame.
Thoughts?
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Christian BiesingerWhat is this supposed to do besides just dispatching a DOM event? What Blink actions are supposed to happen, or what does this indicate?
Dominic FarolinoThat's it -- the website is supposed to listen to the event to receive the token that the IDP sent to the browser.
In a regular FedCM call, there's a promise returned from navigator.credentials.get. But in this case, because it's declarative, we instead deliver the token through this event. (Per https://www.w3.org/TR/design-principles/#state-and-subclassing, the token is stored on the federation element, not on the event object)
Christian BiesingerAh so Blink literally isn't supposed to take any action besides storing the state, and telling the developer about it here so they can grab it off of the element?
Dominic FarolinoYes that's correct
Sam GotoWhat "type" should `token` be? It's a string, but can it be described by any more specific type?
I have moved this change to this other CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7552493
And replaced `token` for `credential` (and made it currently a `DOMString` -- I'll later want to make it a `Credential` but that requires me to make /core depend on /modules, so I'll do separately).
I'm going to resolve this here and take this discussion to this CL: https://chromium-review.googlesource.com/c/chromium/src/+/7552493
Related details
- Christian Biesinger
- Dominic Farolino
- Nasko Oskov
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 5 comments![]( "Open in Gerrit")
// A unique ID for this element, valid for the lifetime of the document.`DOMNodeId`
for (Node& node : NodeTraversal::DescendantsOf(*frame_->GetDocument())) {Can't you do `for (HTMLLoginElement& login : Traversal<HTMLLoginElement>::DescendantsOf(*frame_->GetDocument())`? Like you did in https://chromium-review.googlesource.com/c/chromium/src/+/7543033/14/third_party/blink/renderer/core/frame/local_frame_mojo_handler.cc ...
DOMNodeId dom_id() const {Can't you use GetDomNodeId() which is inherited from Node?
class CORE_EXPORT HTMLLoginElement : public HTMLElement {You should probably override WillRespondToMouseClickEvents() and IsInteractiveContent() and return true if there's a <credential> child
Difficult question: Should you override IsFocusableState/ShouldHaveFocusAppearance
Related details
- Dominic Farolino
- Nasko Oskov
- Sam Goto
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Sam GotoI find it strange to add mojo IPC methods without actual usage. +1 on dom@'s comment that we should see a bit more of the picture and linking to the future CLs in this one is needed.
Sam GotoSGTM
Let me rebase some of these changes so that it gets easier to see how this (specifically, the Mojom browser/renderer API changes) is intended to be used.
I rebased this CL onto another one to decrease its scope to only contain the LocalFrame IPC change.
As requested, the way we are planning to use this is here:
https://chromium-review.googlesource.com/c/chromium/src/+/7563244
And as a consequence here too:
https://chromium-review.googlesource.com/c/chromium/src/+/7563245
Does that help give you a sense of how we are planning to use this?
On a related note, another architectural option that I was considering was to expose the <login> elements via the GetAIPageContent method (rather than the LocalFrame mojom) which is implemented by the AIPageContentAgent via the ai_page_content.mojom.
Because we are only planning to use this for agentic browsing, I think it probably fits there better, than the more broadly available LocalFrame.
Thoughts?
Ah, I did indeed try exposing this in the AnnotatedPageContent proto and I think it might not be the right fit: it is expensive to compute (it is used in the zero state suggestion for AI agents) and has a lot of unrelated things. Re-using the cached one seems incorrect (e.g. maybe the DOM changes from the initial page), so I figured I should report back that I think that exposing this through the LocalFrame to dynamically fetch the <login> elements is the best alternative I found so far.
Related details
- Dominic Farolino
- Nasko Oskov
Sam Goto (Gerrit)
Sam Goto added 9 comments![]( "Open in Gerrit")
// A unique ID for this element, valid for the lifetime of the document.Sam Goto`DOMNodeId`
Done
GetFederations() => (array<Federation> federations);Sam GotoTechnically per https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.jv90dicow9kc, these new methods need to have at least one non-test usage, but they're only really exercised in browser tests and no production path.
https://chromium-review.googlesource.com/c/chromium/src/+/7539587 probably has some "production"-paths. Would it be possible to chain the next CL on top of this one that shows how these are really going to be used? (or point out which parts of that aforementioned CL I should look at to get a feel for how these mojo methods would be used outside of tests)?
Sam GotoI'm working on a CL that branches of this one that can give a concrete sense of how this CL is intended to be used.
I'll report back momentarily when I set that up.
Done.
I rebased this and also kicked off two CLs that branches from this one here to give you a concrete sense of how I'm planning to use this.
Something I've been wondering is if LocalFrame is in fact the right architectural choice, rather than this option I described here:
https://chromium-review.googlesource.com/c/chromium/src/+/7543033/comment/c06762eb_895b0ea9/
WDYT?
I took a pass at trying to extend the AnnotatedPageContent proto and learned that it isn't such a great option. I'll resolve this comment because I have added two CLs that show how this specific mojom IPC is intended to be used.
DispatchFederationEvent(mojo_base.mojom.UnguessableToken element_id, string token);Sam GotoI find it strange to add mojo IPC methods without actual usage. +1 on dom@'s comment that we should see a bit more of the picture and linking to the future CLs in this one is needed.
Sam GotoSGTM
Let me rebase some of these changes so that it gets easier to see how this (specifically, the Mojom browser/renderer API changes) is intended to be used.
Sam GotoI rebased this CL onto another one to decrease its scope to only contain the LocalFrame IPC change.
As requested, the way we are planning to use this is here:
https://chromium-review.googlesource.com/c/chromium/src/+/7563244
And as a consequence here too:
https://chromium-review.googlesource.com/c/chromium/src/+/7563245
Does that help give you a sense of how we are planning to use this?
On a related note, another architectural option that I was considering was to expose the <login> elements via the GetAIPageContent method (rather than the LocalFrame mojom) which is implemented by the AIPageContentAgent via the ai_page_content.mojom.
Because we are only planning to use this for agentic browsing, I think it probably fits there better, than the more broadly available LocalFrame.
Thoughts?
Ah, I did indeed try exposing this in the AnnotatedPageContent proto and I think it might not be the right fit: it is expensive to compute (it is used in the zero state suggestion for AI agents) and has a lot of unrelated things. Re-using the cached one seems incorrect (e.g. maybe the DOM changes from the initial page), so I figured I should report back that I think that exposing this through the LocalFrame to dynamically fetch the <login> elements is the best alternative I found so far.
Ok, I'm going to resolve this, because I think I have addressed the original concern: there are two CLs that build on top of this one that make use of the Mojom IPC that I'm introducing here.
I have also explored using different mojom IPCs (the accessiblity tree and the AnnotatedPageContent proto, specifically, and believe that LocalFrame has better trade-ofs).
for (Node& node : NodeTraversal::DescendantsOf(*frame_->GetDocument())) {Can't you do `for (HTMLLoginElement& login : Traversal<HTMLLoginElement>::DescendantsOf(*frame_->GetDocument())`? Like you did in https://chromium-review.googlesource.com/c/chromium/src/+/7543033/14/third_party/blink/renderer/core/frame/local_frame_mojo_handler.cc ...
Done
this empty line looks wrong
Done
String token_;Sam GotoCan you add some documentation describing what these members are, and how they relate to the concepts in the explainer?
Done
attribute EventHandler ontoken;Sam GotoPer https://github.com/w3ctag/design-principles/pull/612, I think you'll want to add this to global_event_handlers.idl. I'd go with that approach in the meantime, and revert back to this one if my PR turns out to be in the wrong direction.
Sam GotoIn the process of generalizing <login> to be able to handle other <credential> types I had to use a more generic event rather than "token" (which is more federation specific) and ended up reusing "onselect" which I think is already in the global event handlers. We can choose a different event type in the future, but for now, I think this works. WDYT?
Also, addressed in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/7552493
Just to close on this thread, I'm using the "oncomplete" handler in this CL and added it to the global event handlers declaration: https://chromium-review.googlesource.com/c/chromium/src/+/7552493
Can't you use GetDomNodeId() which is inherited from Node?
Yes, done.
class CORE_EXPORT HTMLLoginElement : public HTMLElement {You should probably override WillRespondToMouseClickEvents() and IsInteractiveContent() and return true if there's a <credential> child
Difficult question: Should you override IsFocusableState/ShouldHaveFocusAppearance
I addressed this in this patch:
Resolving here, let me know there if this doesn't seem right to you.
Related details
- Christian Biesinger
- Dominic Farolino
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |