Make requirements for text offset props more precise

[dereklieu]

This PR partially addresses #8410.

• Adds text-field requirement to text-radial-offset and text-variable-anchor.
• Adds !: text-variable-anchor requirement to text-offset.
• Merges a duplicate requires field on text-anchor.

cc @mapbox/studio

[asheemmamoowala]

@dereklieu Thanks for this PR!

Based on your changes, text-variable-anchor will now be documented as:

And text-anchor as:

The docs should be clear as to which of these properties will take precedence and be applied in the event that both are present. In symbol_layout.js text-variable-anchor is given precedence if found on the layer style.

[dereklieu]

@asheemmamoowala that's a great point. Is precedence of one property over another something that is the responsibility of the style spec to communicate? If not, what are next steps to making sure the docs reflect this?

[andrewharvey]

@dereklieu

If your PR affects documentation relevant to the currently released version, please use publisher-production as the base branch. See https://github.com/mapbox/mapbox-gl-js/blob/master/docs/README.md#committing-and-publishing-documentation

Should this PR target publisher-production or does it relate to changes only in master, not yet part of a release?

[mourner]

@andrewharvey in this case, I think we should target master because the main purpose is to propagate the constraints to Studio, which will happen through a new release of mapbox-gl-style-spec. The docs will catch up as a part of the next release (since it's not urgent to update them).

[dereklieu]

Yep, that is indeed the goal. Let me know what else I can contribute here. Thanks for the feedback so far.

[asheemmamoowala]

Is precedence of one property over another something that is the responsibility of the style spec to communicate?

It is, in that the spec is converted automatically into documentation. There are a couple ways we can go about this:

• undo the Adds !:text-variable-anchor requirement to text-offset. portion of this PR. This is what is done in the case of *-color/*-pattern properties where that pattern property takes precedence.
• keep the change, and let the docs be ambiguous about which takes priority when both are present
• Update the spec to use a new way to specify when a property disables another, i.e takes precedence in the priority. This would also require updating the documentation generation.

I suggest going with the first options to keep the docs consistent, and we consider a separate PR for the third option.

[dereklieu]

@asheemmamoowala thanks for the recommendation! I think the ambiguity in the docs currently extends to two property pairs:

• text-offset and text-radial-offset
• text-anchor and text-variable-anchor

In both cases, the docs tell you one can't be used with the other, but if both are present, GL will make a choice. I agree this choice should be documented.

In these cases, I think a good way forward is that the overriding property should have fewer requirements. The overriding property can exist in the presence of the secondary property. The secondary property is literally disabled in the presence of the overriding property.

text-anchor: {
  requires: [
    'text-field',
    { !: 'text-variable-anchor }
  ]
},
text-variable-anchor: {
  requires: [
    'text-field',
    { "symbol-placement": [ "point" ]
  ]
},
text-offset: {
  requires: {
    'text-field',
    { !: 'text-radial-offset' },
    { !: 'text-variable-anchor' }
  }
},
text-radial-offset: {
  requires: {
    'text-field'
  }
}

undo the Adds !:text-variable-anchor requirement to text-offset.

I think we should keep this. text-anchor is disabled by text-variable-anchor. However, I think we should relax the requirement of text-variable-anchor, which currently includes ! text-offset, since having a text-offset doesn't stop you from overriding it with a variable anchor.

Let me know if this makes sense to you - if so I will adjust the PR. Thanks again for your help and consideration here!

[asheemmamoowala]

Let me know if this makes sense to you - if so I will adjust the PR. Thanks again for your help and consideration here!

Sounds good to me!

[dereklieu]

👍 Those changes are reflected in the last commit.

[chloekraw]

Hi, I realize I’m jumping in late here and I’m not sure of the visual effect that Derek's last commits will have. @dereklieu could you add another screenshot?

I also don’t have a great understanding of how the text of our docs are generated. Is all of the text customizable? I’m wondering if it’s an option to make the overriding explicit for the docs on text-radial-offset? So where Derek’s screenshot in #8418 (comment) says “Disabled by text-offset”, we write “Overrides text-offset”? IMO that’s the clearest way to document this behavior.

[dereklieu]

Hey @chloekraw I've attached some screenshots below that show the documentation wording that this would generate.

I agree that the override language clarifies the behavior. However, I believe the Disabled by {property_name} language is automatically generated, and that adding the override means a new property in the style spec and new parsing logic in the documentation generator, which seems beyond the scope of this PR.

An alternative is to include the override language in the doc portion, which is defined in the style spec.

[chloekraw]

Thanks @dereklieu, looks good to me!