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

[render-blocking] Fix implicitly-render-blocking definition. #10214

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[render-blocking] Fix implicitly-render-blocking definition. #10214

wants to merge 1 commit into from

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Mar 20, 2024 •
edited by pr-preview bot
Loading

As per discussion in #10145.

  • At least two implementers are interested (and none opposed):
    • Gecko
    • Blink (as per discussion in WHATNOT meeting)
    • WebKit (as per discussion in WHATNOT meeting)
  • Tests are written and can be reviewed and commented upon at:
    • …
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

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

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Mar 20, 2024, 12:30 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

      <!DOCTYPE html>
      <html>
      <head>
          <meta name="viewport" content="width=device-width, initial-scale=1">
          <meta name="robots" content="noindex">
          <style>body,html{height:100%;margin:0}body{display:flex;align-items:center;justify-content:center;flex-direction:column;-webkit-font-smoothing:antialiased;text-rendering:optimizeLegibility}p{text-align:center;font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Roboto,Oxygen,Ubuntu,Cantarell,Fira Sans,Droid Sans,Helvetica Neue,sans-serif;color:#000;font-size:14px;margin-top:-50px}p.code{font-size:24px;font-weight:500;border-bottom:1px solid #e0e1e2;padding:0 20px 15px}p.text{margin:0}a,a:visited{color:#aaa}</style>
      </head>
      <body>
      <p class="code">
        Error
      </p>
      <p class="text">
        We encountered an error when trying to load your application and your page could not be served. Check the logs for your application in the App Platform dashboard.
      </p>
        <div style="display:none;">
          <h1>
    upstream_reset_before_response_started{connection_termination} (503 UC)      </h1>
          <p data-translate="connection_timed_out">App Platform failed to forward this request to the application.</p>
      </div>
      </body>
      </html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@emilio
Copy link
Contributor Author

emilio commented Mar 20, 2024

@annevk I'd appreciate a sanity-check on the approach. I can go file bugs and write tests if this looks good to you. Thanks!

annevk
annevk reviewed Mar 20, 2024
View reviewed changes
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.

Thanks for writing this up. I think this generally works, but there's some feedback to address.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source
@@ -62237,11 +62253,16 @@ o............A....e

<hr>

<p>A <code>script</code> element on creation sets its
<span>[[ImplicitlyPotentiallyRenderBlocking]]</span> slot to true if the element was created by
its <span>node document</span>'s parser.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Same here. I wonder if we can move all these parser-related requirements to the parser directly. That'd be slightly more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know roughly where that'd go, happy to give that a shot.

Copy link
Member

Choose a reason for hiding this comment

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

I think in https://html.spec.whatwg.org/#create-an-element-for-the-token would probably make sense.

annevk
annevk reviewed Mar 20, 2024
View reviewed changes
source
@@ -1838,6 +1838,9 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li><p>If <var>removedNode</var>'s <code data-x="attr-popover">popover</code> attribute is not in
the <span data-x="attr-popover-none-state">no popover state</span>, then run the <span>hide
popover algorithm</span> given <var>removedNode</var>, false, false, and false.</p></li>

<li><p>Set <var>removedNode</var>'s <span>implicitly potentially render-blocking flag</span> to
Copy link
Member

Choose a reason for hiding this comment

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

No flag suffix please for new state.

source
Comment on lines +7776 to +7777
<dfn>implicitly potentially render-blocking</dfn>. Elements are not <span>implicitly potentially
render-blocking</span> by default.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to reference the boolean state or is this another piece of state or abstraction?

source
@@ -62237,11 +62253,16 @@ o............A....e

<hr>

<p>A <code>script</code> element on creation sets its
<span>[[ImplicitlyPotentiallyRenderBlocking]]</span> slot to true if the element was created by
its <span>node document</span>'s parser.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I think in https://html.spec.whatwg.org/#create-an-element-for-the-token would probably make sense.

source
Comment on lines +17607 to +17608
<p>A <code>style</code> element is <span>implicitly potentially render-blocking</span> if its
<span>implicitly potentially render-blocking flag</span> is true.</p>
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 have to say this here. We can just rely on the "potentially render-blocking" definition for element, no?

Same for the instances below.

source
Comment on lines 62260 to +62265
<p>A <code>script</code> element <var>el</var> is <span>implicitly potentially
render-blocking</span> if <var>el</var>'s <span data-x="concept-script-type">type</span> is
"<code data-x="">classic</code>", <var>el</var> is <span>parser-inserted</span>, and
<var>el</var> does not have an <code data-x="attr-script-async">async</code> or <code
data-x="attr-script-defer">defer</code> attribute.</p>
render-blocking</span> if <var>el</var>'s <span>implicitly potentially render-blocking flag</span>
slot is true, <var>el</var>'s <span data-x="concept-script-type">type</span> is
"<code data-x="">classic</code>", and <var>el</var> does not have an
<code data-x="attr-script-async">async</code> or <code data-x="attr-script-defer">defer</code>
attribute.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why not allow set it when the conditions are correct? Or does this mean that if once of these things changes, its render-blocking status also changes?

@past past mentioned this pull request Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emilio @annevk