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

Allow gradient interpolation hints to influence color-stop fixup #28572

Closed
wants to merge 2 commits into from
Closed

Allow gradient interpolation hints to influence color-stop fixup #28572

wants to merge 2 commits into from

Conversation

Bryce-MW
Copy link
Contributor

@Bryce-MW Bryce-MW commented Aug 15, 2021 •
edited
Loading

I am working on the easing gradients feature for CSS. I want to eventually implement that in Servo as a demonstration since Servo is a bit easier (in my opinion) to modify than Firefox or Chromium. However, at this point, Servo does not even support gradient interpolation hints so I am working towards that support. I think that it will require modifications to webrender if we don't want to use Firefox's approximation. I am happy to implement that approximation though if we want something that works now.

This pull request just makes it so that the interpolation hints influence the position of unpositioned stops. Specifically, on my test website, the first and third gradients look the same prior to this fix and they look correct after this fix. It is also important to note that the white colour-stop of the first gradient should be further to the right than the white colour-stop of the second gradient. There's a bit more discussion about this effect here.

I've implemented this in both layout_2017 and layout_2020. I had to change a few things in 2017 that I don't fully understand so I would appreciate if someone could make sure that I didn't cause a subtle bug or performance issue. Especially around line 88 and 116 of components/layout/display_list/gradient.rs.

  • ./mach build -d does not report any errors (and ./mach build -d --with-layout-2020)
  • ./mach test-tidy does not report any errors
  • There are tests for these changes (Should be covered by the reftests)

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list/gradient.rs, components/style/values/specified/image.rs, components/style/values/generics/image.rs

@highfive
Copy link

warning Warning warning

  • These commits modify style and layout code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 15, 2021
@Bryce-MW
Copy link
Contributor Author

Bryce-MW commented Aug 15, 2021 •
edited
Loading

@bors-servo try=wpt-2020

