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-size-adjust] Specify text-size-adjust more fully #4435

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

litherum
Copy link
Contributor

css-size-adjust-1/Overview.bs Outdated Show resolved Hide resolved
css-size-adjust-1/Overview.bs Outdated Show resolved Hide resolved
css-size-adjust-1/Overview.bs Outdated Show resolved Hide resolved
css-size-adjust-1/Overview.bs Outdated Show resolved Hide resolved
Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to get to this.

I have a bunch of specific comments noted in the text.

As a more general comment I'm a bit worried that this has moved a little bit too far away from defining a specific behavior and towards allowing perhaps too much flexibility.

I think there's a balance between multiple interests in favor of flexibility:

  • UAs that want to experiment with different behaviors
  • user needs or device characteristics that vary

and multiple interests in favor of more precision:

  • UAs that want to be able to implement a new implementation of this behavior in a web-compatible way
  • developer interest in testing in a reasonable manner (without having every possible device)
  • the interests of users who are on the needs or devices that are "different" and that the developer didn't test on or properly consider.

While I'm not sure exactly what the right balance is, I'm a little worried (as I said above) that these edits may be moving things too far towards one of these sides. I'm not quite sure what to do about that...

when a user does not zoom into the page, text size adjustment may still
be desirable. For example, if the viewport rules [[!CSS-DEVICE-ADAPT]]
describe a default zoom, text may need to be adjusted in order to be
readable at that default zoom level.
Copy link
Member

Choose a reason for hiding this comment

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

Is this specific to zoomed-out effects (zoom < 1.0), or is it more general than that? (I'm not sure I entirely understand the scenario this sentence is describing.)

@@ -44,6 +45,12 @@ Introduction {#intro}
time, this needs to be done with minimal disruption to the overall
design of the page.

Even on a display which is large enough to prevent horizontal scrolling,
Copy link
Member

Choose a reason for hiding this comment

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

I think you're better off without this comma, i.e., "a display which is large enough to prevent horizontal scrolling when the user does not zoom into the page".

I also don't think "prevent" is quite the right word here. Maybe "to prevent" -> "not to require", if that's what's intended? If it's not what's intended, then maybe something else?

Comment on lines -89 to -91
Issue: It's not clear how much this section should define
precise behavior versus how much it should allow future room for
innovation and improvement.
Copy link
Member

Choose a reason for hiding this comment

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

I think it might still be worth leaving this issue for now. Your edits certainly change the balance on this question, but it might still be worth thinking about further.

Comment on lines +122 to +123
Here are some criteria which User Agents may consider when making this
determination:
Copy link
Member

Choose a reason for hiding this comment

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

I think this list is still all worded in the form of things that would or might lead to a decision not to do text size adjustment. So I think it's still worth saying that, so it's clear. (The previous header for the list says that -- and you also said it explicitly inside the first item, but I think it's true for all the items. And if it's not, i.e., if some of the items are the other way around, then I think it's worth restructuring them to match the rest.)

<ul>
<li>when the total amount of text in the block formatting context (see
<li>When the total amount of text in the block formatting context (see
Copy link
Member

Choose a reason for hiding this comment

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

technically I think this should probably still be lowercase since they're not complete sentences. (They can still end with a period, since they're followed by complete sentences in many cases.)


<li>Element contents; empty elements might not benefit from being autosized</li>

<li>Elements which have compute styles that are fairly unique among the page. These are often structural
Copy link
Member

Choose a reason for hiding this comment

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

"compute" -> "computed"

<li>Elements which have compute styles that are fairly unique among the page. These are often structural
and might provide more broken layout than they benefit from size adjustment</li>

<li>Elements lay out to be extremely wide compared to the viewport. These would cause horizontal scrolling
Copy link
Member

Choose a reason for hiding this comment

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

probably something like "lay out to be" "whose width is"

Copy link
Member

Choose a reason for hiding this comment

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

That said, in Gecko we don't consider this a condition for disabling adjustment -- instead, we cap the width used to min(viewport inline-size, element inline-size) in this code. So it's perhaps worth mentioning that option as well?

'font-size'. This ratio must be at least the first divided by the
second; however, in order to maintain differentiations between font
sizes, it should often be slightly larger. <span class="issue">Define
this with more detail/precision.</span>
Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably worth retaining this text as an example, and perhaps adding another example if you have one. It may also be worth adding the "differentiations between font sizes" point to the list of criteria.

<dt><dfn>&lt;percentage&gt;</dfn></dt>
<dd>When displaying on a small device, renderers must not do size adjustment but instead the computed value of 'font-size' must be multiplied by this percentage.
<p class="note">Note: This means that 'text-size-adjust: 100%;' is equivalent to 'text-size-adjust: none;'.</p></dd>
<dd>Provided as a hint to the User Agent about a suggested adjustment amount. User agents may consider this percentage value when computing an adjusted text size.</dd>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a sufficient definition. It needs to say what the percentage means (which the old text did -- although it's possible it was wrong), even if it then says that UAs don't have to honor it. It would be better to say what the inputs into the decision of whether to honor it are.

Determining and Reacting to Adjustment Effects {#adjustment-reaction}
=====================================================================

This section is non-normative
Copy link
Member

Choose a reason for hiding this comment

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

Use <em>This section is non-normative.</em> (with em element and .).

Base automatically changed from master to main February 2, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants