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

Update autofocus processing algorithm #4763

Merged
merged 8 commits into from Sep 3, 2019

Conversation

@tkent-google
Copy link
Collaborator

@tkent-google tkent-google commented Jul 11, 2019

  • Run focusing steps after an animation frame
  • Don't autofocus if the top-level document has focused area
  • Don't autofocus if one of ancestor document has non-empty URI fragments.

This fixes #3551 and #4645

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


/interaction.html ( diff )
/webappapis.html ( diff )

Copy link
Member

@domenic domenic left a comment

Looking great; this seems to meet all the requirements from linked threads, and generally be a more sensible and user-friendly model.

Most of my suggestions are nits, but some are about clarifications where I'm not 100% sure I understood the intent.

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
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
</p></li>
</ol>

<li><p>If <var>element</var> is a <span>focusable area</span>, then set
Copy link
Member

@domenic domenic Jul 11, 2019

Choose a reason for hiding this comment

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

Better to break the three things down into 1. 2. 3.

It may also be good to add a note about when something might reach this step while not being a focusable area. (E.g. it became hidden.)

Copy link
Collaborator Author

@tkent-google tkent-google Jul 12, 2019

Choose a reason for hiding this comment

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

Done: Added a sub-<ol>.

Added a note <p>. Actually, we add any element with autofocus attribute to candidates list without checking focusable area or not.

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

@domenic domenic left a comment

LGTM. Let's get some tests written so we can see how this compares to Gecko/WebKit behavior, i.e. how much of a change this would be for them. I still remain hopeful that even without that, they'll chime in to let us know if the algorithm makes sense.

source Outdated

<li>
<p>If <var>topDocument</var>'s <span data-x="focused area of the document">focused area</span>
is not <var>topDocument</var> itself, or <var>topDocument</var>'s <span>target element</span> is
Copy link
Collaborator Author

@tkent-google tkent-google Jul 18, 2019

Choose a reason for hiding this comment

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

I found checking 'target element' of a document was not helpful because it is set asynchronously and an autofocus element can get focus before setting 'target element'. I'd like to change "document's target element is not null" to " document's URL's fragment is not empty".

Copy link
Collaborator

@smaug---- smaug---- Aug 30, 2019

Choose a reason for hiding this comment

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

url's fragment may change too during page load. But I guess that isn't really an issue.

Copy link
Collaborator Author

@tkent-google tkent-google Sep 2, 2019

Choose a reason for hiding this comment

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

It's possible that URL fragment is empty, a user indicated a specific scroll position, and the user doesn't want to change focused element by autofocus attribute.
However I have never heard complain about it. I think we can address it later if web developers complain.

@tkent-google
Copy link
Collaborator Author

@tkent-google tkent-google commented Jul 19, 2019

