Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-view-transitions-1] domUpdated isn't the right term #8144

Closed
annevk opened this issue Nov 29, 2022 · 18 comments · Fixed by #8330
Closed

[css-view-transitions-1] domUpdated isn't the right term #8144

annevk opened this issue Nov 29, 2022 · 18 comments · Fixed by #8330
Labels
css-view-transitions-1 View Transitions; Bugs only Needs Edits

Comments

@annevk
Copy link
Member

annevk commented Nov 29, 2022

Apart from a couple of events that are prefixed with DOM*, we don't really expose DOM as a term to web content and I think we should keep it that way.

Either keep this somewhat more abstract or go with nodeTreeChanged or some such.

cc @jakearchibald @domenic

@annevk annevk added the css-view-transitions-1 View Transitions; Bugs only label Nov 29, 2022
@jakearchibald
Copy link
Contributor

jakearchibald commented Nov 29, 2022

The developer provides a callback that updates the state of the page in some way that forms the 'after' state of a transition.

domUpdated relates to the success/failure of that update.

It doesn't need to be a tree change. It can be a class name changing, an inline style, a CSSOM change etc etc.

Some other name ideas:

  • stateUpdated
  • viewUpdated
  • updateHandled
  • changeHandled

I'll try and think up more options.

Although, I think domUpdated works pretty well. The developer updates the DOM, therefore the DOM is updated.

@annevk
Copy link
Member Author

annevk commented Nov 30, 2022

It sounds like you and I have a different understanding of what DOM means. If the overall feature is called view transitions, viewUpdated makes some sense to me.

@jakearchibald jakearchibald changed the title domUpdated isn't the right term [css-view-transitions-1] domUpdated isn't the right term Nov 30, 2022
@jakearchibald
Copy link
Contributor

The problem with viewUpdated, is… what the user sees isn't updated yet.

Here's a diagram I've been working on to go in the spec that tries to explain the phases of a view transition.

out.mp4

The three promises map to those phases:

  • domUpdated - once "developer updates DOM"
  • ready - once "transition layer populated"
  • finished - once "transition layer cleared"

I had assumed that a change to styling counted as a DOM change, as text content of a style would have changed, or a rule in CSSOM would have changed etc etc. My assumption is that CSSOM was a subset of the DOM, since it's properties that hang off nodes that are part of the document.

viewUpdated seems wrong, because the user may not see it yet. It's the document model that's updated.

@annevk
Copy link
Member Author

annevk commented Nov 30, 2022

How about something like stateChanged?

I think there's quite distinct ideas about the scope of the term DOM and as such I'd rather also not have it as a term-of-art in APIs or even prose.

@jakearchibald
Copy link
Contributor

I thought you were saying 'state' was problematic in #8143?

@jakearchibald
Copy link
Contributor

Ah, ok, I think I understand what you mean in #8143 now.

stateChanged is vague, and clashes with state updates in frameworks (which happen before DOM updates). I'll have a think.

@jakearchibald
Copy link
Contributor

@vmpstr suggested the name should reflect that it's linked to the callback passed to startViewTransition.

So:

  • callbackDone
  • updateCallbackDone

I prefer the latter, in case we add other callbacks in future.

@tbondwilkinson
Copy link

From an API perspective:

startViewTransition(callback);

There's nothing about the API shape right now that implies a name for the callback parameter passed to startViewTransition. The spec does call it a UpdateDOMCallback, but that doesn't appear in the API. If Anne has an issue with DOM appearing in the API, perhaps the spec should also be changed to a more neutral name?

You could consider using "render," as I believe that's a term unrelated to DOM but reflects that the operation is to change the rendered content.

renderViewCallback, perhaps.

@jakearchibald
Copy link
Contributor

There's nothing about the API shape right now that implies a name for the callback

There might in future, if the API changes to a dictionary to allow for options, and perhaps other callbacks.

perhaps the spec should also be changed to a more neutral name?

Yep, it will be, to reflect the name we decide here. But the developer-facing name is more important.

You could consider using "render,"

See the video in #8144 (comment). I don't think "render" is a good choice because the change is not rendered visually.

@tbondwilkinson
Copy link

