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

Fix InsertRule to use the right CssRuleTypes #32125

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

Loirooriol
Copy link
Contributor

CSSRule::Type() returns an u16 for CSSOM. InsertRule() was incorrectly using this to create a CssRuleTypes.

Instead of CssRuleTypes::from_bits(rule_type), it should be something like CssRuleTypes::from_bits(1 << rule_type).

However, that would only work when Type() provides an actual value, which per https://drafts.csswg.org/cssom/#dom-cssrule-type only happens for old rule types. New rule types just return 0.

Therefore, this patch changes the signature of SpecificCSSRule::ty() to return the actual CssRuleType, and then CSSRule::Type() can zero it out when necessary.

The fix is only relevant for CSS Nesting, which is currently disabled on Servo, so no test is necessary.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because the fix is only relevant for CSS Nesting, which is currently disabled

CSSRule::Type() returns an u16 for CSSOM. InsertRule() was incorrectly
using this to create a CssRuleTypes.

Instead of CssRuleTypes::from_bits(rule_type), it should be something
like CssRuleTypes::from_bits(1 << rule_type).

However, that would only work when Type() provides an actual value,
which per https://drafts.csswg.org/cssom/#dom-cssrule-type only happens
for old rule types. New rule types just return 0.

Therefore, this patch changes the signature of SpecificCSSRule::ty()
to return the actual CssRuleType, and then CSSRule::Type() can zero it
out when necessary.

The fix is only relevant for CSS Nesting, which is currently disabled
on Servo, so no test is necessary.
@Loirooriol Loirooriol added the T-full Do a full try run label Apr 21, 2024
@github-actions github-actions bot removed the T-full Do a full try run label Apr 21, 2024
Copy link