source Outdated
@@ -91156,6 +91241,11 @@ dictionary <dfn>PromiseRejectionEventInit</dfn> : <span>EventInit</span> {
<li><p>For each <span>fully active</span> <code>Document</code> in <var>docs</var>, update the
rendering or user interface of that <code>Document</code> and its <span
data-x="concept-document-bc">browsing context</span> to reflect the current state.</p></li>

<li><p>For each <span>fully active</span> <code>Document</code> in <var>docs</var>, <span>flush
Copy link
Member

@domenic domenic Jul 22, 2019

Choose a reason for hiding this comment

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

What is the reasoning behind this position? So far it seems most things go before rAF callbacks, except intersection observer. So I would normally put this before...

Copy link
Collaborator Author

@tkent-google tkent-google Jul 25, 2019

Choose a reason for hiding this comment

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

Because 'focusable area' check needs style computation, and 'focusing steps' needs layout result. I'd like to put this step after finishing rendering in order to avoid an additional style computation and layout caused by 'flush autofocus candidates' steps.

Copy link
Member

@domenic domenic Jul 29, 2019

Choose a reason for hiding this comment

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

Thanks, that makes sense. Can you add a source code comment (<!-- ... -->), or maybe a <p class="note">, explaining this? I'm not sure whether a <p class="note"> makes sense because the spec doesn't talk about style computation/etc., but at least a source code comment would help future spec editors.

Copy link
Collaborator Author

@tkent-google tkent-google Aug 1, 2019

Choose a reason for hiding this comment

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

Ok, I added a comment.

Copy link
Contributor

@bzbarsky bzbarsky Aug 15, 2019

Choose a reason for hiding this comment

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

I'm still a bit concerned about this. We do want to have done style computation, but do we want to have done painting? Focusing things will generally scroll to them and whatnot. Will that just get picked up on the next tick of the rendering loop?

Copy link
Collaborator Author

@tkent-google tkent-google Aug 16, 2019

Choose a reason for hiding this comment

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

Yeah, "after paint" timing is better for kicking the flush steps.
I couldn't find places better than here in the HTML specification.

Copy link
Contributor

@bzbarsky bzbarsky left a comment

This generally seems reasonable to me. @smaug---, thoughts?

source Outdated Show resolved Hide resolved
source Outdated
@@ -91156,6 +91241,11 @@ dictionary <dfn>PromiseRejectionEventInit</dfn> : <span>EventInit</span> {
<li><p>For each <span>fully active</span> <code>Document</code> in <var>docs</var>, update the
rendering or user interface of that <code>Document</code> and its <span
data-x="concept-document-bc">browsing context</span> to reflect the current state.</p></li>

<li><p>For each <span>fully active</span> <code>Document</code> in <var>docs</var>, <span>flush
Copy link
Contributor

@bzbarsky bzbarsky Aug 15, 2019

Choose a reason for hiding this comment

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

I'm still a bit concerned about this. We do want to have done style computation, but do we want to have done painting? Focusing things will generally scroll to them and whatnot. Will that just get picked up on the next tick of the rendering loop?

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

@domenic domenic commented Aug 21, 2019

I feel like we're getting close to merging this. According to the test results, the new behavior doesn't match any browser exactly, but instead is an attempt to take advantage of the fact that browsers are not interoperable, in order to produce a good, reasonable specification that matches some union of their behaviors and meets user expectations.

@bzbarsky, would you like us to hold off for @smaug----'s review?

Also another attempt at pinging @rniwa, as he's been helpful and active reviewing other focus stuff recently.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Aug 21, 2019

I'm going to look at this soon. I'm finishing up my work to update WebKit's tabIndex behavior still.

source Outdated

<li><p>If <span data-x="concept-document-url">URL</span>'s
<span data-x="concept-url-fragment">fragment</span> of any <code>Document</code> in
<var>inclusiveAncestorDocuments</var> is not empty, then <span>continue</span>.</p></li>
Copy link
Collaborator

@smaug---- smaug---- Aug 30, 2019

Choose a reason for hiding this comment

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

Probably just because of it is hard to see the context here, worth to check 'continue' makes sense?

Copy link
Collaborator Author

@tkent-google tkent-google Sep 2, 2019

Choose a reason for hiding this comment

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

candidates contains elements from various frames. So inclusiveAncestorDocuemnts for the next element in candidates may be different.

tkent-google added a commit to web-platform-tests/wpt that referenced this issue Sep 3, 2019
tkent-google and others added 6 commits Sep 3, 2019
- Run focusing steps after an animation frame
- Don't autofocus if the top-level document has focused area
- Don't autofocus if one of ancestor document has :target element.

This fixes whatwg#3551 and whatwg#4645
@tkent-google tkent-google force-pushed the tkent-autofocus-refresh branch from 81b2976 to 59ef44d Sep 3, 2019
@tkent-google
Copy link
Collaborator Author

@tkent-google tkent-google commented Sep 3, 2019

Rebased on ToT.

@domenic domenic merged commit 2d783db into whatwg:master Sep 3, 2019
2 checks passed
foolip added a commit to web-platform-tests/wpt that referenced this issue Sep 3, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 9, 2019
…rithm update, a=testonly

Automatic update from web-platform-tests
html: Update tests for an autofocus algorithm update (#17929)

whatwg/html#4763
--

wpt-commits: 08e6411a9cb36e4e7987d24ddb31e1d62690e1cb
wpt-pr: 17929
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Sep 9, 2019
…rithm update, a=testonly

Automatic update from web-platform-tests
html: Update tests for an autofocus algorithm update (#17929)

whatwg/html#4763
--

wpt-commits: 08e6411a9cb36e4e7987d24ddb31e1d62690e1cb
wpt-pr: 17929
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…rithm update, a=testonly

Automatic update from web-platform-tests
html: Update tests for an autofocus algorithm update (#17929)

whatwg/html#4763
--

wpt-commits: 08e6411a9cb36e4e7987d24ddb31e1d62690e1cb
wpt-pr: 17929

UltraBlame original commit: 99752f25f5f5ed857cc819607882fdb49e2d0509
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…rithm update, a=testonly

Automatic update from web-platform-tests
html: Update tests for an autofocus algorithm update (#17929)

whatwg/html#4763
--

wpt-commits: 08e6411a9cb36e4e7987d24ddb31e1d62690e1cb
wpt-pr: 17929

UltraBlame original commit: 99752f25f5f5ed857cc819607882fdb49e2d0509
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…rithm update, a=testonly

Automatic update from web-platform-tests
html: Update tests for an autofocus algorithm update (#17929)

whatwg/html#4763
--

wpt-commits: 08e6411a9cb36e4e7987d24ddb31e1d62690e1cb
wpt-pr: 17929

UltraBlame original commit: 99752f25f5f5ed857cc819607882fdb49e2d0509
autofocus candidates</span> for that <code>Document</code> if its
<span data-x="concept-document-bc">browsing context</span> is a
<span>top-level browsing context</span>.</p></li>
<!-- The above step is placed just after 'update the rendering' step in order to avoid
Copy link
Collaborator

@rniwa rniwa Oct 9, 2019

Choose a reason for hiding this comment

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

This doesn't seem right. We've already marked the painting time. Why are we focusing an element and potentially arbitrary author scripts for arbitrary amount of time?

Also, "update the rendering or user interface of that Document and its browsing context to reflect the current state." updates the painting of the document. If we had focused an element, there is a good chance, the focus ring may should be drawn around the newly focused element. Yet autofocusing step being after this rendering update would mean that it would necessarily be one frame / rAF later.

Copy link
Collaborator

@rniwa rniwa Oct 9, 2019

Choose a reason for hiding this comment

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

Tracking this in #4992.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

5 participants