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.

Sign up for GitHub

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

Jump to bottom

Define timing of focus fixup rule precisely when triggered by style changes. #8225

Closed
emilio opened this issue Aug 27, 2022 · 8 comments · Fixed by #8392
Closed

Define timing of focus fixup rule precisely when triggered by style changes. #8225

emilio opened this issue Aug 27, 2022 · 8 comments · Fixed by #8392
Labels
topic: focus

Comments

@emilio
Copy link
Contributor

emilio commented Aug 27, 2022 •
edited
Loading

I was looking into implementing the focus fixup rule in Gecko, and per spec it's not clear when it should happen when it happens as a response to style changes.

Consider this testcase:

<!doctype html>
<input>
<script>
onload = function() {
  let input = document.querySelector("input");
  input.focus();
  console.log("focus", document.activeElement);
  input.style.visibility = "hidden";
  console.log("style tweak", document.activeElement);
  input.getBoundingClientRect();
  console.log("gbcr", document.activeElement);
  requestAnimationFrame(() => requestAnimationFrame(() => {
    console.log("raf", document.activeElement);
  }));
}
</script>

Naively, reading the current spec, I'd expect the element to be blurred after the style update (basically, as a result of getBoundingClientRect()). It seems Blink at least does it at a well defined point in the "update the rendering" steps (which is reasonable too).

I thought WebKit implemented this but at least the Safari version I have access to behaves like Firefox.

I think I'd be fine making this a proper step in the "update the rendering", it seems odd that getBoundingClientRect() would fire a blur event.

@mfreed7 / @josepharhar, it seems blink sometimes applies the focus fixup rule sync (when making the form control disabled for example), but in the case of style changes it does it async. I think I'd rather have a single point in update-the-rendering where this happens but I'm curious if there's any reason to do it sync sometimes. Also I wonder when are you doing it r/n?
@emilio
Copy link
Contributor Author

emilio commented Aug 27, 2022

@nt1m / @rniwa, do you know the status of this in WebKit? I recall seeing some patches in flight to implement this long ago, but it doesn't seem to work, was it reverted or something?

@nt1m
Copy link
Member

nt1m commented Aug 27, 2022

I think I tried to implement this in the update rendering steps: https://bugs.webkit.org/show_bug.cgi?id=237273#c1 , it's the easiest way (since you don't have to find every single place where "is this element focusable?" changes and hook it up there).

It works, although the spec implies we should do synchronously and not async, so I just left it there for now.

@nt1m
Copy link
Member

nt1m commented Aug 27, 2022

(As for the current state, we don't implement the focus fixup rule for most part, aside for finding the right element to focus when showing/hiding a dialog)

@mfreed7
Copy link
Contributor

mfreed7 commented Aug 29, 2022

See this Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1275097. It's currently done in a semi-hacky way, e.g. with timers.

@Loirooriol tried to fix this up at one point and make it synchronous, so perhaps he has more comments.

It feels odd to do this in "update the rendering", but that is likely the most performant place to do it. I'm afraid of the patch above - it'll likely slow things down considerably, since we'll be forcing style updates more often.

@Loirooriol
Copy link
Contributor

Yeah I tried to make it synchronous, but since style changes can affect focusability, then things like document.activeElement, document.querySelector(":focus"), etc. need to flush styles, which seems bad for perf. And several tests failed, so probably not web compatible, but I don't remember the details.

Then I agree that doing it in "update the rendering" seems more reasonable.

@domenic domenic added the agenda+ To be discussed at a triage meeting label Sep 1, 2022
@emilio
Copy link
Contributor Author

emilio commented Sep 1, 2022

So, there seemed to be consensus today on doing this at some point during update the rendering. I sent a patch updating the tests here: https://bugzilla.mozilla.org/show_bug.cgi?id=1788741

However, that uncovers a case where all browsers are interoperable in removing focus synchronously, which is focused element removal.

Not sure to what extent we want that to be special, do we want to keep that behavior? It might be reasonable.

@past past mentioned this issue Sep 1, 2022
Closed
@past past removed the agenda+ To be discussed at a triage meeting label Sep 1, 2022
@mfreed7
Copy link
Contributor

mfreed7 commented Sep 2, 2022

So, there seemed to be consensus today on doing this at some point during update the rendering. I sent a patch updating the tests here: https://bugzilla.mozilla.org/show_bug.cgi?id=1788741

However, that uncovers a case where all browsers are interoperable in removing focus synchronously, which is focused element removal.

Not sure to what extent we want that to be special, do we want to keep that behavior? It might be reasonable.

Interestingly, we just discussed this case w.r.t. the pop-up API.

I'm in favor of keeping that behavior (synchronously fire events for focused element removal), given that it's interoperable, and changing it scares me.

@Loirooriol
Copy link
Contributor

Worth noting that Blink also blurs synchronously when losing the tabindex:

<!DOCTYPE html>
<div id="test" tabindex="1"></div>
<script>
test.focus();
console.log(document.activeElement); // test
test.removeAttribute("tabindex");
console.log(document.activeElement); // document.body
</script>

But no interoperability here so changing may be fine.

@past past mentioned this issue Sep 30, 2022
Closed
@past past mentioned this issue Oct 13, 2022
Closed
domenic added a commit that referenced this issue Oct 16, 2022
@domenic
Make the focus fixup rule more explicit
14eb00c 
* Split it into two variants: one which runs synchronously on HTML element removal, and one which runs during "update the rendering". Closes #8225.

* Be clear that the fixup does not cause any of the normal focus algorithms to run, and thus blur and change events do not fire. Also, delete the confusing "somehow unfocused without another element being explicitly focused" sentence. Fixes #3847. Fixes #6729.
@domenic domenic mentioned this issue Oct 16, 2022
Merged
3 tasks
domenic added a commit that referenced this issue Nov 4, 2022
@domenic
Make the focus fixup rule more explicit
c4533d4 
* Split it into two variants: one which runs synchronously on HTML element removal, and one which runs during "update the rendering". Closes #8225.

* After this split, only the "update the rendering" variant causes the normal focus algorithms to run, and thus blur and change events to fire.

* Delete the confusing "somehow unfocused without another element being explicitly focused" sentence.

Fixes #3847. Fixes #6729.
domenic added a commit that referenced this issue Jan 18, 2023
@domenic
Make the focus fixup rule more explicit
6a5109c 
* Split it into two variants: one which runs synchronously on HTML element removal, and one which runs during "update the rendering". Closes #8225.

* After this split, only the "update the rendering" variant causes the normal focus algorithms to run, and thus blur and change events to fire.

* Delete the confusing "somehow unfocused without another element being explicitly focused" sentence.

Fixes #3847. Fixes #6729.
domenic added a commit that referenced this issue Jan 27, 2023
@domenic
Make the focus fixup rule more explicit
e233336 
* Split it into two variants: one which runs synchronously on HTML element removal, and one which runs during "update the rendering". Closes #8225.

* After this split, only the "update the rendering" variant causes the normal focus algorithms to run, and thus blur and change events to fire.

* Delete the confusing "somehow unfocused without another element being explicitly focused" sentence.

Fixes #3847. Fixes #6729.
@hamishwillee hamishwillee mentioned this issue Mar 21, 2023
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: focus
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make the focus fixup rule more explicit whatwg/html
6 participants
@past @domenic @emilio @nt1m @Loirooriol @mfreed7