🔨 Triggering try run (#8774416732) for Linux WPT, MacOS, Windows, Android

Copy link

Test results for linux-wpt-layout-2013 from try job (#8774416732):

Flaky unexpected result (17)
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
  • FAIL [expected PASS] /css/css-sizing/dynamic-available-size-iframe.html (#31559)
  • OK /css/cssom-view/MediaQueryList-extends-EventTarget.html (#25269)
    • FAIL [expected PASS] subtest: onchange adds listener

      promise_test: Unhandled rejection with value: object "TypeError: _event is undefined"
      

    • FAIL [expected PASS] subtest: onchange removes listener

      assert_equals: expected 1 but got 0
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-window-open.html (#28691)
    • PASS [expected FAIL] subtest: load event does not fire on window.open('about:blank')
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-cross-origin.sub.window.html (#29056)
    • FAIL [expected PASS] subtest: Cross-origin navigation started from unload handler must be ignored

      promise_test: Unhandled rejection with value: object "SecurityError: The operation is insecure."
      

  • OK /html/browsers/history/the-history-interface/traverse_the_history_2.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • CRASH [expected OK] /html/canvas/offscreen/canvas-host/2d.canvas.host.size.attributes.parse.minus.html
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
    • FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html (#24066)
    • FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • OK /html/semantics/forms/historical.html (#28568)
    • PASS [expected FAIL] subtest: &lt;input name=isindex&gt; should not be supported
  • OK [expected ERROR] /html/semantics/scripting-1/the-script-element/defer-script/async-script.html?reload (#29054)
  • TIMEOUT /resource-timing/test_resource_timing.html (#25720)
    • FAIL [expected PASS] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (img)

      assert_equals: expected 25039104 but got 25039360
      

  • TIMEOUT [expected OK] /webmessaging/with-ports/017.html (#24486)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, about:blank

      Test timed out
      

  • TIMEOUT [expected OK] /webmessaging/without-ports/017.html (#24486)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, about:blank

      Test timed out
      

  • OK [expected ERROR] /workers/constructors/Worker/Worker-constructor.html (#22991)
Stable unexpected results that are known to be intermittent (8)
  • OK /html/browsers/browsing-the-web/navigating-across-documents/empty-iframe-load-event.html (#29066)
    • FAIL [expected PASS] subtest: Check execution order from nested timeout

      assert_equals: Expected nested setTimeout to run second expected true but got false
      

    • FAIL [expected PASS] subtest: Check execution order on load handler

      assert_equals: Expected onload to run first expected false but got true
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin-fragment.html (#20768)
    • PASS [expected FAIL] subtest: Tests that a fragment navigation in the unload handler will not block the initial navigation
  • OK [expected TIMEOUT] /html/semantics/embedded-content/media-elements/track/track-element/no-cuechange-before-play.html (#31014)
    • FAIL [expected TIMEOUT] subtest: Ensure that the 'cuechange' event is not fired before video playback has begun.

      assert_true: Not expecting event, but got canplaythrough event expected true got false
      

  • OK /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html (#24057)
    • FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
    • FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
      

  • TIMEOUT /resource-timing/test_resource_timing.https.html (#25216)
    • FAIL [expected PASS] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (img)

      assert_equals: expected 6541056 but got 6540544
      

  • TIMEOUT [expected OK] /webmessaging/without-ports/018.html (#24485)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, javascript:

      Test timed out
      

  • TIMEOUT [expected OK] /webstorage/localstorage-about-blank-3P-iframe-opens-3P-window.partitioned.tentative.html (#29053)
    • TIMEOUT [expected PASS] subtest: StorageKey: test 3P about:blank window opened from a 3P iframe

      Test timed out
      

Copy link

Test results for linux-wpt-layout-2020 from try job (#8774416732):

Flaky unexpected result (21)
  • TIMEOUT [expected OK] /FileAPI/url/url-charset.window.html (#26997)
    • TIMEOUT [expected PASS] subtest: Blob charset should override any auto-detected charset.

      Test timed out
      

    • TIMEOUT [expected PASS] subtest: Blob charset should override &lt;meta charset&gt;.

      Test timed out
      

  • FAIL [expected PASS] /_mozilla/css/iframe/hide_and_show.html (#15265)
  • FAIL [expected PASS] /_mozilla/mozilla/iframe/resize_after_load.html (#13573)
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
  • OK /css/css-fonts/variations/at-font-face-font-matching.html (#20684)
    • PASS [expected FAIL] subtest: Matching font-style: 'normal' should prefer 'oblique -50deg -20deg' over 'oblique -40deg -30deg'
    • PASS [expected FAIL] subtest: Matching font-style: 'oblique -10deg' should prefer 'oblique -5deg' over 'oblique -1deg 0deg'
  • OK /css/cssom-view/MediaQueryList-addListener-handleEvent.html (#24571)
    • FAIL [expected PASS] subtest: calls handleEvent method of event listener

      assert_equals: expected (object) object "[object Object]" but got (undefined) undefined
      

  • TIMEOUT /css/cssom-view/MediaQueryList-extends-EventTarget.html (#25269)
    • FAIL [expected PASS] subtest: listeners for "change" type are called

      assert_equals: expected 1 but got 0
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin.window.html (#29049)
    • PASS [expected FAIL] subtest: Same-origin navigation started from unload handler must be ignored
  • OK /html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html (#23849)
    • PASS [expected FAIL] subtest: The document for a standalone media file should have one child in the body.
  • OK /html/browsers/history/the-history-interface/traverse_the_history_4.html (#21383)
    • FAIL [expected PASS] subtest: Multiple history traversals, last would be aborted

      assert_array_equals: Pages opened during history navigation expected property 1 to be 5 but got 3 (expected array [6, 5] got [6, 3])
      

  • OK /html/interaction/focus/the-autofocus-attribute/document-with-fragment-valid.html (#28259)
    • PASS [expected FAIL] subtest: Autofocus elements in top-level browsing context's documents with URL fragments should be skipped.
  • TIMEOUT [expected OK] /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
    • TIMEOUT [expected FAIL] subtest: Contenteditable element should support autofocus

      Test timed out
      

    • NOTRUN [expected FAIL] subtest: Element with tabindex should support autofocus
    • NOTRUN [expected PASS] subtest: Non-HTMLElement should not support autofocus
    • NOTRUN [expected FAIL] subtest: Host element with delegatesFocus should support autofocus
    • NOTRUN [expected FAIL] subtest: Host element with delegatesFocus including no focusable descendants should be skipped
    • NOTRUN [expected FAIL] subtest: Area element should support autofocus
  • TIMEOUT [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
    • NOTRUN [expected FAIL] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
  • OK /html/semantics/forms/form-submission-0/urlencoded2.window.html (#28687)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: 0x00 in name (formdata event)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: \r\n in name (formdata event)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: lone surrogate in name and value (formdata event)
  • ERROR [expected OK] /html/semantics/scripting-1/the-script-element/defer-script/async-script.html?reload (#29054)
  • OK /html/syntax/parsing/DOMContentLoaded-defer.html (#21550)
    • PASS [expected FAIL] subtest: The end: DOMContentLoaded and defer scripts
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-delayed.html (#27659)
    • FAIL [expected PASS] subtest: async document.write in a module

      assert_true: onload must be called expected true got false
      

  • TIMEOUT /resource-timing/test_resource_timing.html (#25720)
    • FAIL [expected PASS] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (img)

      assert_equals: expected 5925632 but got 5925888
      

  • OK [expected TIMEOUT] /webmessaging/with-ports/018.html (#24485)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, javascript:
  • OK [expected TIMEOUT] /webmessaging/without-ports/018.html (#24485)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, javascript:
Stable unexpected results that are known to be intermittent (8)
  • FAIL [expected PASS] /_mozilla/css/dirty_viewport.html (#13731)
  • OK /css/cssom-view/MediaQueryList-addListener-removeListener.html (#24569)
    • FAIL [expected PASS] subtest: listeners are called correct number of times

      assert_equals: expected 2 but got 1
      

  • TIMEOUT /fetch/metadata/generated/element-img-environment-change.sub.html (#30111)
    • FAIL [expected TIMEOUT] subtest: sec-fetch-site - Not sent to non-trustworthy cross-site destination, no attributes

      promise_test: Unhandled rejection with value: object "Error: Failed to query for recorded headers."
      

    • TIMEOUT [expected NOTRUN] subtest: sec-fetch-mode - Not sent to non-trustworthy same-origin destination, no attributes

      Test timed out
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-cross-origin.sub.window.html (#29056)
    • FAIL [expected PASS] subtest: Cross-origin navigation started from unload handler must be ignored

      promise_test: Unhandled rejection with value: object "SecurityError: The operation is insecure."
      

  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
  • OK [expected TIMEOUT] /html/semantics/forms/form-submission-0/reparent-form-during-planned-navigation-task.html (#29724)
    • PASS [expected TIMEOUT] subtest: reparent-form-during-planned-navigation-task
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-static-import-delayed.html (#26243)
    • PASS [expected FAIL] subtest: document.write in an imported module
  • TIMEOUT [expected OK] /webmessaging/with-ports/017.html (#24486)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, about:blank

      Test timed out
      

Copy link

✨ Try run (#8774416732) succeeded.

@Loirooriol Loirooriol marked this pull request as ready for review April 21, 2024 19:45
@mrobinson mrobinson added this pull request to the merge queue Apr 22, 2024
Merged via the queue into servo:main with commit f9e154a Apr 22, 2024
73 checks passed
@Loirooriol Loirooriol deleted the css-rule-type branch June 11, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants