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

FIDEFE-4626 - Add HCM media queries to built CSS #3

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

hannajones
Copy link

This patch adds support for prefers-contrast and forced-colors media queries by creating and using a custom formatter for our CSS.

@@ -1,4 +1,52 @@
{
"border": {
"color": {
"default": {
Copy link
Author

Choose a reason for hiding this comment

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

I left in the "default" name for now even though it sounds like we might not want that long term. If/when we decide on that we can have a separate task to handle this in a different way.

Copy link
Author

@hannajones hannajones Jan 25, 2024

Choose a reason for hiding this comment

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

Unfortunately it seems like we still need this even after switching to value as an object because we still have the same problem where style-dictionary doesn't parse token names like text-color-deemphasized if it's nested under text-color 😞

Strictly speaking the default here isn't necessary anymore, but I kept it for consistency for now

@hannajones hannajones marked this pull request as draft January 24, 2024 15:23
TGiles added a commit that referenced this pull request Jan 25, 2024
* FIDEFE-4623 - Add CSS formatter for converting tokens to light-dark() calls

I originally thought we would be able to use a transformer to handle
this case, but outputting references causes my transformer to fail.
Because of this, I switched to using a custom CSS formatter and adding
the variable references in this formatter.

Additionally, I also switched over to using config.js instead of
config.json using PR #3 as a starting point.

I also changed the design tokens JSON format to fit what I needed for
the light-dark formatting. I'll need to consolidate my changes with the
changes in PR #3, but wanted to get this PR updated.

* Remove build.js, use css/variables formatter, update tokens structure

This removes the build.js file in favor of using config.js (which
requires `style-dictionary build` to build our files). I also removed
the custom file formatting code in favor of the included css/variables
formatter. Lastly, I updated the tokens structure so instead of
"darkValue", we use "dark".

Most likely the structure of the tokens file will change again, but
I'll create a new PR with those proposed changes.

* Restore custom file formatting, simplify ref logic.

So we can't use the "css/variables" built-in formatter, otherwise we'll
lose our light-dark value. I reverted those changes back to the
mappedValues loop so that we maintain the light-dark formatting.

I additionally sorted the token so that referenced tokens come after
the referenced definitions. I don't think we have to do this for CSS,
but it does make the generated CSS a little easier to follow in my
opinion.

* Remove light-dark formatter and replace with transformer.

We can greatly simplify the config.js file by doing two things:
1. Use a transformer for dealing with light-dark.
2. In this transformer, modify the original value of the token.

By modifying the value of the original token, we are able to take
advantage of the built-in css/variables formatting and so we don't need
custom CSS file formatting.

Additionally, we can use ...StyleDictionary.transformGroup["group_name"]
to get the individual transforms of a group. By doing this we can extend
an existing group as seen in this patch.

* fix eslint error
TGiles pushed a commit that referenced this pull request Jan 25, 2024
…ect> / <embed> as subdocuments. r=longsonr

These look like two really old bugs.

Part of the issue is that <object> / <embed> manage their frame loader quite
differently from <iframe>. This means that we may have a PresShell /
ViewManager / etc for a frame loader that doesn't yet have a frame associated.

For example, this is the viewport creation for the SVG document that reproduces
the problem:

	#0  0x00005cc60e8302e7 in mozilla::ViewportFrame::SetViewInternal(nsView*) (this=0x68599020, aView=0x683d8f00) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/ViewportFrame.h:106
	#1  0x00005cc60e842858 in nsIFrame::SetView(nsView*) (this=0x68599020, aView=0x683d8f00) at /home/emilio/src/moz/gecko/layout/generic/nsFrame.cpp:7057
	#2  0x00005cc60e77258a in nsCSSFrameConstructor::ConstructRootFrame() (this=0xc72c715e00) at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:2424
	#3  0x00005cc60e7342f5 in mozilla::PresShell::Initialize() (this=0x6830e000) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:1758
	#4  0x00005cc60c9fb02a in nsContentSink::StartLayout(bool) (this=<optimized out>, aIgnorePendingSheets=<optimized out>) at /home/emilio/src/moz/gecko/dom/base/nsContentSink.cpp:1160
	#5  0x00005cc60e2c1581 in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int, bool)
	    (this=<optimized out>, aName=<optimized out>, aAtts=0x6fde8200, aAttsCount=<optimized out>, aLineNumber=3, aColumnNumber=<optimized out>, aInterruptable=true) at /home/emilio/src/moz/gecko/dom/xml/nsXMLContentSink.cpp:982
	#6  0x00005cc60e2c17d7 in non-virtual thunk to nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int) () at /home/emilio/src/moz/gecko/dom/xml/nsXMLContentSink.cpp:889
	#7  0x00005cc60c360307 in nsExpatDriver::HandleStartElement(void*, char16_t const*, char16_t const**) (aUserData=0x224f650d0cc0, aName=0x685aef20 u"http://www.w3.org/2000/svg\xffffdesc", aAtts=0x6fde8200)
	    at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:293
	#8  0x00005cc60ee91cea in doContent (parser=0xc72c70f800, startTagLevel=0, enc=<optimized out>, s=<optimized out>, end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7ffca2653288, haveMore=1 '\001')
	    at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:2872
	#9  0x00005cc60ee90059 in contentProcessor (parser=0xc72c70f800, start=0xffffffe6 <error: Cannot access memory at address 0xffffffe6>, end=0xc72c511360 "", endPtr=0x1) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:2528
	#10 0x00005cc60ee8f8d5 in doProlog
	    (parser=<optimized out>, enc=0x5cc612ce0930 <little2_encoding_ns>, s=0x5bbd31ab508e "<", end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, tok=<optimized out>, next=<optimized out>, nextPtr=0x7ffca2653288, haveMore=1 '\001', allowClosingDoctype=1 '\001') at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:4591
	#11 0x00005cc60ee8d86e in prologProcessor (parser=0xc72c70f800, s=0x5bbd31ab5020 "<", end=0x5bbd31af5020 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7ffca2653288) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:4311
	#12 0x00005cc60ee8cf45 in MOZ_XML_Parse (parser=0xc72c70f800, s=0x5bbd31ab5020 "<", len=262144, isFinal=0) at /home/emilio/src/moz/gecko/parser/expat/lib/xmlparse.c:1894
	#13 0x00005cc60c3627bc in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*)
	    (this=0x224f650d0cc0, aBuffer=0x5bbd31ab5020 u"<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>\n<svg height=\"2970\" width=\"2100\" viewBox=\"0 0 2100 2970\" version=\"1.1\" xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlin"..., aLength=131072, aIsFinal=false, aConsumed=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:875
	#14 0x00005cc60c362c91 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) (this=<optimized out>, aScanner=..., aFlushTokens=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsExpatDriver.cpp:970
	#15 0x00005cc60c3666a8 in nsParser::Tokenize(bool) (this=0x224f65038e80, aIsFinalChunk=false) at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:1410
	mozilla#16 0x00005cc60c36595e in nsParser::ResumeParse(bool, bool, bool) (this=0x224f65038e80, allowIteration=true, aIsFinalChunk=false, aCanInterrupt=<optimized out>) at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:961
	mozilla#17 0x00005cc60c366c86 in nsParser::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) (this=0x224f65038e80, request=<optimized out>, pIStream=0x6fdfc430, sourceOffset=<optimized out>, aLength=131072)
	    at /home/emilio/src/moz/gecko/parser/htmlparser/nsParser.cpp:1317
	mozilla#18 0x00005cc60c897cc2 in nsObjectLoadingContent::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) (this=<optimized out>, aRequest=0x68483080, aInputStream=0x6fdfc430, aOffset=0, aCount=131072)
	    at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:1055
	mozilla#19 0x00005cc60b9d18f8 in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int)
	    (this=0x68483000, aRequest=0x68483080, aContext=<optimized out>, aStream=0x6fdfc430, aOffset=0, aCount=743510880) at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:968
	mozilla#20 0x00005cc60b9d5cbf in mozilla::net::HttpChannelChild::OnTransportAndData(nsresult const&, nsresult const&, unsigned long const&, unsigned int const&, nsTString<char> const&)
	    (this=0x68483000, aChannelStatus=<optimized out>, aTransportStatus=@0x683be5bc: -2142568440, aOffset=<optimized out>, aCount=<optimized out>, aData=...) at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:867
	mozilla#21 0x00005cc60badb535 in mozilla::net::ChannelEventQueue::FlushQueue() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/netwerk/ipc/ChannelEventQueue.cpp:90
	mozilla#22 0x00005cc60b976fda in mozilla::net::ChannelEventQueue::MaybeFlushQueue() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/net/ChannelEventQueue.h:350
	mozilla#23 0x00005cc60baec442 in mozilla::net::ChannelEventQueue::CompleteResume() (this=0xc72ce2cae0) at /home/emilio/src/moz/gecko/obj-debug/dist/include/mozilla/net/ChannelEventQueue.h:329
	mozilla#24 mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() (this=<optimized out>) at /home/emilio/src/moz/gecko/netwerk/ipc/ChannelEventQueue.cpp:148
	mozilla#25 0x00005cc60b53d4f1 in mozilla::SchedulerGroup::Runnable::Run() (this=0x685b0200) at /home/emilio/src/moz/gecko/xpcom/threads/SchedulerGroup.cpp:282
	mozilla#26 0x00005cc60b54ff80 in nsThread::ProcessNextEvent(bool, bool*) (this=0x3dd7f4f3020, aMayWait=<optimized out>, aResult=0x7ffca2653ea7) at /home/emilio/src/moz/gecko/xpcom/threads/nsThread.cpp:1220
	mozilla#27 0x00005cc60b552f0d in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x68599020, aMayWait=true) at /home/emilio/src/moz/gecko/xpcom/threads/nsThreadUtils.cpp:481

The parent view may not be set properly if the frame is not current by the time
it is created. For example in this case the parent for the root view is
non-null and comes from the following MakeWindow call:

	#0  nsDocumentViewer::MakeWindow(nsSize const&, nsView*) (this=0xc72ca72cd0, aSize=..., aContainerView=0x683ba500) at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:2368
	#1  0x00005cc60e789b50 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::dom::WindowGlobalChild*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool)
	    (this=0xc72ca72cd0, aParentWidget=<optimized out>, aState=0x0, aActor=0x0, aBounds=..., aDoCreation=true, aNeedMakeCX=<optimized out>, aForceSetNewDocument=<optimized out>)
	    at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:933
	#2  0x00005cc60e789959 in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::dom::WindowGlobalChild*)
	    (this=0xc72ca72cd0, aParentWidget=0x7ffca2651020, aBounds=..., aActor=0x7f6216725c00) at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:762
	#3  0x00005cc60f4f584f in nsDocShell::SetupNewViewer(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x684db000, aNewViewer=<optimized out>, aWindowActor=<optimized out>)
	    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:8017
	#4  0x00005cc60f4f5204 in nsDocShell::Embed(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x684db000, aContentViewer=0x7ffca2651020, aWindowActor=0x683ba500) at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:5745
	#5  0x00005cc60f4dbc7b in nsDocShell::CreateContentViewer(nsTSubstring<char> const&, nsIRequest*, nsIStreamListener**) (this=0x684db000, aContentType=..., aRequest=0x68483080, aContentHandler=<optimized out>)
	    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:7819
	#6  0x00005cc60f4dab99 in nsDSURIContentListener::DoContent(nsTSubstring<char> const&, bool, nsIRequest*, nsIStreamListener**, bool*)
	    (this=0x683056a0, aContentType=..., aIsContentPreferred=<optimized out>, aRequest=0x68483080, aContentHandler=0xc72c5e8608, aAbortProcess=0x7ffca265139f) at /home/emilio/src/moz/gecko/docshell/base/nsDSURIContentListener.cpp:181
	#7  0x00005cc60c2fd8f5 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (this=0xc72c5e85e0, aListener=0x683056a0, aChannel=<optimized out>) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:632
	#8  0x00005cc60c2fccd1 in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (this=0xc72c5e85e0, request=0x68483080, aCtxt=<optimized out>) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:313
	#9  0x00005cc60c2fc5aa in nsDocumentOpenInfo::OnStartRequest(nsIRequest*) (this=<optimized out>, request=0x68483080) at /home/emilio/src/moz/gecko/uriloader/base/nsURILoader.cpp:191
	#10 0x00005cc60c8975c4 in nsObjectLoadingContent::LoadObject(bool, bool, nsIRequest*) (this=0x4b1b3938b6a8, aNotify=<optimized out>, aForceLoad=<optimized out>, aLoadingChannel=0x68483080)
	    at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:2218
	#11 0x00005cc60c89681f in nsObjectLoadingContent::OnStartRequest(nsIRequest*) (this=0x4b1b3938b6a8, aRequest=0x68483080) at /home/emilio/src/moz/gecko/dom/base/nsObjectLoadingContent.cpp:1006
	#12 0x00005cc60b9d1020 in mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) (this=0x68483000, aRequest=0x68483080, aContext=<optimized out>)
	    at /home/emilio/src/moz/gecko/netwerk/protocol/http/HttpChannelChild.cpp:708
	#13 0x00005cc60b9d481b in mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, mozilla::net::ParentLoadInfoForwarderArgs const&, bool const&, bool const&, bool const&, unsigned long const&, int const&, unsigned int const&, nsTString<char> const&, nsTString<char> const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, unsigned int const&, nsTString<char> const&, long const&, bool const&, bool const&, bool const&, mozilla::net::ResourceTimingStructArgs const&, bool const&, mozilla::Maybe<unsigned int> const&, bool const&, nsILoadInfo::CrossOriginOpenerPolicy const&)

However, even though aContainerView is non-null, the view is incorrect, it's
the view for the _old_ frame.

The view parent/child relationship gets cleared properly in:

	#1  0x00005cc60e8e82bf in BeginSwapDocShellsForViews (aSibling=0x0) at /home/emilio/src/moz/gecko/layout/generic/nsSubDocumentFrame.cpp:1027
	warning: Source file is more recent than executable.
	#2  0x00005cc60e8e810b in nsSubDocumentFrame::DestroyFrom (this=0x6cd04eaa45a8, aDestructRoot=0x6cd04eaa45a8, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsSubDocumentFrame.cpp:943
	#3  0x00005cc60e7b71a3 in nsIFrame::Destroy (this=0x6cd04eaa45a8) at /home/emilio/src/moz/gecko/layout/generic/nsIFrame.h:657
	#4  0x00005cc60e80baac in nsBlockFrame::RemoveFrame (this=0x4b1b39362d88, aListID=<optimized out>, aOldFrame=0x6cd04eaa45a8) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:5597
	#5  0x00005cc60e8df29f in nsPlaceholderFrame::DestroyFrom (this=0x4b1b39363240, aDestructRoot=0x4b1b39363240, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsPlaceholderFrame.cpp:181
	#6  0x00005cc60e80cf19 in nsBlockFrame::DoRemoveFrameInternal (this=<optimized out>, aDeletedFrame=0x0, aFlags=<optimized out>, aPostDestroyData=...) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:6265
	#7  0x00005cc60e82d947 in nsBlockFrame::DoRemoveFrame (this=0x4b1b39362d88, aDeletedFrame=0x683d8f00, aFlags=244338087) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.h:528
	#8  0x00005cc60e80ba3a in nsBlockFrame::RemoveFrame (this=0x4b1b39362d88, aListID=<optimized out>, aOldFrame=0x4b1b39363240) at /home/emilio/src/moz/gecko/layout/generic/nsBlockFrame.cpp:5581
	#9  0x00005cc60e77fd5c in nsCSSFrameConstructor::ContentRemoved (this=<optimized out>, aChild=0x4b1b3938b600, aOldNextSibling=<optimized out>, aFlags=<optimized out>)
	    at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:7583
	#10 0x00005cc60e779a35 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x6fdf9800, aContent=0x4b1b3938b600, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Sync)
	    at /home/emilio/src/moz/gecko/layout/base/nsCSSFrameConstructor.cpp:8593
	#11 0x00005cc60e751745 in mozilla::RestyleManager::ProcessRestyledFrames (this=<optimized out>, aChangeList=...) at /home/emilio/src/moz/gecko/layout/base/RestyleManager.cpp:1484

But the temporary state is stored in the _old_ frame-loader, so when we create
the new frame, we get to nsSubDocumentFrame::Init, and find nothing, and thus
go through nsFrameLoader::Show. But we do have a pres-shell, and
nsFrameLoader::Show just early-returns then, and thus we end up with a detached
pres shell which is not hooked to the view tree and thus not painted...

So there are multiple potential fixes. First (this is the approach this patch
takes):

 * Make nsHideViewer not fail to hide a presentation when the frame loader
   has changed. This is not an issue per se, but leaves stale views / etc
   living for too long which is not nice.

 * Fix up the Show() code path to handle this case properly by parenting the
   pres-shell and initializing the docshell properly.

Second potential fix would be to store the temporary state somewhere else than
the frame loader (like the element). This may be a less invasive change
somehow, but it looks pretty fishy to me, and not particularly better...

Terribly sorry about the lack of test-case, but this is racy as crazy and I had
a lot of trouble to even reproduce it in a debug build. This needs the
PresShell creation for the subdocument to happen right after setting .data on
the <object>, but before processing its reframe.

Differential Revision: https://phabricator.services.mozilla.com/D69487
@hannajones hannajones force-pushed the hcm-media-queries branch 2 times, most recently from d75abc1 to aeccc1c Compare January 25, 2024 21:39
@hannajones hannajones marked this pull request as ready for review January 25, 2024 21:39
Copy link

@TGiles TGiles left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, thanks for that! I'm getting test failures though, is that expected?

$ npm run test

> design-system@1.0.0 test
> jest --testPathPattern=tests

  console.log

    css

      at Object.buildPlatform (node_modules/style-dictionary/lib/buildPlatform.js:41:11)
          at Array.forEach (<anonymous>)

  console.log
    ✔︎ tests/build/css/tokens-shared.css

      at buildFile (node_modules/style-dictionary/lib/buildFile.js:114:13)
          at Array.forEach (<anonymous>)
          at Array.forEach (<anonymous>)

 FAIL  tests/config.test.js
  generated CSS
    css/variables/hcm format
      √ should contain three blocks of CSS, including media queries (3 ms)
      × should produce the expected base CSS rules (6 ms)
      √ should produce the expected prefers-contrast CSS rules
      √ should produce the expected forced-colors CSS rules

  ● generated CSS › css/variables/hcm format › should produce the expected base CSS rules

    expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -1,10 +1,10 @@
      Object {
        "--border-radius-circle": "9999px",
        "--border-radius-medium": "8px",
        "--border-radius-small": "4px",
    -   "--border-width-default": "1px",
    +   "--border-width": "1px",
        "--color-background-critical": "light-dark(var(--color-red-05), var(--color-red-80))",
        "--color-background-information": "light-dark(var(--color-blue-05), var(--color-blue-80))",
        "--color-background-success": "light-dark(var(--color-green-05), var(--color-yellow-80))",
        "--color-background-warning": "light-dark(var(--color-yellow-05), var(--color-blue-80))",
        "--color-black-a10": "rgba(0, 0, 0, 0.1)",

      114 |         }, {});
      115 |
    > 116 |         expect(formattedCSS).toStrictEqual(FIXTURE_BY_QUERY[queryName]);
          |                              ^
      117 |       });
      118 |     });
      119 |   });

      at Object.toStrictEqual (tests/config.test.js:116:30)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 3 passed, 4 total
