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

Remove "control group" concept from HTML #3647

Merged
merged 5 commits into from
May 14, 2018

Conversation

TakayoshiKochi
Copy link
Member

@TakayoshiKochi TakayoshiKochi commented Apr 27, 2018

The control group concept existed to give special focus behavior to

elements, treating them on par with Documents in terms of allowing focus to shift inside of them. However, this was never implemented in user agents.

This change removes the control group concept, instead using Document
directly. This allows some simplifications, e.g. because the first
focusable area of a non-empty Document is always the Document's
viewport.

The change uncovered some areas of the spec that were potentially wrong
or unclear when considering focus around documents. We will follow up on
that in #3675. But for now we keep
the same algorithms, just without s.

Fixes #2171.


/interaction.html ( diff )
/interactive-elements.html ( diff )

As focus "control group" existed for explaining the behavior of
`<dialog>` elements, but it has never implemented in UAs.

This change mostly swaps "control group" with "document" where
appropriate.

And now `<dialog>` element cannot be a focusing target unless it has
explicit tabindex.
source Outdated

<li>

<p>Let <var>control</var> be the first non-<span>inert</span> <span>focusable area</span> in
<var>subject</var>'s <span>control group</span> whose <span>DOM anchor</span> has an <code
<var>subject</var>'s descendants whose <span>DOM anchor</span> has an <code
data-x="attr-fe-autofocus">autofocus</code> attribute specified.</p>
Copy link
Member

@domenic domenic Apr 30, 2018

Choose a reason for hiding this comment

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

I think this should be rephrased to:

Let control be the first descendant element of subject, in tree order, that is not inert and has the autofocus attribute specified.

No need to involve focusable areas anymore.

source Outdated
data-x="attr-fe-autofocus">autofocus</code> attribute specified.</p>

<p>If there isn't one, then let <var>control</var> be the first non-<span>inert</span>
<span>focusable area</span> in <var>subject</var>'s <span>control group</span>.</p>
<span>focusable area</span> in <var>subject</var>'s descendants.</p>
Copy link
Member

@domenic domenic Apr 30, 2018

Choose a reason for hiding this comment

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

Similarly:

... then let control be the first non-inert descendant element of subject, in tree order.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

A few nits, but overall great work!! Very excited for this; thanks so much.

source Outdated
is <dfn>expressly inert</dfn> if it is <span>inert</span> and its nearest ancestor <span>control
group owner object</span> is not <span>inert</span>.</p>
is <dfn>expressly inert</dfn> if it is <span>inert</span> and its <code>Document</code> is not
<span>inert</span>.</p>

<div class="example">
Copy link
Member

Choose a reason for hiding this comment

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

This example is now no longer accurate. I think it can be removed since the concept is simpler to understand now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this example <div> altogether.

source Outdated
<dfn>focused area of the control group</dfn>. Which control is so designated changes over time,
based on algorithms in this specification. If a <span>control group</span> is empty, it has no <span
data-x="focused area of the control group">focused area</span>.</p>
<p>One <span>focusable area</span> <span>in a document tree</span> is designated the <dfn>focused
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to link "in a document tree" here since we're not using it as a predicate. (That is, we're not testing whether something is "in a document tree".)

I would instead just say "in each document tree" and link to "document tree".

Copy link
Member Author

@TakayoshiKochi TakayoshiKochi May 7, 2018

Choose a reason for hiding this comment

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

Looks like there is no definition for the term "document tree", and "document tree" is exclusively used in a phrase "in a document tree" in the spec.

Does

One focusable area in each document is ...

without link from "document" look good?

source Outdated
data-x="focused area of the control group">focused area</span>.</p>
<p>One <span>focusable area</span> <span>in a document tree</span> is designated the <dfn>focused
area of the document</dfn>. Which control is so designated changes over time, based on algorithms
in this specification. If a <code>Document</code> has no <span>focusable area</span>, it has no
Copy link
Member

Choose a reason for hiding this comment

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

has no focusable area -> has no focusable areas

source Outdated
depth-first traversal of the box tree. <ref spec=CSS></p>
anchors</span>. <span data-x="focusable area">Focusable areas</span> with the same <span>DOM
anchor</span> in a <code>Document</code> are ordered relative to their CSS box's relative
positions in a pre-order, depth-first traversal of the box tree. <ref spec=CSS></p>
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing problem but maybe fix while you're here: box's -> boxes'

source Outdated
<var>candidate</var> be the designated <span>focused area of the control
group</span>.</p>
<p>Otherwise, let <var>candidate</var> be the designated <span>focused area of the
document</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This step is wrong if there is no focused area. In that case, we should leave it as the document.

If you want I would suggest restructuring this algorithm a bit to make things easier:

  1. Let candidate be ...
  2. While candidate's focused area is a browsing context container with..., set candidate to the activate document of...
  3. If candidate has a focused area, set candidate to candidate's focused area.
  4. Return candidate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, and almost adopted your comment, except 2. (left the original "redo this step" phrasing).

source Outdated
@@ -73000,7 +72934,7 @@ END:VCARD</pre>
</ol>

<p>User agents must <span>immediately</span> run the <span>focusing steps</span> for a <span>focusable area</span>,
<code>dialog</code>, or <span>browsing context</span> <var>candidate</var> whenever the
or <span>browsing context</span> <var>candidate</var> whenever the
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comma from the previous line now that there's only two items in the list.

