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

Add support for CSS reading-flow #10613

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

dizhang168
Copy link
Contributor

@dizhang168 dizhang168 commented Sep 10, 2024

The CSSWG resolved to add the new CSS property reading-flow: (w3c/csswg-drafts#7387, spec). Chrome has been working on a prototype for how to change the sequential focus navigation order within a container that has reading-flow.
This PR specs the new CSS property reading-flow per proposal described at #10407.

(See WHATWG Working Mode: Changes for more details.)


/index.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domfarolino
Copy link
Member

By the way, in the OP you have "At least two implementers are interested (and none opposed):" checked, but neither of the two linked standards positions have been resolved yet, so I'd uncheck that unless you you have other cross-browser support to point to.

@dizhang168
Copy link
Contributor Author

Thanks for the first pass!

By the way, in the OP you have "At least two implementers are interested (and none opposed):" checked, but neither of the two linked standards positions have been resolved yet, so I'd uncheck that unless you you have other cross-browser support to point to.

Thanks for the call out. There has been conversation in meetings and implementers from non-chromium browsers are interested and none opposed. But I will wait for their comments on the official position issues before checking the checkbox.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@dizhang168
Copy link
Contributor Author

Status update: I have addressed all the formatting comments. @domfarolino and other reviewers proposed more suggestions and changes at TPAC 2024. I will go back to working on the proposal before making more changes to this spec PR.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing this! It looks quite clear. One thing I'm missing from the surrounding discussion we had are the examples. I thought those would be added to the specification in some fashion.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor

@fantasai fantasai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML spec shouldn't be written in a way that depends on specific CSS properties, because these can change--we shouldn't need to change the HTML spec to change how CSS calculates the reading order.

HTML should use a generic concept of a "rendering-defined sibling reading order" or something like that, which both specs can refer to. Then CSS can say how that order is calculated, and HTML can say how it interacts with HTML features and algorithms.

There have been multiple different proposals for how CSS calculates reading order, only some of which depend on values of reading-flow. For example, we have discussed a property on the items (reading-order), a property on the container (reading-flow), a variety of different types of values for each, and the possibility of even changing behavior for effects like dense packing to perform layout-based reordering by default (no specific property). All of this is out of scope for the HTML spec, and shouldn't be locked down into it as if it were the defining spec for these features.

@dizhang168
Copy link
Contributor Author

dizhang168 commented Nov 12, 2024

Thanks for the feedback. I will update this spec to remove any specific CSS properties and instead refer to CSS Display 4 for rendering-defined sibling reading order and reading flow container.

Next, I will refactor this PR accordingly to have only the reading flow order in the context of a focus navigation scope and illustrate the steps with a detailed example.

@dizhang168
Copy link
Contributor Author

I have updated the PR with a thorough example for the reading flow order algorithm without referring directly to any 'reading-flow' property. Would appreciate your review, thanks.
@domfarolino @annevk @emilio

Note: I am working with @rachelandrew to update the definitions in css-display-4.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Dec 18, 2024
@annevk
Copy link
Member

annevk commented Dec 18, 2024

Adding "do not merge yet" until we have confirmation that the CSS side is table.

@dizhang168
Copy link
Contributor Author

dizhang168 commented Dec 18, 2024

CSS side is stable and the FPWD for CSS Display 4 is expected to be published this week.
#10613 (review)

Edit: Linked the wrong comment: w3c/transitions#685 (comment)

@annevk
Copy link
Member

annevk commented Dec 18, 2024

My best understanding is that the CSS WG does not consider it stable. That's what they also said during the last joint meeting.

@dizhang168
Copy link
Contributor Author

Sorry, I realized I linked the wrong comment previously. My understanding is that the CSSWG discussed the topic last week and resolved that the CSS Display level 4, which includes the spec for reading-flow can move to FPWD: w3c/csswg-drafts#6900 (comment)
As such, it has been reviewed and should land for December 19, this week:
w3c/transitions#685 (comment)

Further, as fantasai mentioned above, "The stability of the feature on the CSS side shouldn't matter, ideally, because the HTML spec is written in a way that doesn't care how CSS defines the reading order." This is true since all we do is reference the concepts of "reading flow container" and "rendering-defined sibling reading flow" from the CSS spec. Tabindex and focus navigation scopes are all HTML concepts. I believe we can remove the label "do not merge yet" and this PR is ready as is.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
images/reading-flow-order-example.svg Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@rachelandrew
Copy link

The spec is now published on TR https://www.w3.org/TR/css-display-4/ my understanding is that the outstanding CSS things to discuss shouldn't impact the HTML side, as Di mentioned in her comment.

@domfarolino
Copy link
Member

RE #10613 (comment): this is great to hear! I'll remove the do not merge label from our end, given that update.

@domfarolino domfarolino removed the do not merge yet Pull request must not be merged per rationale in comment label Dec 19, 2024
source Show resolved Hide resolved
@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Dec 19, 2024
@annevk
Copy link
Member

annevk commented Dec 19, 2024

My understanding is different and I'd appreciate it if you could leave the label intact.

@rachelandrew
Copy link

@annevk can you explain what you are concerned about (and the relationship to the HTML spec) so we can get those issues prioritized in the CSSWG? I'd be happy to focus on the issues that allow us to get this landed.

@annevk
Copy link
Member

annevk commented Dec 19, 2024

@fantasai raised a number of issues on the CSS side and I believe she intends to raise more. And we need the CSS side to be stable (which FPWD does not indicate at all; the CSS WG is quite notorious for renaming things late in the game) to be able to fully test this feature end-to-end, which is a pre-requisite for our criteria: https://whatwg.org/working-mode#changes. (@fantasai did indicate above that she hoped that it didn't matter whether the CSS side was stable or not, but it does matter to our process.)

@rachelandrew
Copy link

@fantasai if you have more issues in mind, could you raise them? I can get them on the agenda and work through them once we know what they are.

source Show resolved Hide resolved
source Show resolved Hide resolved
@dizhang168
Copy link
Contributor Author

We should move the conversation about CSS+HTML integration to w3c/csswg-drafts#11328 and keep this PR about HTML spec changes only.

source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I've done another pass and I think this LGTM to me. There are two things I find a little funny but not blocking.

  1. It's a little strange that nothing actually calls the reading flow order "algorithm" like a normal algorithm, despite it being written algorithmically. This is an artifact of how focus navigation scopes are constructed—they are not done so algorithmically, but rather descriptively. Therefore the reference to how they're ordered by "tabindex value" is also descriptive, instead of calling the algorithm and working on its return value etc. This quirk predates this PR, and we pile on with how we reference reading flow order—the definition is written very algorithmically and has a return value and everything, but we reference it descriptively. I think this is fine since it sets us up for a more ideal future where all of these things become more algorithmic in nature, but it's just worth pointing out.
  2. The second thing is we're introducing another ordering type for focus navigation scopes but we're keeping the old name—that mentions "tabindex-ordered" directly—for the general concept of all ordered scopes: "tabindex-ordered focus navigation scope" and "flattened tabindex-ordered focus navigation scope". We sidestep this weirdness by specializing some "tabindex-ordered focus navigation scopes" to actually be "reading-flow-ordered focus navigation scopes", which isn't totally weird since reading-flow-ordered scopes are still partially influenced by tabindex. Another way to do this would be to rename all of the tabindex-ordered scopes to a generic "ordered focus navigation scope" and require that some kind of order type (tabindex value or reading flow) be supplied in order to use them. I'm not sure it's necessary, but I'd also be fine with this as a follow-up or general refactoring to this area. I'm fine with things as they are though.

@@ -79807,6 +79812,12 @@ dictionary <dfn dictionary>ToggleEventInit</dfn> : <span>EventInit</span> {
<li><p>If <var>element</var> is in the <span data-x="popover-showing-state">popover showing
state</span> and has a <span>popover invoker</span> set, then return <var>element</var>.</p></li>

<li><p>If <var>element</var>'s parent is a <span>reading-flow-ordered scope owner</span>, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we should probably make all of the items in this list properly link to "parent", but keeping consistency in this PR, as you do, is I think fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: focus
Development

Successfully merging this pull request may close these issues.

7 participants