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

Element.checkVisibility review #734

Closed
1 task done
vmpstr opened this issue Apr 27, 2022 · 12 comments
Closed
1 task done

Element.checkVisibility review #734

vmpstr opened this issue Apr 27, 2022 · 12 comments
Assignees
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Review type: Already shipped Already shipped in at least one browser Topic: CSS Venue: CSS WG

Comments

@vmpstr
Copy link

vmpstr commented Apr 27, 2022

Hello,

I'm requesting a TAG review of Element.isVisible.

Element.isVisible() returns true if the element is visible, and false if it is not. It checks a variety of factors that would make an element invisible, including visibility, content-visibility, and opacity.

Further details:

  • I have reviewed the TAG's Web Platform Design Principles
  • Relevant time constraints or deadlines: None, but this feature assists with successful adoption of content-visibility: hidden
  • The group where the work on this specification is currently being done: CSSWG
  • This work is being funded by: Google

💬 leave review feedback as a comment in this issue and @-notify @vmpstr

@vmpstr
Copy link
Author

vmpstr commented Jun 9, 2022

For additional info, we're still discussing a good name for this function. The bulk of the discussion can be found in this issue w3c/csswg-drafts#7317

@plinss
Copy link
Member

plinss commented Jun 21, 2022

Has there been any discussion of making this async? In general we'd prefer not to add more functionality that synchronously blocks for style computation (or layout).

@atanassov
Copy link

Also, despite the use cases being somewhat obvious, we encourage you to write a complete explainer and capture all the other important bits in a single place - other considerations, code examples, privacy, security and a11y considerations etc. Can you please write one?

@josepharhar
Copy link

There is already an explainer and privacy/security self review linked in the issue description. Is there something missing in those?

@vmpstr
Copy link
Author

vmpstr commented Jun 21, 2022

Has there been any discussion of making this async? In general we'd prefer not to add more functionality that synchronously blocks for style computation (or layout).

We haven't considered making this asynchronous. IMHO it would complicate the API and code using it, to avoid a potential style (+ possibly layout) update. If my reading of https://www.w3.org/TR/design-principles/#synchronous is correct, the general preference is for synchronous APIs unless there is substantial I/O or heavy work. Again IMHO, I don't think style and layout qualify here. There is already a number of APIs like getBoundingClientRect that have to process style + layout. In other words, a number of existing APIs already work as if style and layout is always up-to-date from script perspective. I can't see a justification why this API should be different.

@plinss
Copy link
Member

plinss commented Jun 21, 2022

The reason I ask is that the general consensus is having synchronous mechanisms that force style computation and layout were a mistake. If we were designing the DOM APIs today those would all be promise based.

Now, one can argue that horse has left the stable, and adding one more synchronous method is relatively harmless, and even better for developers as they wouldn't be faced with a mix of sync/async methods.

On the other hand, many of us would like to see new async methods to access that data, and all the existing synchronous methods and properties become deprecated. Going down that path, adding more and more synchronous methods makes the eventual transition to async more complex.

I don't believe the TAG is going to reject this for being synchronous, but we'd like to see that the option has at least been taken into consideration by the relevant WGs. We'd also like to see some traction on developing async replacements for the various other APIs that force style computation and layout. (Were those to be under active development, it would make more sense for new mechanisms to follow that path and lead authors into the async world.)

@vmpstr
Copy link
Author

vmpstr commented Jul 20, 2022

Sorry for the late reply.

Without considering existing features, I still believe that this function should be synchronous. The intended use-case for this feature is for the developer to check the existing element with current styles for visibility to see if a further measurement is appropriate. Something like the following:

if (e.checkVisibility())
  return e.getBoundingClientRect();
else
  return null;

The reason this pattern is useful is the way content-visibility: hidden, for example, works: if one just invokes e.getBoundingClientRect() and e is in a subtree of a content-visibility: hidden element then that will cause an update in an otherwise skipped subtree. The checkVisibility function, on the other hand, would only update the rendering while still respecting skipped elements, then can check that e is indeed under a content-visibility: hidden and return false

Having this function be asynchronous puts a burden on the developer to ensure that the function is either called in an async function with an await, or to restructure the 'natural' flow to account for the fact that the visibility check will happen at a future time. It isn't clear to me whether the guidance to prefer asynchronous APIs to avoid forced rendering updates should take precedence. It is my humble opinion that in this case it should not, and the function call should remain synchronous.

@torgo torgo modified the milestones: 2022-08-29-week, 2022-09-12-TPAC Sep 9, 2022
@torgo torgo added the Review type: Already shipped Already shipped in at least one browser label Sep 9, 2022
@torgo torgo removed this from the 2022-09-12-TPAC milestone Sep 18, 2022
@torgo torgo added this to the 2022-12-12-week milestone Nov 28, 2022
@torgo torgo changed the title Element.isVisible review Element.checkVisibility review Dec 2, 2022
@torgo torgo removed the Review type: Already shipped Already shipped in at least one browser label May 22, 2023
@torgo
Copy link
Member

torgo commented May 22, 2023

@vmpstr we're not clear on the current status of this and the info in Chromestatus is at odds with the info in the request. Also the explainer is in an archived repo. Is there current work still going on and are you still seeking TAG review? If so, can you give us an update and also update the issue as well please?

@josepharhar
Copy link

I updated the chromestatus, we shipped this in chrome 105.

@torgo
Copy link
Member

torgo commented May 24, 2023

Thanks for that @josepharhar. We discussed again in today's TAG plenary call. Regarding multi-stakeholder support, we also note from caniuse data that there are multiple implementations, which is also not reflected in Chromestatus. Can you please update, or maybe start using the BCD data which powers CanIUse to also power this indication? We still have some reservations regarding sync vs. async as plinss highlighted however we are going to move that discussion to our Design Principles work.

@torgo torgo closed this as completed May 24, 2023
@torgo torgo added the Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes label May 24, 2023
@torgo torgo removed this from the 2023-05-22-week milestone May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Review type: Already shipped Already shipped in at least one browser Topic: CSS Venue: CSS WG
Projects
None yet
Development

No branches or pull requests

7 participants