Snapshots:   0 total
Time:        0.595 s, estimated 2 s
Ran all test suites matching /tests/i.

@hannajones
Copy link
Author

Code changes look good to me, thanks for that! I'm getting test failures though, is that expected?

Whoops missed that, thanks! Updated the JSON to have fewer -default references since we're moving away from that + updated the test fixtures 👍🏻

@hannajones hannajones requested a review from TGiles January 26, 2024 18:40
Copy link

@TGiles TGiles 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 the patch!

Copy link
Collaborator

@mstriemer mstriemer left a comment

Choose a reason for hiding this comment

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

This looks awesome, could land as is but I've got a few comments (should be filed if not fixed here)

I also noticed that the tokens-shared.css doesn't match what I get when I run npm run build - --border-width-default becomes --border-width

Also noticed the output from running the build is shown in the test output:


  console.log

    css

      at Object.buildPlatform (node_modules/style-dictionary/lib/buildPlatform.js:41:11)
          at Array.forEach (<anonymous>)

  console.log
    ✔︎ tests/build/css/tokens-shared.css

      at buildFile (node_modules/style-dictionary/lib/buildFile.js:114:13)
          at Array.forEach (<anonymous>)
          at Array.forEach (<anonymous>)

Maybe we should file to disable that when testing (hopefully that's possible)

toolkit/themes/shared/design-system/config.js Outdated Show resolved Hide resolved
toolkit/themes/shared/design-system/config.js Show resolved Hide resolved
toolkit/themes/shared/design-system/config.js Show resolved Hide resolved
@hannajones
Copy link
Author

I also noticed that the tokens-shared.css doesn't match what I get when I run npm run build - --border-width-default becomes --border-width

D'oh I updated that after Tim's comment but forgot to commit the newer version of the built CSS 😅 It should just be border-width everywhere now

Also noticed the output from running the build is shown in the test output
Maybe we should file to disable that when testing (hopefully that's possible)

Looks like SD doesn't support log levels yet, but I was able to turn it off by just running the Jest tests with a --silent flag. Seems like something we'll have to re-visit if/when we convert these to mochitests anyway.

@hannajones hannajones merged commit fc44ab9 into json-design-tokens Jan 29, 2024
TGiles added a commit that referenced this pull request Feb 15, 2024
* FIDEFE-4623 - Add CSS formatter for converting tokens to light-dark() calls

I originally thought we would be able to use a transformer to handle
this case, but outputting references causes my transformer to fail.
Because of this, I switched to using a custom CSS formatter and adding
the variable references in this formatter.

Additionally, I also switched over to using config.js instead of
config.json using PR #3 as a starting point.

I also changed the design tokens JSON format to fit what I needed for
the light-dark formatting. I'll need to consolidate my changes with the
changes in PR #3, but wanted to get this PR updated.

* Remove build.js, use css/variables formatter, update tokens structure

This removes the build.js file in favor of using config.js (which
requires `style-dictionary build` to build our files). I also removed
the custom file formatting code in favor of the included css/variables
formatter. Lastly, I updated the tokens structure so instead of
"darkValue", we use "dark".

Most likely the structure of the tokens file will change again, but
I'll create a new PR with those proposed changes.

* Restore custom file formatting, simplify ref logic.

So we can't use the "css/variables" built-in formatter, otherwise we'll
lose our light-dark value. I reverted those changes back to the
mappedValues loop so that we maintain the light-dark formatting.

I additionally sorted the token so that referenced tokens come after
the referenced definitions. I don't think we have to do this for CSS,
but it does make the generated CSS a little easier to follow in my
opinion.

* Remove light-dark formatter and replace with transformer.

We can greatly simplify the config.js file by doing two things:
1. Use a transformer for dealing with light-dark.
2. In this transformer, modify the original value of the token.

By modifying the value of the original token, we are able to take
advantage of the built-in css/variables formatting and so we don't need
custom CSS file formatting.

Additionally, we can use ...StyleDictionary.transformGroup["group_name"]
to get the individual transforms of a group. By doing this we can extend
an existing group as seen in this patch.

* fix eslint error
TGiles pushed a commit that referenced this pull request Feb 15, 2024
* FIDEFE-4626 - Add HCM media queries to built CSS

* remove some of the 'default' references from JSON, fix test fixtures

* address review comments
TGiles pushed a commit that referenced this pull request Feb 26, 2024
Upstream commit: https://webrtc.googlesource.com/src/+/3df661f79828194d0acb51a1480833bafd9e5066
    [121] Allow RTP retransmission for cloned encoded Video Frames

    Fix the unintended disabling of RTP retransmissions for cloned encoded
    frames, caused by passing an infinite "expected_retransmission_time".
    Instead use a constant 10ms for now. For frames encoded locally, this is
    set from an estimate of the RTT, but we currently don't have access to
    that here (TODO added to pipe it through)

    If an integration is cloning and then sending frames it received, it's
    almost certainly resending received media to other peers on a local
    network, so 10ms is a fair upperbound.

    Tested locally with Chrome on Mac, configuring packet drops & observing
    on chrome://webrtc-internals that retransmission packets are now sent.

    (cherry picked from commit 3e801c32086be59e502e276ff5d6beea42069582)

    No-Try: True
    Bug: chromium:1512631
    Change-Id: I2483415dc7e0079f8a7b66f6607f4907698514c4
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/331900
    Reviewed-by: Harald Alvestrand <hta@webrtc.org>
    Commit-Queue: Tony Herre <herre@google.com>
    Cr-Original-Commit-Position: refs/heads/main@{#41405}
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/332261
    Reviewed-by: Stefan Holmer <stefan@webrtc.org>
    Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
    Reviewed-by: Henrik Boström <hbos@webrtc.org>
    Cr-Commit-Position: refs/branch-heads/6167@{#3}
    Cr-Branched-From: ece5cb83715dea85617114b6d4e981fdee2623ba-refs/heads/main@{#41315}
TGiles added a commit that referenced this pull request Mar 5, 2024
* FIDEFE-4623 - Add CSS formatter for converting tokens to light-dark() calls

I originally thought we would be able to use a transformer to handle
this case, but outputting references causes my transformer to fail.
Because of this, I switched to using a custom CSS formatter and adding
the variable references in this formatter.

Additionally, I also switched over to using config.js instead of
config.json using PR #3 as a starting point.

I also changed the design tokens JSON format to fit what I needed for
the light-dark formatting. I'll need to consolidate my changes with the
changes in PR #3, but wanted to get this PR updated.

* Remove build.js, use css/variables formatter, update tokens structure

This removes the build.js file in favor of using config.js (which
requires `style-dictionary build` to build our files). I also removed
the custom file formatting code in favor of the included css/variables
formatter. Lastly, I updated the tokens structure so instead of
"darkValue", we use "dark".

Most likely the structure of the tokens file will change again, but
I'll create a new PR with those proposed changes.

* Restore custom file formatting, simplify ref logic.

So we can't use the "css/variables" built-in formatter, otherwise we'll
lose our light-dark value. I reverted those changes back to the
mappedValues loop so that we maintain the light-dark formatting.

I additionally sorted the token so that referenced tokens come after
the referenced definitions. I don't think we have to do this for CSS,
but it does make the generated CSS a little easier to follow in my
opinion.

* Remove light-dark formatter and replace with transformer.

We can greatly simplify the config.js file by doing two things:
1. Use a transformer for dealing with light-dark.
2. In this transformer, modify the original value of the token.

By modifying the value of the original token, we are able to take
advantage of the built-in css/variables formatting and so we don't need
custom CSS file formatting.

Additionally, we can use ...StyleDictionary.transformGroup["group_name"]
to get the individual transforms of a group. By doing this we can extend
an existing group as seen in this patch.

* fix eslint error
TGiles pushed a commit that referenced this pull request Mar 5, 2024
* FIDEFE-4626 - Add HCM media queries to built CSS

* remove some of the 'default' references from JSON, fix test fixtures

* address review comments
TGiles added a commit that referenced this pull request Mar 8, 2024
* FIDEFE-4623 - Add CSS formatter for converting tokens to light-dark() calls

I originally thought we would be able to use a transformer to handle
this case, but outputting references causes my transformer to fail.
Because of this, I switched to using a custom CSS formatter and adding
the variable references in this formatter.

Additionally, I also switched over to using config.js instead of
config.json using PR #3 as a starting point.

I also changed the design tokens JSON format to fit what I needed for
the light-dark formatting. I'll need to consolidate my changes with the
changes in PR #3, but wanted to get this PR updated.

* Remove build.js, use css/variables formatter, update tokens structure

This removes the build.js file in favor of using config.js (which
requires `style-dictionary build` to build our files). I also removed
the custom file formatting code in favor of the included css/variables
formatter. Lastly, I updated the tokens structure so instead of
"darkValue", we use "dark".

Most likely the structure of the tokens file will change again, but
I'll create a new PR with those proposed changes.

* Restore custom file formatting, simplify ref logic.

So we can't use the "css/variables" built-in formatter, otherwise we'll
lose our light-dark value. I reverted those changes back to the
mappedValues loop so that we maintain the light-dark formatting.

I additionally sorted the token so that referenced tokens come after
the referenced definitions. I don't think we have to do this for CSS,
but it does make the generated CSS a little easier to follow in my
opinion.

* Remove light-dark formatter and replace with transformer.

We can greatly simplify the config.js file by doing two things:
1. Use a transformer for dealing with light-dark.
2. In this transformer, modify the original value of the token.

By modifying the value of the original token, we are able to take
advantage of the built-in css/variables formatting and so we don't need
custom CSS file formatting.

Additionally, we can use ...StyleDictionary.transformGroup["group_name"]
to get the individual transforms of a group. By doing this we can extend
an existing group as seen in this patch.

* fix eslint error
TGiles pushed a commit that referenced this pull request Mar 8, 2024
* FIDEFE-4626 - Add HCM media queries to built CSS

* remove some of the 'default' references from JSON, fix test fixtures

* address review comments
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.

3 participants