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

Properly position floats when subsequent boxes collapse margins with containing block (2) #29945

Merged

Conversation

Loirooriol
Copy link
Contributor

PR #29939 tried to address this but missed various cases.


@Loirooriol
Copy link
Contributor Author

@bors-servo try=wpt-2020

@bors-servo
Copy link
Contributor

⌛ Trying commit 020484f with merge 4ef032d...

bors-servo added a commit that referenced this pull request Jun 28, 2023
…gins-2, r=<try>

Properly position floats when subsequent boxes collapse margins with containing block (2)

PR #29939 tried to address this but missed various cases.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29944
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@github-actions
Copy link

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

Flaky unexpected result (1)
  • TIMEOUT [expected PASS] /css/css-ui/appearance-progress-bar-002.html
Stable unexpected results that are known to be intermittent (3)
  • PASS [expected FAIL] /_mozilla/css/dirty_viewport.html (#13731)
  • PASS [expected FAIL] /_mozilla/css/pseudo_content_with_layers.html (#13479)
  • PASS [expected FAIL] /_mozilla/mozilla/iframe/resize_after_load.html (#13573)
Stable unexpected results (1)
  • FAIL [expected PASS] /css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html

@bors-servo
Copy link
Contributor

💔 Test failed - checks-github

@github-actions
Copy link

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

Flaky unexpected result (1)
  • TIMEOUT [expected PASS] /css/css-ui/appearance-progress-bar-002.html
Stable unexpected results that are known to be intermittent (3)
  • PASS [expected FAIL] /_mozilla/css/dirty_viewport.html (#13731)
  • PASS [expected FAIL] /_mozilla/css/pseudo_content_with_layers.html (#13479)
  • PASS [expected FAIL] /_mozilla/mozilla/iframe/resize_after_load.html (#13573)
Stable unexpected results (1)
  • FAIL [expected PASS] /css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html

…containing block (2)

PR servo#29939 tried to address this but missed various cases.
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks like there is a test failing, but this is clearly a progression. Great fix.

@Loirooriol Loirooriol force-pushed the fix-float-placement-with-future-margins-2 branch from 020484f to a435e02 Compare June 29, 2023 09:05
@Loirooriol
Copy link
Contributor Author

@bors-servo r=mrobinson

Yes, I forgot to git add the test expectation. Thanks for the review!

@bors-servo
Copy link
Contributor

📌 Commit a435e02 has been approved by mrobinson

@bors-servo
Copy link
Contributor

⌛ Testing commit a435e02 with merge aabbbfe...

bors-servo added a commit that referenced this pull request Jun 29, 2023
…gins-2, r=mrobinson

Properly position floats when subsequent boxes collapse margins with containing block (2)

PR #29939 tried to address this but missed various cases.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29944
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@github-actions
Copy link

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

Flaky unexpected result (11)
  • OK /_mozilla/mozilla/task_queue_throttling.any.html (#22519)
    • FAIL [expected PASS] subtest: Throttling the performance timeline task queue. assert_true: expected true got false
  • TIMEOUT [expected OK] /_webgl/conformance/uniforms/out-of-bounds-uniform-array-access.html (#26225)
    • NOTRUN [expected PASS] subtest: Overall test
  • TIMEOUT [expected PASS] /css/CSS2/margin-padding-clear/margin-collapse-130.xht
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin.window.html (#29049)
    • FAIL [expected PASS] subtest: Same-origin navigation started from unload handler must be ignored assert_equals: expected "?pass" but got "?fail"
  • TIMEOUT [expected OK] /html/browsers/origin/cross-origin-objects/cross-origin-objects.html (#28569)
  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
    • TIMEOUT [expected FAIL] subtest: Check that popups from a sandboxed iframe escape the sandbox if
      allow-popups-to-escape-sandbox is used Test timed out
  • CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html (#24066)
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/sandbox-top-navigation-child-special-cases.tentative.sub.window.html (#29069)
  • OK /html/semantics/embedded-content/the-img-element/non-active-document.html (#21544)
    • FAIL [expected PASS] subtest: createHTMLDocument assert_unreached: got unexpected error event Reached unreachable code
    • FAIL [expected PASS] subtest: <template> assert_unreached: got unexpected error event Reached unreachable code
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.html (#26371)
    • TIMEOUT [expected FAIL] subtest: delayed handling: delaying handling rejected promise created from createImageBitmap will cause both events to fire Test timed out
  • OK [expected TIMEOUT] /webmessaging/without-ports/017.html (#24486)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, about:blank
Stable unexpected results that are known to be intermittent (10)
  • FAIL [expected PASS] /css/css-text/white-space/trailing-other-space-separators-break-spaces-005.html (#25875)
  • TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
    • PASS [expected FAIL] subtest: border-image sec-fetch-site - HTTPS downgrade (header not sent)
  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/iframe-src-aboutblank-navigate-immediately.html (#29048)
    • PASS [expected FAIL] subtest: Navigating to a different document with link click
  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html (#28681)
    • FAIL [expected PASS] subtest: load & pageshow events do not fire on contentWindow of <iframe> element created with src='' assert_unreached: load should not be fired Reached unreachable code
  • 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."
  • TIMEOUT [expected OK] /html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html (#23205)
    • NOTRUN [expected PASS] subtest: Check that rel=noopener with target=_self does a normal load
  • OK [expected ERROR] /html/semantics/scripting-1/the-script-element/defer-script/async-script.html?reload (#29054)
  • TIMEOUT /html/webappapis/scripting/events/compile-event-handler-settings-objects.html (#24246)
    • FAIL [expected PASS] subtest: The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document assert_equals: expected "/html/webappapis/scripting/events/resources/open-window.html" but got "blank"
  • TIMEOUT [expected CRASH] /webmessaging/broadcastchannel/cross-partition.https.tentative.html (#29058)
  • OK [expected ERROR] /workers/semantics/run-a-worker/003.html (#22765)

@bors-servo
Copy link
Contributor

💔 Test failed - checks-github

@Loirooriol
Copy link
Contributor Author

@bors-servo retry

Unit tests seem fine locally...

@Loirooriol
Copy link
Contributor Author

BTW, if I apply a patch to fix clearance, adjoining-float-nested-forced-clearance-002.html fails because there is a BFC root that is not avoiding floats (we still have to implement that). I'm adding some colors to see what's going on:

The yellow BFC should avoid the magenta and cyan floats and then force the green container to grow taller, like in Blink/WebKit:

Interestingly, Firefox is also failing:

@Loirooriol
Copy link
Contributor Author

@bors-servo r=mrobinson

@bors-servo
Copy link
Contributor

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

📌 Commit a435e02 has been approved by mrobinson

@bors-servo
Copy link
Contributor

⌛ Testing commit a435e02 with merge 7412e28...

@github-actions
Copy link

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

Stable unexpected results that are known to be intermittent (3)
  • PASS [expected FAIL] /_mozilla/css/dirty_viewport.html (#13731)
  • PASS [expected FAIL] /_mozilla/css/pseudo_content_with_layers.html (#13479)
  • PASS [expected FAIL] /_mozilla/mozilla/iframe/resize_after_load.html (#13573)

@github-actions
Copy link

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

Flaky unexpected result (17)
  • OK /_mozilla/css/stylesheet_media_queries.html (#17159)
    • FAIL [expected PASS] subtest: Media queries within stylesheets assert_equals: expected "rgb(0, 255, 0)" but got "rgb(255, 0, 0)"
  • OK /_mozilla/mozilla/task_queue_throttling.any.html (#22519)
    • FAIL [expected PASS] subtest: Throttling the performance timeline task queue. assert_true: expected true got false
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
    • NOTRUN [expected PASS] subtest: Overall test
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-with-non-reserved-words.html (#16216)
    • NOTRUN [expected PASS] subtest: Overall test
  • TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
    • FAIL [expected PASS] subtest: border-image sec-fetch-user - Not sent to non-trustworthy same-site destination assert_unreached: Reached unreachable code
    • TIMEOUT [expected PASS] subtest: background-image sec-fetch-site - HTTPS downgrade (header not sent) 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."
  • TIMEOUT [expected OK] /html/browsers/origin/cross-origin-objects/cross-origin-objects.html (#28569)
  • OK /html/browsers/the-window-object/open-close/creating_browsing_context_test_01.html (#29046)
    • PASS [expected FAIL] subtest: first argument: absolute url
  • OK /html/semantics/embedded-content/media-elements/media_fragment_seek.html (#24114)
    • FAIL [expected PASS] subtest: Video should seek to time specified in media fragment syntax assert_equals: expected 3 but got 0
  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html (#24057)
    • TIMEOUT [expected FAIL] subtest: Check that popups from a sandboxed iframe escape the sandbox if
      allow-popups-to-escape-sandbox is used Test timed out
  • OK /html/semantics/embedded-content/the-img-element/non-active-document.html (#21544)
    • FAIL [expected PASS] subtest: createHTMLDocument assert_unreached: got unexpected error event Reached unreachable code
    • FAIL [expected PASS] subtest: <template> assert_unreached: got unexpected error event Reached unreachable code
  • OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
    • PASS [expected FAIL] subtest: text/plain: 0x00 in name (formdata event)
    • PASS [expected FAIL] subtest: text/plain: 0x00 in value (normal form)
  • TIMEOUT /html/webappapis/scripting/events/compile-event-handler-settings-objects.html (#24246)
    • FAIL [expected PASS] subtest: The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document assert_equals: expected "/html/webappapis/scripting/events/resources/open-window.html" but got "blank"
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.html (#26371)
    • TIMEOUT [expected FAIL] subtest: delayed handling: delaying handling rejected promise created from createImageBitmap will cause both events to fire Test timed out
  • OK [expected TIMEOUT] /webaudio/the-audio-api/the-audiocontext-interface/audiocontext-not-fully-active.html (#27664)
  • OK [expected TIMEOUT] /webmessaging/without-ports/018.html (#24485)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, javascript:
  • OK [expected ERROR] /workers/constructors/Worker/Worker-constructor.html (#22991)
Stable unexpected results that are known to be intermittent (9)

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-github
Approved by: mrobinson
Pushing 7412e28 to master...

@bors-servo bors-servo merged commit 7412e28 into servo:master Jun 30, 2023
10 checks passed
@Loirooriol Loirooriol deleted the fix-float-placement-with-future-margins-2 branch July 1, 2023 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Layout 2020: Floats can still overlap due to margin collapse
3 participants