source Outdated
(if the <span>control group owner object</span> is a <code>Document</code>, this will always be
a <span>viewport</span>).</p>
focus target</var> be the first <span>focusable area</span> of its <code>Document</code>,
this will always be a <span>viewport</span>).</p>
Copy link
Member

Choose a reason for hiding this comment

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

This should be simplified to just "then let new focus target be its Document's viewport".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also removed the closing parenthesis.

source Outdated

<p class="example">For example, this might happen because an element is removed from its
<code>Document</code>, or has a <code data-x="attr-hidden">hidden</code> attribute added. It might
also happen to an <code>input</code> element when the element gets <span
data-x="concept-fe-disabled">disabled</span>.</p>

<p class="example">In a <code>Document</code> without <code>dialog</code> elements, whose <span
data-x="focused area of the control group">focused area</span> is a <code>button</code> element,
<p class="example">In a <code>Document</code>, whose <span
Copy link
Member

Choose a reason for hiding this comment

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

Can remove the comma now.

Line breaking also seems off here; remember to break on spaces (even if the spaces are inside an attribute).

source Outdated
relative to each other. The order in the <span>sequential focus navigation order</span> does not
have to be related to the order in the <span>control group</span> itself. If a <span>focusable area</span> is
omitted from the <span>sequential focus navigation order</span> of its <span>control group</span>, then
have to be related to the order in the <code>Document</code> itself. If a <span>focusable area</span> is
Copy link
Member

Choose a reason for hiding this comment

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

I would change "order" to "tree order" (and make it a link) since it's now about documents which have a well-defined concept of order.

source Outdated
focus navigation order</span>.</p>

<p>The <dfn>home control group</dfn> is the <span>control group</span> to which <var>starting point</var> belongs.</p>
<p>The <dfn>home document</dfn> is the <code>document</code> to which <var>starting point</var> belongs.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Capital "D"

@TakayoshiKochi
Copy link
Member Author

Thanks for the review! I was OOO last week and am back again.
Except for places I added some reply comments, basically I adopted what you commented.
PTAL.

source Outdated

<p><dfn>Focus fixup rule</dfn>: When the designated <span data-x="focused area of the control
Copy link
Member

Choose a reason for hiding this comment

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

I realized that now we are only worrying about documents, the first focusable area is always the viewport. So I will push a commit that simplifies this.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I pushed a few small fixups; please take a look and tell me if they look right? If so we'll go ahead and merge this.

Proposed commit message:

Remove "control group" concept from the focus model

The control group concept existed to give special focus behavior to
<dialog> elements, treating them on par with Documents in terms of
allowing focus to shift inside of them. However, this was never
implemented in user agents.

This change removes the control group concept, instead using Document
directly. This allows some simplifications, e.g. because the first
focusable area of a non-empty Document is always the Document's
viewport.

The change uncovered some areas of the spec that were potentially wrong
or unclear when considering focus around documents. We will follow up on
that in https://github.com/whatwg/html/issues/3675. But for now we keep
the same algorithms, just without <dialog>s.

Fixes #2171.

@domenic domenic added topic: focus removal/deprecation Removing or deprecating a feature labels May 9, 2018
@TakayoshiKochi
Copy link
Member Author

Thanks!
I've looked at your changes and they look good.

I copied your commit message to the top message (with small tweaks like adding backticks
around dialog element), but I'm not sure it will be used as the git's commit message...

@domenic domenic merged commit 90a60b2 into whatwg:master May 14, 2018
@TakayoshiKochi TakayoshiKochi deleted the remove_control_group branch May 16, 2018 03:29
TakayoshiKochi pushed a commit to TakayoshiKochi/html that referenced this pull request Jul 5, 2018
The PR whatwg#3647 removed "control group" concept, and most of them
were converted to Document. The "control group" could be empty
when e.g. no focusable element in a dialog element, but for a
document without any focusable element, its viewport is still
focusable.

Remove sentences talking about such a non-existent Document
condition.
domenic pushed a commit that referenced this pull request Jul 11, 2018
The PR #3647 removed "control group" concept, and most of them
were converted to Document. The "control group" could be empty
when e.g. no focusable element in a dialog element, but for a
document without any focusable element, its viewport is still
focusable.

Removes sentences talking about such a non-existent Document
condition.

Fixes #3675.
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
The PR whatwg#3647 removed "control group" concept, and most of them
were converted to Document. The "control group" could be empty
when e.g. no focusable element in a dialog element, but for a
document without any focusable element, its viewport is still
focusable.

Removes sentences talking about such a non-existent Document
condition.

Fixes whatwg#3675.
Loirooriol added a commit to Loirooriol/html that referenced this pull request Feb 2, 2022
It seems a remnant of control groups, which were removed in whatwg#3647.
All uses are replaced with the concept of "inert".

Also cleaning up "When a key event is to be routed in a top-level
browsing context", which also seems to have remnants of when control
groups allowed inert elements inside an inert dialog to be focusable.

Fixes whatwg#7564.
Loirooriol added a commit to Loirooriol/html that referenced this pull request Feb 2, 2022
It seems a remnant of control groups, which were removed in whatwg#3647.
All uses are replaced with the concept of "inert".

Also cleaning up "When a key event is to be routed in a top-level
browsing context", which also seems to have remnants of when control
groups allowed inert elements inside an inert dialog to be focusable.

Fixes whatwg#7564.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature topic: focus
Development

Successfully merging this pull request may close these issues.

None yet

2 participants