Adding this since I modified layout_2020 (https://github.com/servo/servo/wiki/Layout-2020)

EDIT: I guess I can't do this. That link says to do it though so I guess someone else will have to approve it. I've done the tests locally and they seem to be fine

@bors-servo
Copy link
Contributor

@Bryce-MW: 🔑 Insufficient privileges: not in try users

1 similar comment
@bors-servo
Copy link
Contributor

@Bryce-MW: 🔑 Insufficient privileges: not in try users

@Bryce-MW Bryce-MW mentioned this pull request Aug 15, 2021
Open
@mrobinson
Copy link
Member

@bors-servo try

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

🔨 Triggering try run (#5800530349) with platform=all and layout=all

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

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

Flaky unexpected result (1)
  • 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

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

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

Flaky unexpected result (27)
  • 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 [expected OK] /_webgl/conformance/uniforms/out-of-bounds-uniform-array-access.html (#26225)
    • NOTRUN [expected PASS] subtest: Overall test
  • OK /css/CSS2/linebox/inline-negative-margin-001.html (#23862)
    • FAIL [expected PASS] subtest: [data-expected-height] 1 assert_equals:
      <inline-block data-expected-height="10">123 <span style="margin-left: -8ch">1234 </span></inline-block>
      height expected 10 but got 20
    • FAIL [expected PASS] subtest: [data-expected-height] 2 assert_equals:
      <inline-block data-expected-height="10">123 <span style="margin-left: -8ch">123 </span></inline-block>
      height expected 10 but got 20
    • PASS [expected FAIL] subtest: [data-expected-height] 4
  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html (#28681)
    • PASS [expected FAIL] subtest: load & pageshow events do not fire on contentWindow of <iframe> element created with src='about:blank'
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin-fragment.html (#20768)
    • FAIL [expected PASS] subtest: Tests that a fragment navigation in the unload handler will not block the initial navigation assert_equals: expected "" but got "#fragment"
  • OK /html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/a-click.html (#28697)
    • PASS [expected FAIL] subtest: aElement.click() before the load event must NOT replace
  • OK /html/browsers/history/the-history-interface/traverse_the_history_4.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • TIMEOUT [expected FAIL] /html/canvas/element/manual/text/canvas.2d.disconnected.html (#30063)
  • TIMEOUT [expected OK] /html/interaction/focus/the-autofocus-attribute/document-with-fragment-top.html (#28259)
    • TIMEOUT [expected FAIL] subtest: Autofocus elements in top-level browsing context's documents with "top" fragments should work. Test timed out
  • 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
  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
    • 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 [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
  • TIMEOUT [expected OK] /html/semantics/forms/form-submission-0/form-submit-iframe-then-location-navigate.html (#29634)
    • TIMEOUT [expected PASS] subtest: Verifies that location navigations take precedence when following form submissions. Test timed out
  • OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
    • PASS [expected FAIL] subtest: text/plain: 0x00 in filename (formdata event)
  • OK /html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html (#23205)
    • FAIL [expected PASS] subtest: Check that rel=noopener with target=_self does a normal load this.openedWindow.findLink is not a function
  • OK /html/syntax/parsing/DOMContentLoaded-defer.html (#21550)
    • FAIL [expected PASS] subtest: The end: DOMContentLoaded and defer scripts assert_false: DOMContentLoaded should not have fired before executing a task queued from a defer script expected false got true
  • 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
  • ERROR /resource-timing/content-type-parsing.html (#29131)
    • FAIL [expected TIMEOUT] subtest: mime-type 16 : text/html;charset=�gbk assert_equals: expected (string) "text/html" but got (undefined) undefined
    • TIMEOUT [expected NOTRUN] subtest: mime-type 17 : text/html;charset= gbk Test timed out
  • TIMEOUT [expected CRASH] /webaudio/the-audio-api/the-audiocontext-interface/audiocontextoptions.html (#21408)
  • TIMEOUT [expected OK] /webmessaging/with-ports/018.html (#24485)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, javascript: 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 TIMEOUT] /webmessaging/without-ports/018.html (#24485)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, javascript:
  • 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
  • ERROR [expected OK] /workers/constructors/Worker/Worker-constructor.html (#22991)
Stable unexpected results that are known to be intermittent (10)
  • TIMEOUT /FileAPI/url/url-in-tags-revoke.window.html (#19978)
    • FAIL [expected TIMEOUT] subtest: Opening a blob URL in a new window immediately before revoking it works. assert_equals: expected (string) "test_frame_OK" but got (undefined) undefined
  • OK /css/cssom-view/scroll-behavior-smooth-navigation.html (#29564)
    • FAIL [expected PASS] subtest: Instant scrolling while doing history navigation. assert_equals: Shouldn't be scrolled back to top yet. expected 0 but got 18631
  • TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
    • TIMEOUT [expected PASS] subtest: background-image sec-fetch-user - Not sent to non-trustworthy same-origin destination Test timed out
  • OK /fetch/private-network-access/worker-blob-fetch.tentative.window.html (#30064)
    • FAIL [expected PASS] subtest: private to private: success. assert_equals: fetch error expected (undefined) undefined but got (string) "unknown error"
    • PASS [expected FAIL] subtest: public to public: success.
  • TIMEOUT [expected OK] /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-referrer.window.html (#29081)
    • TIMEOUT [expected PASS] subtest: no-referrer referrer policy used to create the starting page Test timed out
  • TIMEOUT [expected PASS] /html/canvas/element/manual/drawing-text-to-the-canvas/canvas.2d.disconnected-font-size-math.html (#30063)
  • OK /html/semantics/embedded-content/the-img-element/non-active-document.html (#21544)
    • PASS [expected FAIL] subtest: createHTMLDocument
    • PASS [expected FAIL] subtest: <template>
  • TIMEOUT [expected OK] /html/semantics/forms/form-submission-0/reparent-form-during-planned-navigation-task.html (#29724)
    • TIMEOUT [expected PASS] subtest: reparent-form-during-planned-navigation-task Test timed out
  • OK [expected CRASH] /html/semantics/tabular-data/the-table-element/caption-methods.html (#14157, #14026)
  • OK [expected TIMEOUT] /webmessaging/with-ports/017.html (#24486)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, about:blank

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

✨ Try run (#5800530349) succeeded.

@mrobinson
Copy link
Member

@emilio Would you mind taking a look at this one to see if the changes to style are something that you'd want in Gecko?

emilio
emilio requested changes Oct 4, 2023
View reviewed changes
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

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

So the style changes are only needed so that components/layout/display_list/gradient.rs doesn't discard the interpolation hints. That seems like a smell.

It seems like instead convert_gradient_stops could work with GradientItem directly, and then style doesn't need to change at all.

@mrobinson
Copy link
Member

I think this PR might be abandoned? I doesn't really change test results as far as I can see and it needs to be reworked. I'm going to close for now, but @Bryce-MW please feel free to reopen if you are still working on this. Servo is more active now so it's quite likely that this can land with a little bit of TLC.

@mrobinson mrobinson closed this Mar 15, 2024
@Bryce-MW
Copy link
Contributor Author

I gave up on trying to get this merged a long time ago. I haven't been actively working on gradients for a while but if I did come back I'd likely start over rather than reworking this PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio requested changes

Assignees
No one assigned
Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Bryce-MW @highfive @bors-servo @mrobinson @emilio