updateCallbackDone just feels so generic. :( Is there really no verb commonly used for the modification of HTML elements on the page?

@annevk
Copy link
Member Author

annevk commented Dec 5, 2022

Mutations (see MutationObserver), but those exclude changes to style sheets.

@jakearchibald
Copy link
Contributor

The nice thing about updateCallbackDone is it's clear it's related to whatever you did in the callback.

If you mutate the DOM, change a style, and then (for whatever reason) await something unrelated in the callback, a name like domUpdated, viewUpdated, mutated etc etc might suggest that the promise is related to a subset of things you did in the callback, but it isn't.

@khushalsagar
Copy link
Member

I'm good with updateCallbackDone as well. This promise is tightly coupled with the execution of the developer provided callback in startViewTransition so the name makes that obvious.

@ydaniv
Copy link
Contributor

ydaniv commented Jan 5, 2023

Perhaps it would be more correct to say targetReady? Since it's not just updating the DOM that needs to be done. The developer may want to preload other resources for the final state to be visually ready, etc.
Currently there's no mention in the spec of a "target" but there are always a source and target views for a transition.

@khushalsagar
Copy link
Member

Posting a summary of all the options brought up on the issue so folks can quickly skim through and pick the one they most agree with.

  • Option 1 builds on domUpdated: "Updated" is the verb to indicate state has changed and "dom" is the noun to refer to this state.

    For the prefix we want something which encompasses any update to the DOM (node state, tree state, computed style, loading state, canvas update etc). The options are:

    • stateUpdated : state can be a bit generic.
    • viewUpdated : We landed on "view" transitions with the idea that the transition is between the user visible "views" of the page. Since the callback doesn't change visuals for the user, view can be ambiguous.
    • renderingUpdated : Similar to above, rendering hasn't been updated when this promise resolves.
    • nodeTreeUpdated : Doesn't encompass all the ways in which the page state can change.
    • targetUpdated : Follows the nomenclature of source and target views to refer to new page state as "target". Not great since the spec has settled on old and new views for this.

    There are a few other options for the suffix too:

    • domReady : We already have a "ready" promise to indicate the pseudo-DOM has been generated.
    • domChanged
    • domMutated
    • domUpdateHandled : Don't see the advantage of this vs domUpdated, its just longer.
    • domChangeHandled : Same as above.
  • Option 2 is to tie the promise to the callback passed by the developer in startViewTransition. The callback is currently named UpdateDOMCallback and the promise will updateDOMCallbackDone or updateDOMCallbackHandled. So it seems like we still need a noun + verb combination as the options above.

    A related options is to skip the noun. So you'd just have updateCallback (see verb options for update above) and the promise will be callback name + "done/handled", like updateCallbackDone or updateCallbackHandled.

Please add in case I missed something.


My take is that the pattern for nounVerbCallback for the callback and nounVerb for the promise is good. And update is fine for the verb. The discussion started with avoiding the use of "dom" as the noun. And from the options above, I'd go with state. It's generic but intentionally so since we want to encompass a broad type of updates. So, updateStateCallback and stateUpdated.

If we can't agree on a noun then next best option is to drop it and go with updateCallback and updateCallbackDone.

@khushalsagar
Copy link
Member

Conclusion from a discussion just now, we've got 2 proposals that we'd like the group's feedback on:

  1. updateStateCallback for the callback and stateUpdated for the corresponding promise.
  2. updateCallback for the callback and updateCallbackDone for the corresponding promise.

No strong preference on either of the options above.

@ydaniv
Copy link
Contributor

ydaniv commented Jan 10, 2023

I'd prefer 2.

To me "state" sounds very vague. I do however understand that the callback argument is the "update callback".

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-view-transitions-1] domUpdated isn't the right term, and agreed to the following:

  • RESOLVED: adopt suggested change to rename to `updateCallbackDone
The full IRC log of that discussion <JakeA> View transitions wraps a state change - we attempt generate a transition from that state change
<JakeA> The developer provides a callback where they do that change, usually by modifying the DOM in some way. It can be async.
<JakeA> Sometimes you just want to know when the state change is complete
<JakeA> We provided a promise for that,`domUpdated`, which fulfills when the promise returned by the callback fulfills
<JakeA> Anne said "we don't really expose DOM as a term to web content", so the name is bad
<JakeA> So, we need a new name
<JakeA> Couple of options: `stateUpdated` - avoids using DOM, but a bit vague, might be confusing due to frameworks use of 'state'.
<JakeA> Vlad suggested that, since the promise resolves along with the callback, we name it `updateCallbackDone`.
<JakeA> I like that, since if the developer does stuff other than DOM updating in the callback, the promise name still makes sense.
<JakeA> Anne is happy with that.
<bramus> JakeA: I can take up this one. posted a bunch of messages above
<TabAtkins> +1 to suggested name change, it's clear and tied directly to what's actually happening, and I think will make sense for an author.
<masonf> `DOMContentLoaded` ?
<bramus> JakeA: I guess we are saying: any objections?
<bramus> astearns: not knowing what the callback is named that seems vague. what is it?
<bramus> JakeA: it does not have an developer visible name. authors pass it in as argument
<bramus> JakeA: `updateCallback` and then `updateCallbackDone`
<florian> have not followed deeply, but seems reasonable
<bramus> TabAtkins: I like it. makes sense and matches spec language
<bramus> bramus: +1
<bramus> astearns: proposed resolution: adopt suggested change to rename to `updateCallbackDone `
<bramus> RESOLVED: adopt suggested change to rename to `updateCallbackDone

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2023
This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
aarongable pushed a commit to chromium/chromium that referenced this issue Jan 19, 2023
This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 19, 2023
This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 19, 2023
This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 19, 2023
This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 1, 2023
…one, a=testonly

Automatic update from web-platform-tests
VT: Rename domUpdated to updateCallbackDone

This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}

--

wpt-commits: ad7fcb93363494d53af41c5ef25230e07c5b1d42
wpt-pr: 38045
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 3, 2023
…one, a=testonly

Automatic update from web-platform-tests
VT: Rename domUpdated to updateCallbackDone

This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}

--

wpt-commits: ad7fcb93363494d53af41c5ef25230e07c5b1d42
wpt-pr: 38045
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 1, 2024
…one, a=testonly

Automatic update from web-platform-tests
VT: Rename domUpdated to updateCallbackDone

This patch renames the promise as a result of the following resolution:
w3c/csswg-drafts#8144

R=khushalsagar@chromium.org, bokan@chromium.org

Change-Id: Ic0d4f3af2569462ccbf7e30fe258f0e52b21286e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178008
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1094162}

--

wpt-commits: ad7fcb93363494d53af41c5ef25230e07c5b1d42
wpt-pr: 38045
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-view-transitions-1 View Transitions; Bugs only Needs Edits
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants