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

[css-fonts] Reconsider the definition of "first available font" #4796

Closed
jfkthame opened this issue Feb 20, 2020 · 24 comments
Closed

[css-fonts] Reconsider the definition of "first available font" #4796

jfkthame opened this issue Feb 20, 2020 · 24 comments

Comments

@jfkthame
Copy link
Contributor

jfkthame commented Feb 20, 2020

The spec currently says:

The first available font, used for example in the definition of font-relative lengths such as 'ex' or in the definition of the 'line-height' property is defined to be the first available font that would match the U+0020 (space) character given font families in the 'font-family' list (or a user agent’s default font if none are available).

I recently landed a patch to make Gecko conform more closely to this behavior, but backed it out because it caused significant regressions. On looking into things, I think the behavior specified here is not in fact desirable or helpful, and should be altered.

The problem arises because of the text "that would match the U+0020 (space) character". This, by any logical reading I can think of, means that a font that does not support U+0020 is not eligible to be considered the first available font, and therefore the next in the list (or the browser's default) must be used as the basis for 'ex' or line-height computations.

This leads to poor behavior when a site does something like

.foo {
    font-family: "my-icons", "Blank";
}
@font-face {
    font-family: "my-icons";
    src: url("my-icons.woff2") format("woff2");
}
@font-face {
    font-family: "Blank";
    src: url("data:font/opentype;base64,T1RUTwAKAIAAA ....");
}

(this is simplified from CSS on tumblr.com), where "my-icons" is an icon font that supports only a set of PUA codepoints; it does not have any regular ASCII characters, not even <space>; and "Blank" is the Adobe Blank font in base64 format. It's presumably used here to avoid the PUA codepoints for the icons getting rendered as hexboxes/last-resort glyphs/tofu/whatever while waiting for the real icon font to load.

However, when the browser needs to refer to the ascent and descent of the first available font during computation of leading and half-leading, or to resolve a font-relative unit such as 'ex', it should -- if it accurately implements this part of the spec -- ignore the "my-icons" font, because that font does not "match the U+0020 (space) character", and therefore it must look at "Blank" to get the metrics to use.

This is unfortunate in this example because "Blank" has a 'cmap' table that maps all one million-plus possible Unicode codepoints to its 2K empty glyphs, and although the font data is not especially huge, it may take a long time to load this font.

It would be similarly unfortunate if the second font were a large webfont resource that would end up getting downloaded only to provide the size of the 'ex' unit (for example), or the normal line height of an empty element. But that seems to be what the spec text requires.

My testing indicates that browsers currently do use metrics from the "my-icons" font in an example like this, contrary to the spec. If the @font-face rule is given a unicode-range descriptor that excludes U+0020, then they do not treat it as the first available font, which corresponds with the cases discussed in #1765.

I think the problem is that in addressing the earlier issue, the spec text defining first available font was written in such a way that it appears to care about actual character support in the font, whereas what was really of interest was potential character coverage as specified by unicode-range.

My suggestion, therefore, is to modify the definition quoted above to something like:

The first available font, used for example in the definition of font-relative lengths such as 'ex' or in the definition of the 'line-height' property is defined to be the first available font that is not excluded by a unicode-range descriptor from matching the U+0020 space character, given font families in the 'font-family' list (or a user agent’s default font if none are available).

A webfont without an explicit unicode-range descriptor, or a locally-installed font used directly via font-family: <name>, will always be eligible to be the first available font, regardless of whether it supports U+0020 or not. Only webfonts that explicitly (using unicode-range) declare their non-support for U+0020 will be excluded.

@svgeesus
Copy link
Contributor

Related: #1800 #1804 and

WebKit doesn’t do this; it just uses the primary font, and if the primary font doesn’t support “0” then it uses that font’s .notdef glyph. No downloads necessary.

and

[So my preference would be an ordered list of things to try:

@svgeesus
Copy link
Contributor

I am supportive of the specific text proposed by @jfkthame but would like to hear from other implementers

@kojiishi
Copy link
Contributor

Blink's logic is in FontFallbackList::DeterminePrimarySimpleFontData:

    if (font_data->IsSegmented() &&
        !ToSegmentedFontData(font_data)->ContainsCharacter(kSpaceCharacter))
      continue;

and as far as I can read from our code, SegmentedFontData::ContainsCharacter does not seem to check cmap.

@jfkthame, do you have a simple test case I can double check? /cc @drott

@jfkthame
Copy link
Contributor Author

@kojiishi See http://wpt.live/css/css-values/ex-unit-004.html. This was designed to test the behavior as currently written in the spec: it uses the ex unit on an element where the first font listed in font-family is a font that does not include a space character and therefore (per current spec) should not be used as "first available font". All browsers currently fail this test -- they do use the test font as the basis for ex.

Adding a unicode-range: U+78; descriptor to the @font-face rule that loads the ExTestNoSpace font in that test makes it pass in both Firefox and Chrome, as they then do ignore that font for "first available font" purposes; this is the behavior I'm suggesting the spec should require.

@kojiishi
Copy link
Contributor

See http://wpt.live/css/css-values/ex-unit-004.html

Thank you, confirmed the code above does not check cmap, only checks unicode-range.

@faceless2
Copy link

@jfkthame's proposal works for us, for what that's worth.

For reference I want to flag up issue with the related test http://wpt.live/css/css-values/ch-unit-017.html which is - as currently specified - incorrect, because the font being used for font metrics doesn't include the space character. This change proposed in this issue would make that test work as described.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-fonts] Reconsider the definition of "first available font", and agreed to the following:

  • RESOLVED: Amend our algo for selecting first available font to check for the 0 glyph, if not there check for notdef glyph, else use 0.5em
  • RESOLVED: Remove previous resolution, discussion continues on github
The full IRC log of that discussion <dael> Topic: [css-fonts] Reconsider the definition of "first available font"
<dael> github: https://github.com//issues/4796
<dael> chris: Not all fonts have glyph for space which means not first available font
<dael> chris: Prop is a three way thing where thirds is width of notdef glyph which all fonts are required to have
<dael> astearns: I see that's 2nd on list
<dael> chris: Yes, sorry
<dael> astearns: If notdef is guar?
<dael> TabAtkins: Required and there are different. I'm happy with this
<dael> astearns: Is it what browsers impl?
<dael> chris: We believe so
<dael> chris: There's a WPT that's wrong right now but it would become correct with this change. I suspect that means browsers do what this is proposed
<dael> astearns: Prop: Amend our algo for selecting first available font to check for the 0 glyph, if not there check for notdef glyph, else use 0.5em
<dael> astearns: Objections?
<dael> RESOLVED: Amend our algo for selecting first available font to check for the 0 glyph, if not there check for notdef glyph, else use 0.5em
<dael> chris: My proposal is in the issue comments
<dael> fantasai: I think ti's different then original proposal so wondering if he was okay. I've got your proposal and one from Johnathan
<dael> chris: Right, his inclues unicode range so if the unicode range excludes space. Not about if font excludes space then it counts as not having one.
<dael> astearns: And if you use unicode range you exclude notdef so it falls to 0/5em
<dael> chris: notdef isn't excludable with unicode range
<dael> fantasai: resolution defines ch unit but first available font is a lot more general. I'm a little confused. 4796 from what I understand is the new definition is first space checking for first space. He prop not checking if there's a space but if unicode range on font would cover space character. THen you prop check for 0
<dael> chris: Not quite. If it doesn't have a 0 then you can use notdef glyph rather then say we fail. That's compat with webkit
<dael> fantasai: So to find first available font I check for a space, then for a 0, then notdef glyph, then use 0.5em? 0.5em is not a font.
<dael> chris: Right. I see what you're saying.
<dael> astearns: Talking about how to determine a measure from a first available font without a space or a 0 or notdef
<dael> fantasai: ch has a definition of use 0 or 0.5em. If you're looking for x there's other metrics. Here we're looking for what font file.
<dael> astearns: We're over-time. We should undo previous resolution and bring up later
<dael> RESOLVED: Remove previous resolution, discussion continues on github

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-fonts] Reconsider the definition of "first available font" #4796, and agreed to the following:

  • RESOLVED: The first available font is the first font in the font-family list whose unicode includes the space glyph
The full IRC log of that discussion <dael> Topic: [css-fonts] Reconsider the definition of "first available font" #4796
<dael> github: https://github.com//issues/4796
<dael> AmeliaBR: fantasai comment is resolution from a couple months ago was illogical so we should take it from there
<dael> fantasai: We rescinded the resolution in the same session b/c we had it could be 0.5 em or a glyph but fonts are neither so makes no sense. I think we start at the top.
<dael> Rossen_: What's the proposal?
<Rossen_> q
<dael> florian: It's okay if the first available font does not contain all glyphs. I think ch unit uses 0 glyph, ic uses a particular character. If the glyphs are missing the units can say I default to 1em or whatever. But we should still figure out what the first available font is
<dael> AmeliaBR: So one prop is the first font in the font stack with any valid glyphs is your first available font?
<dael> myles: That's what we do in WK. THe first font is the primary font and if it doesn't support the characters we'll use another one
<dael> florian: Problem in the spec is it says first font if it doesn't cotnain space it doesn't count
<dael> florian: One reason is common to put first font and reduce it via the unicode range. You're just using that font for & or something and want rest of the stack for the rest.
<dael> florian: As jfkthame pointed out it's if the font contains. If the change to if the unicode range it's webcompat
<dael> dbaron: Suggestion in issue is change it to be if unicode range matches the space and than it was added what to fallback to if there's not a glyph
<dael> fantasai: sounds like it's what we did before is for the value of ch and than ammend algo for first available font to see if space is in its unicode range
<dael> florian: And for ch unit it already says that. It says in [reads] (it's 0.5em wide)
<dael> florian: Could be interpreted as if not in first available font
<fantasai> https://www.w3.org/TR/css-values-3/#ch
<fantasai> "In the cases where it is impossible or impractical to determine the measure of the “0” glyph, it must be assumed to be 0.5em wide by 1em tall."
<dael> AmeliaBR: Can someone type up a resolution text for the proposal?
<fantasai> "Equal to the used advance measure of the "0" (ZERO, U+0030) glyph found in the font used to render it."
<dael> dbaron: I don't quite understand what happened last time, though we did retract original resolution in that discussion
<dael> florian: b/c made no sense. We said font becomes 0.5em which makes no sense.
<myles> Proposed Resolution: The first available font is the first font in the font-family list whose unicode-range supports the space glyph
<dael> fantasai: I put in text from Values. It says ch is the advance measure of the 0 in font used to render it. WHich is not same as first available. So that's separate. Need to resolve what first available font is. If there's an issue with ch it's separate.
<dbaron> I think a more specific proposed resolution is to accept jfkthame's proposal at the end of https://github.com//issues/4796#issue-568427691
<dael> fantasai: We should take prop from myles and if there's an issue for def of ch that's sep against values and units
<dael> AmeliaBR: On prop resolution do we have concept in spec what unicode means for system and generic
<dael> myles: Assumed to have definite range of every unicode character. Spec lists it out.
<AmeliaBR> s/unicode/unicode-range/
<dael> florian: Another clarification. If the first font in stack doesn't have space we skip it. If it does do we look if it has the glyph?
<dael> myles: We do not
<dael> Rossen_: Thoughts or objections to "The first available font is the first font in the font-family list whose unicode-range supports the space glyph"
<AmeliaBR> s/unicode-range character/unicode character/
<fantasai> s/supports/includes/
<AmeliaBR> s/what unicode means/what unicode-range means/
<dael> dbaron: There was working in issue
<dael> jfkthame: Should be talking about character not range
<dael> RESOLVED: The first available font is the first font in the font-family list whose unicode includes the space glyph
<fantasai> s/glyph/character/
<fantasai> s/unicode/unicode-range/
<dbaron> s/working/wording/
<AmeliaBR> s/first font/first available/

@svgeesus
Copy link
Contributor

svgeesus commented May 7, 2020

The resolution as listed makes little sense and the large number of s/foo/bar in the IRC (which don't get applied, when content goes to the issue) makes it even harder to figure out.

@svgeesus
Copy link
Contributor

svgeesus commented May 7, 2020

I'm guessing that

RESOLVED: The first available font is the first available font in the font-family list whose unicode-range includes the space character

is meant?

@litherum
Copy link
Contributor

litherum commented May 7, 2020

Yes.

@svgeesus
Copy link
Contributor

svgeesus commented May 7, 2020

And it does not matter whether the font actually has a glyph for that character.

@kojiishi
Copy link
Contributor

kojiishi commented May 8, 2020

And it does not matter whether the font actually has a glyph for that character.

Yes. As @dbaron mentioned, the text @jfkthame proposed is what the resolution is meant.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 12, 2021
This patch makes the following test more focus on testing the
`size-adjust` descriptor by eliminating dependencies to other
parts of the spec:
  external/wpt/css/css-fonts/size-adjust-01.html

Before this change, the test had:
```
@font-face {
  font-family: large-font;
  unicode-range: U+41-5A; /* Uppercase ASCII only */
}
.large {
  font-family: large-font, sans-serif;
}
```
In CSS, the [first available font] is defined as:
[first available font]: https://drafts.csswg.org/css-fonts-4/#first-available-font
> the first available font that would match the U+0020 (space)
> character

which was further clarified in
w3c/csswg-drafts#4796:
> RESOLVED: The first available font is the first available
> font in the font-family list whose unicode-range includes
> the space character

From these, the [first available font] of the `.large` in the
test is `40px sans-serif`, while it is `60px sans-serif` in
the reference.

The current CSS does not define the exact baseline position
when multiple fonts are used as the result of cascading. Blink
uses the metrics from the [first available font], making this
test fail to match the reference.

This patch adds U+20 to the `@font-family`, to make it the
[first available font].

Bug: 1196631
Change-Id: I8de5b8a1ef22ffde5c791164c8183e17264c14c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2817869
Commit-Queue: Koji Ishii <[email protected]>
Reviewed-by: Dominik Röttsches <[email protected]>
Reviewed-by: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#871419}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 12, 2021
This patch makes the following test more focus on testing the
`size-adjust` descriptor by eliminating dependencies to other
parts of the spec:
  external/wpt/css/css-fonts/size-adjust-01.html

Before this change, the test had:
```
@font-face {
  font-family: large-font;
  unicode-range: U+41-5A; /* Uppercase ASCII only */
}
.large {
  font-family: large-font, sans-serif;
}
```
In CSS, the [first available font] is defined as:
[first available font]: https://drafts.csswg.org/css-fonts-4/#first-available-font
> the first available font that would match the U+0020 (space)
> character

which was further clarified in
w3c/csswg-drafts#4796:
> RESOLVED: The first available font is the first available
> font in the font-family list whose unicode-range includes
> the space character

From these, the [first available font] of the `.large` in the
test is `40px sans-serif`, while it is `60px sans-serif` in
the reference.

The current CSS does not define the exact baseline position
when multiple fonts are used as the result of cascading. Blink
uses the metrics from the [first available font], making this
test fail to match the reference.

This patch adds U+20 to the `@font-family`, to make it the
[first available font].

Bug: 1196631
Change-Id: I8de5b8a1ef22ffde5c791164c8183e17264c14c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2817869
Commit-Queue: Koji Ishii <[email protected]>
Reviewed-by: Dominik Röttsches <[email protected]>
Reviewed-by: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#871419}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Apr 12, 2021
This patch makes the following test more focus on testing the
`size-adjust` descriptor by eliminating dependencies to other
parts of the spec:
  external/wpt/css/css-fonts/size-adjust-01.html

Before this change, the test had:
```
@font-face {
  font-family: large-font;
  unicode-range: U+41-5A; /* Uppercase ASCII only */
}
.large {
  font-family: large-font, sans-serif;
}
```
In CSS, the [first available font] is defined as:
[first available font]: https://drafts.csswg.org/css-fonts-4/#first-available-font
> the first available font that would match the U+0020 (space)
> character

which was further clarified in
w3c/csswg-drafts#4796:
> RESOLVED: The first available font is the first available
> font in the font-family list whose unicode-range includes
> the space character

From these, the [first available font] of the `.large` in the
test is `40px sans-serif`, while it is `60px sans-serif` in
the reference.

The current CSS does not define the exact baseline position
when multiple fonts are used as the result of cascading. Blink
uses the metrics from the [first available font], making this
test fail to match the reference.

This patch adds U+20 to the `@font-family`, to make it the
[first available font].

Bug: 1196631
Change-Id: I8de5b8a1ef22ffde5c791164c8183e17264c14c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2817869
Commit-Queue: Koji Ishii <[email protected]>
Reviewed-by: Dominik Röttsches <[email protected]>
Reviewed-by: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#871419}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 15, 2021
…adjust`, a=testonly

Automatic update from web-platform-tests
Make size-adjust-01 more focus on `size-adjust`

This patch makes the following test more focus on testing the
`size-adjust` descriptor by eliminating dependencies to other
parts of the spec:
  external/wpt/css/css-fonts/size-adjust-01.html

Before this change, the test had:
```
@font-face {
  font-family: large-font;
  unicode-range: U+41-5A; /* Uppercase ASCII only */
}
.large {
  font-family: large-font, sans-serif;
}
```
In CSS, the [first available font] is defined as:
[first available font]: https://drafts.csswg.org/css-fonts-4/#first-available-font
> the first available font that would match the U+0020 (space)
> character

which was further clarified in
w3c/csswg-drafts#4796:
> RESOLVED: The first available font is the first available
> font in the font-family list whose unicode-range includes
> the space character

From these, the [first available font] of the `.large` in the
test is `40px sans-serif`, while it is `60px sans-serif` in
the reference.

The current CSS does not define the exact baseline position
when multiple fonts are used as the result of cascading. Blink
uses the metrics from the [first available font], making this
test fail to match the reference.

This patch adds U+20 to the `@font-family`, to make it the
[first available font].

Bug: 1196631
Change-Id: I8de5b8a1ef22ffde5c791164c8183e17264c14c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2817869
Commit-Queue: Koji Ishii <[email protected]>
Reviewed-by: Dominik Röttsches <[email protected]>
Reviewed-by: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#871419}

--

wpt-commits: 2181d00ec2c4f91be714a025cd60690b4e5f233c
wpt-pr: 28443
@svgeesus
Copy link
Contributor

http://wpt.live/css/css-values/ex-unit-004.html this test needs to be altered so the previous fail condition is now the pass condition

@nigelmegitt
Copy link

Just been drawn to this via w3c/ttml2#1242 (comment) and I have a question. Given the updated text defines "first available font" in terms of the unicode-range property, which itself is a descriptor that is defined for @font-face, what is the intent relating to fonts that are available, installed, and referenced by font-family without an @font-face rule?

Since such fonts do not have an applicable unicode-range property, they can never be the "first available font", even if they do support the 0x0020 code point. Is there an assumption that the character map for each font in the font-family list will be queried (of course, after applying any @font-face rules)? If so, it seems like we should use some other term than unicode-range. For example could we change:

the first font whose unicode-range includes the U+0020 (space) character

to

the first font whose character map includes the U+0020 (space) character

?

@jfkthame
Copy link
Contributor Author

jfkthame commented Nov 4, 2022

I suppose my unspoken assumption has been that installed fonts referenced directly by family name implicitly have a Unicode range of U+0000 - U+10FFFF, i.e. just like a @font-face resource with no explicit unicode-range, they can be used for any character supported by their character map.

I think the reason "first available font" is defined in terms of unicode-range rather than character map is that unicode-range is available to the browser without having to download the resource.

@nigelmegitt
Copy link

Thanks @jfkthame, good to know we agree on the intent. My concern is more definitional: fonts simply do not have a unicode-range as defined; only @font-face rules have that descriptor. I agree that the UA has access to the set of unicode points that each available font claims to support, but that's a different thing to unicode-range, as things are defined today, is it not?

@jfkthame
Copy link
Contributor Author

jfkthame commented Nov 4, 2022

Agreed; by a literal reading of the definition it seems there's no way for a directly-referenced font to satisfy it, which I'm sure was never the intended outcome.

The spec is also clear (see the Note) that being considered the "first available font" is not dependent on actually having a glyph for the space character. So the font's character map (or its effective character map, in the case of a @font-face resource) is not taken into consideration. Only the unicode-range descriptor can disqualify a potential candidate.

My conclusion is that any font mentioned directly in the font-family list is a valid candidate for "first available font", without regard to its actual character repertoire, much like a @font-face resource with no unicode-range (because the initial value of the descriptor is the complete range of Unicode).

So you're right, the text of the spec should be clarified; but I don't think it should be changed to refer to character map.

@svgeesus
Copy link
Contributor

svgeesus commented Nov 4, 2022

We used to have the concept of a font database, and @font-face adding to that database. IIRC this was kind of like a hidden set of @font-face with local src, describing all the local fonts.

The implication of that model would be that the descriptors would have their initial values, so the unicode-range would be the initial "all of Unicode" range.

I wonder if we could usefully resurrect that concept?

Alternatively we could just use a negative:

The first available font, used for example in the definition of font-relative lengths such as ex or in the definition of the line-height property, is defined to be the first font whose glyph for the U+0020 (space) character is not excluded by unicode-range, given the font families in the font-family list (or a user agent’s default font, if none are available).

@jfkthame
Copy link
Contributor Author

jfkthame commented Nov 4, 2022

The first available font, used for example in the definition of font-relative lengths such as ex or in the definition of the line-height property, is defined to be the first font whose glyph for the U+0020 (space) character is not excluded by unicode-range, given the font families in the font-family list (or a user agent’s default font, if none are available).

The phrasing "whose glyph for the U+0020 (space) character" could be thought to imply that the font necessarily must have such a glyph.

Maybe something more like "the first font for which the character U+0020 (space) is not excluded by a unicode-range descriptor"?

Alternatively, we could keep the current text "the first font whose unicode-range includes the U+0020 (space) character", but add a note that "installed fonts referenced directly by family name, rather than via @font-face rules, are considered to have a unicode-range that covers the entire Unicode code space".

@nigelmegitt
Copy link

Both these solutions are slightly missing the point, that fonts don't have a unicode-range, only @font-face rules do. Nevertheless, there is some applicable set of code points associated with each available font, and it's that which we should reference. I'm neutral on whether we should care about an actual glyph being present at U+0020, but for computation of line height, we clearly do need to care that the font defines some size in the block progression direction that can be used.

@faceless2
Copy link

"installed fonts referenced directly by family name, rather than via @font-face rules, are considered to have a unicode-range that covers the entire Unicode code space".

This feels like a good solution to me - until this discussion I'd assumed that's how it worked anyway.

I don't think we should check if the local font has U+0020 actually defined. We explicitly don't do this for web-fonts, it would be inconsistent to require it for local fonts. If for any reason this behaviour needs customising it can be done so with

@font-face { font-family: "Arial"; src: local(Arial); unicode-range: nnn; }

@svgeesus
Copy link
Contributor

svgeesus commented Nov 7, 2022

"the first font for which the character U+0020 (space) is not excluded by a unicode-range descriptor"?

I like that more precise wording

@svgeesus
Copy link
Contributor

svgeesus commented Nov 7, 2022

"installed fonts referenced directly by family name, rather than via @font-face rules, are considered to have a unicode-range that covers the entire Unicode code space".

Good to make that explicit also. It is a common assumption, but the spec doesn't actually say anything like this.

jakearchibald pushed a commit to jakearchibald/csswg-drafts that referenced this issue Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants