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

Fix autocapitalize getter #5911

Merged
merged 1 commit into from Sep 16, 2020
Merged

Fix autocapitalize getter #5911

merged 1 commit into from Sep 16, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 14, 2020

This was tested and such as part of #3273, but the text is ambiguous and we didn't catch that in review. Thanks @smaug----!

cc @rlanday


/interaction.html ( diff )

@annevk annevk requested a review from domenic September 14, 2020 10:08
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Hmm. I don't really find the current text ambiguous. "The string value corresponding to sentences" is clearly "sentences". So the last step here seems to subsume the previous two.

Since this confused an implementer, we clearly need to do something, but I'm worried that the new text is also a bit confusing.

I guess we could match the reflection text and say "the keyword value associated with state"? At least then the two other steps are non-redundant, because such a phrase is ambiguous for the sentences state.

source Outdated
empty string.</p></li>

<li><p>If <var>state</var> is <span data-x="autocap-hint-none">none</span>, then return
<code data-x="attr-autocapitalize-none">none</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

quotes around strings, here and below?

Copy link
Member Author

Choose a reason for hiding this comment

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

And not the other instances of these keywords? Shall I add them everywhere for these keywords and we'll use that for new things and whenever someone touches a keyword that's a string?

Copy link
Member

Choose a reason for hiding this comment

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

Keywords are usually referenced as keywords. However, this is an IDL getter that returns a string. So it needs quotes. The fact that the thing inside those quotes is a keyword is interesting, and worth a cross-link, but it doesn't imply anything about other references to those keywords.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'd rather have all keywords marked up as strings all the time, but I did it this way for now.

@annevk annevk requested a review from domenic September 15, 2020 14:13
source Outdated Show resolved Hide resolved
@annevk annevk merged commit bf2ceeb into master Sep 16, 2020
@annevk annevk deleted the annevk/autocapitalize-getter branch September 16, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants